[engine] skip caching for native nodes

Review Request #3581 — Created March 17, 2016 and submitted

kwlzn, patricklaw, stuhood

Native nodes should not be cached.

This review

  • Introduces a _should_cache method in Engine which has one predicate today based on Node type.
  • An items() interface for testing purpose, to allow iterate over all cached key, values.


  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
  1. Thanks!

  2. src/python/pants/engine/exp/nodes.py (Diff revision 1)

    Rather than a classmethod, why not make this a property of a Node instance, and then have subclasses override it?

    def is_cacheable(self):
    1. done. Have nodes override is_cacheable

  3. src/python/pants/engine/exp/storage.py (Diff revision 1)

    Since __init__ is only supposed to be called from create, it doesn't make sense for it to have default arguments.

    1. Well, not for type_ probably?

      Also, not clear why you made this change.

    2. without the default, which I reverted.

      the rest is super minor, just because digest and hash are related, so i placed them next to each other.

  2. src/python/pants/engine/exp/storage.py (Diff revision 2)

    seems like directly accessing Key._32_BIT_STRUCT here violates the private convention - should _32_BIT_STRUCT get renamed to something non-private?

    or should Key get a public @classmethod that does a: return self._32_BIT_STRUCT.unpack(digest)[0]?

    1. good catch, i will make it a classmethod

    2. Mmm... +1.

      This should likely be a separate factory function for Key... Key.create_from_digest() or something.

    3. Done, refactored into Key.compute_hash_from_digest.

Review request changed

Status: Closed (submitted)

Change Summary:

Submitted as 0dbdf54e86b7285ceb08c62c5183d6362e285788