Make CacheKey hash and pickle friendly

Review Request #986 — Created Sept. 3, 2014 and submitted

davidt
pants
acbc647...
pants-reviews
benjyw, johanoskarsson

Don't need payloads on cache_keys, so removing them makes them pickle-friendly.

tests/python/pants_test/cache::

Passed: https://travis-ci.org/pantsbuild/pants/builds/38538962

  • 0
  • 0
  • 2
  • 1
  • 3
Description From Last Updated
BE
  1. 
      
  2. Maybe rename the ctor arg to _hash. I don't like shadowing builtins, especially since you use that builtin a few lines later, which makes this code confusing.

  3. Do you not want a _setstate_ for symmetry and so you can set num_chunking_units and payloads to... something? 0 and None I guess? Otherwise IIRC they will simply be missing from the object's __dict__.

    Actually, now that I think of it, this is problematic either way, as code will assume those two are set to something useful, but that won't be true after unpickling.

    Maybe we need a bit of a refactor here, e.g., a CacheKey that only includes the id and hash, and then some wrapper around it that adds num_chunking_units and payloads, for when those are needed?

    That seems like a better separation of concerns and will also avoid confusion due to pickling.

    1. Turns out we never need cachekey's payloads (the only usage also has targets and can get there there instead), so i just removed 'em and left the rest untouched.

  4. 
      
ST
  1. 
      
  2. Please add comments (on either the class or methods) indicating why these methods are implemented.

  3. 
      
DA
DA
DA
DA
JO
  1. I haven't kept up enough to know why we need this to be pickle friendly, but it sounds like a good change either way.

  2. 
      
DA
DA
DA
BE
  1. AFAICT you didn't actually remove the payloads field from CacheKey in build_invalidator.py though?

    1. I believe I did: https://rbcommons.com/s/twitter/r/986/diff/4/#1

    2. Well that's weird. I didn't see that file in the diffs I was looking at earlier, but it does appear now.

  2. 
      
PA
  1. 
      
  2. This used to say the number of sources. It was a mistake for me to print number of payloads--instead we should print the sum of t.num_chunking_units which in practice right now will be number of sources. In the future it might be bytes or whatever else.

    1. Eh, the goal of this change is to change no functionality or bahavior at all, so I can fix that but would prefer to do it in a followup.

    2. sgtm

  3. 
      
DA
Review request changed

Status: Closed (submitted)

Change Summary:

bdf68f00a6bdeeef8c165ce6e7abe021297760f9 
BE
  1. Ship It!

  2. 
      
Loading...