An artifact cache subsystem.

Review Request #2405 — Created June 22, 2015 and submitted

benjyw
pants
feec84c...
pants-reviews
stuhood, zundel

This was no small endeavor, as the artifact cache code had
quite a few implicit tentacles out into the codebase.

This creates a CacheSetup subsystem, which in turn gives tasks
a CacheFactory, which is the thing that actually creates caches, based
on option values. The CacheFactory is necessary in order to encapsulate
non-option arguments such as the logger and the stable_name.

The resulting code is simpler and easier to grok (although the diffs
may not be). For example, tasks no longer need to explicitly set up
the cache; It gets set up as needed. Also, code that needs the read
cache accesses it directly, and ditto the write cache. The confusing
ReadWriteArtifactCache class is no longer needed, and has been deleted.

Also simplifies names of options and variables: 'artifact_cache'
is a mouthful, when there's only one kind of cache that 99% of users
care about - namely this one.

This is the first case we have of task-specific Subsystems, so there
are a few tweaks here and there to get that working properly.

This pecial-cases migration of the old cache-related options, which
required a little refactoring in migrate_config.py.

Lots of manual testing to verify that the cache ends up used as expected with the new config locations. Also checked that the use of the cache to optimize tool bootstrapping in tests still works.

CI went green here: https://travis-ci.org/pantsbuild/pants/builds/68066786

ST
  1. Awesome cleanup... thanks Benjy! We have an intern this summer who we've tasked with adding caching in a few places (and thinking about how to improve that process) and this helps immensely.

  2. IIRC, this field is now explicitly the level. Could take the opportunity to rename to compression_level

    1. Not a bad idea. Done.

  3. src/docs/setup_repo.md (Diff revision 1)
     
     

    to the cache to to a particular cache

  4. s/call a/call a/

    And to the latter: "here here!"

  5. src/python/pants/cache/cache_setup.py (Diff revision 1)
     
     
     

    Thread safety needed here?

    1. To protect the assignment to self._read_cache/self._write cache. I guess so, yes. Fixed. 
      
      I'm not actually sure why this lock is needed, but it was there before, presumably for a good reason.
  6. src/python/pants/cache/cache_setup.py (Diff revision 1)
     
     

    Should this be using six somehow?

    1. Good point. I thought basestring was a thing in python3, but apparently it isn't. Fixed. There are a couple more places where we use it that I'll clean up in a separate change.

  7. 
      
ZU
  1. Ship It!
  2. 
      
BE
BE
  1. Thanks for the reviews! Will push after one more CI run with my followup changes, just in case.

  2. 
      
BE
BE
Review request changed

Status: Closed (submitted)

Change Summary:

Submitted as 4794f58b5e766ff67d1d8304f08a43d545c1a14a.

Loading...