Java thrift library hash key for cache to include thrift fields

Review Request #2258 — Created May 22, 2015 and discarded

chrischen
pants
areitz, ity

This rb fixes the issue which pants cache for java thrift library does not include language, compiler and rpc style in its hash key generation. As a result, if any of those values changes, pants will throw an error due to cache.

For example, if a java_thrift_library target was compiled, but then changed its rpc style from sync to fingle, and compile again. Since hash didn't change, pants doesn't invalidate the target. Pants would eventually return an error because it couldn't find the file at the cached location.
To provide a more specific error, it would try to find a file like
.pants.d/gen/scrooge/java-finagle/gen-file-map-by-target/src.thrift.com.twitter.bouncer.bounce-action-thrift-java
but the actual file is .pants.d/gen/scrooge/java-sync/gen-file-map-by-target/src.thrift.com.twitter.bouncer.bounce-action-thrift-java

The fix is to
1) Add compiler, language, rpc style to payload, so they are part of the hash.
2) Fold java thrift library options in global_options into java_thrift_library since they are used as defaults for this class now.

See the relevent discussion here https://groups.google.com/forum/#!topic/pants-devel/9I2oiTNmOhg

https://github.com/pantsbuild/pants/pull/1577
./pants test tests/python/pants_test/targets:java_thrift_library
./pants test contrib/scrooge/tests/python/pants_test/contrib/scrooge/tasks

  • 5
  • 0
  • 0
  • 0
  • 5
Description From Last Updated
Chatted with Chris. He is going to bring back options the same way as they were before so that rpc_style, ... IT ity
With this rb, the defaults cannot be override by any config? is that right? Changing compilers or rpc_style will now ... TE tejal
See src/python/pants/backend/jvm/targets/jvm_target.py for how to extend a payload. You need to accept an optional payload as an argument from potential ... PA patricklaw
Why not pull these off of the payload to avoid redundancy? PA patricklaw
I still need to support these options, but I need to migrate them to java_thrift_library. I want to make sure ... CH chrischen
CH
  1. 
      
  2. I still need to support these options, but I need to migrate them to java_thrift_library. I want to make sure the approach is ok in general.

  3. 
      
CH
PA
  1. The general thrust of this change looks good to me, and I don't think Foursquare has much internally that could interfere with this change.

  2. See src/python/pants/backend/jvm/targets/jvm_target.py for how to extend a payload. You need to accept an optional payload as an argument from potential subclasses, and then default to a new Payload() if that's None.

    1. Yeah, I agree with this one.

  3. Why not pull these off of the payload to avoid redundancy?

    1. Sorry, poorly worded: by "pull off of" I mean "read off of", and get rid of the attributes.

  4. 
      
AR
  1. LGTM, I think making java_thrift_library take payload as an argument is good feedback.

  2. 
      
TE
  1. 
      
  2. With this rb, the defaults cannot be override by any config?
    is that right?

    Changing compilers or rpc_style will now need a code change.
    I will have to updating all targets to override the code defaults?

    I think the defaults make sense for twitter, bit not neessarily for non twitter world.

  3. 
      
CH
  1. I am going to write another implementation using FingerprintStrategy which will be cleaner and has better support for options.

  2. 
      
IT
  1. 
      
  2. Chatted with Chris.

    He is going to bring back options the same way as they were before so that rpc_style, language, compiler can all be configurable (as they were before).

  3. 
      
CH
Review request changed

Status: Discarded

CH
  1. Implemented a different approach which was submitted.

  2. 
      
Loading...