Skip to content
This repository has been archived by the owner on Nov 14, 2024. It is now read-only.

[WIP] Shade Feign 8.17 #2054

Closed
wants to merge 14 commits into from
Closed

[WIP] Shade Feign 8.17 #2054

wants to merge 14 commits into from

Conversation

jeremyk-91
Copy link
Contributor

@jeremyk-91 jeremyk-91 commented Jun 20, 2017

Goals (and why): Shade Feign, so that several internal products (including shopping and blob store) will not explode from dependency conflicts. In the current state this is largely an outline of how I intend to implement this across Feign-using AtlasDB projects.

  • Finish adding feign-shading to all feign-using projects.
  • Test that we can still deploy timelock after this change.
  • Test that we can use internal shopping product after this change with no hiccups.
  • Release notes.

Implementation Description (bullets):

  • We introduce a concept of 'lean' jars, which are built without any form of shadowing and used internally (even for things like AtlasDB Config where we currently always shadow). This allows us to easily depend on classes pulled in by our projects without worrying about scenarios like this:
        PingableLeader ping = Feign.builder()
                .decoder(new TextDelegateDecoder(new JacksonDecoder()))
// fails, because a com.palantir.shaded.JacksonDecoder is not a feign.Decoder (even though it is a com.palantir.shaded.Decoder)
  • So for each project, we internally depend on lean jars where necessary, but we only publish shadowed jars. The shadowing takes place in feign-shading.gradle plus possibly other dependency rules in projects that already publish shadow jars (atlasdb-cassandra and atlasdb-config, most notably).
  • Generally dependencies should use the explicitShadow configuration (be used both at compile time and at runtime in the shadow jar). The Feign dependencies, however, should use the compile configuration (they should not be present at runtime in the shadow jar, because they are being shaded away).
  • There are some slightly trickier shenanigans for Dagger and Remoting2, where projects already shade some of their dependencies and we want to keep them shaded. The daggerShadowJar and remotingShadowJar basically shade these components, but not Feign.

Concerns (what feedback would you like?):

  • Will we accidentally publish the lean jars? My understanding of publish-jars.gradle suggests that it's able to pick the shadow jar if one exists. Publication was a bit funky because of an API break in Shadow in 2.0.0, but looking at my Maven caches after a publishToMavenLocal we do indeed publish the right jars.
  • I'm removing the prohibition on generating the non-shadow/lean jar for atlasdb-config and atlasdb-cassandra (with the understanding that only the shadow jar will ever be published). For config given that this only touches a bunch of (very stable) Remoting2 classes I think we're safe. I'm not so sure about Cassandra.
  • Is there a better way to do this?

Where should we start reviewing?: feign-shading.gradle

Priority (whenever / two weeks / yesterday): ASAP; is a release-blocker.


This change is Reviewable

@codecov-io
Copy link

codecov-io commented Jun 20, 2017

Codecov Report

Merging #2054 into develop will increase coverage by 4.85%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #2054      +/-   ##
=============================================
+ Coverage      57.77%   62.62%   +4.85%     
- Complexity      4066     4068       +2     
=============================================
  Files            754      754              
  Lines          37240    37240              
  Branches        4028     4028              
=============================================
+ Hits           21514    23322    +1808     
+ Misses         14295    12387    -1908     
- Partials        1431     1531     +100
Impacted Files Coverage Δ Complexity Δ
.../atlasdb/transaction/impl/AbstractTransaction.java 60% <0%> (-20%) 3% <0%> (ø)
...in/java/com/palantir/paxos/PaxosAcceptorState.java 60.52% <0%> (-18.43%) 0% <0%> (ø)
...r/atlasdb/schema/stream/StreamStoreDefinition.java 55.55% <0%> (-14.82%) 4% <0%> (ø)
...tlasdb/transaction/impl/ForwardingTransaction.java 73.07% <0%> (-11.54%) 14% <0%> (ø)
...ain/java/com/palantir/paxos/PaxosAcceptorImpl.java 62.5% <0%> (-10.72%) 0% <0%> (ø)
...a/com/palantir/atlasdb/ptobject/EncodingUtils.java 74.74% <0%> (-5.06%) 62% <0%> (ø)
...in/java/com/palantir/paxos/PaxosQuorumChecker.java 71.18% <0%> (-3.39%) 0% <0%> (ø)
.../impl/ValidatingQueryRewritingKeyValueService.java 35.04% <0%> (-2.57%) 21% <0%> (ø)
...com/palantir/atlasdb/table/description/Schema.java 76.3% <0%> (-2.32%) 26% <0%> (ø)
.../atlasdb/stream/AbstractPersistentStreamStore.java 76.34% <0%> (-2.16%) 17% <0%> (ø)
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a068d92...10f2953. Read the comment docs.

@@ -1153,12 +1156,6 @@
"com.netflix.feign:feign-core"
]
},
"org.mortbay.jetty.alpn:jetty-alpn-agent": {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add this back as a runtime dep

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(or not, already added in the distribution project)

Copy link
Contributor

@gmaretic gmaretic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! As discussed offline, would be good to document how exactly the dagger/remoting shading works with feign shading!

@jeremyk-91
Copy link
Contributor Author

jeremyk-91 commented Jun 21, 2017

This won't actually work because clients often pull in multiple AtlasDB jars, so will have multiple copies of the Feign classes. Also there's a lot of hackery with getting the nebula dependency recommender and the shadow plugin to play nicely with each other.

Was hoping not to have to do some surgery on the src/java itself, but looks like we'll have to! The plan going forward is to set up a new atlas-feign project and use that instead.

Closing out. 😞 I guess I've learned a fair bit about shadowing and Gradle though!

@jeremyk-91 jeremyk-91 closed this Jun 21, 2017
@jeremyk-91
Copy link
Contributor Author

(N.B. Please don't delete the branch until the new fix merges.)

@jeremyk-91 jeremyk-91 mentioned this pull request Jun 21, 2017
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants