Shade Feign II

[jeremyk-91]

Goals (and why): (copypasted from Shade Feign I: #2054)
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):
Much simpler than Shade Feign I.

• Introduce a new project atlasdb-feign that absorbs all the HTTP things.
• The new project shades Feign, and explicitShadows OkHttp.
• The public API of AtlasDbHttpClients is unchanged.

Concerns (what feedback would you like?):

• Did I miss anything when porting over half of the logic from AtlasDbHttpClients to AtlasDbFeignTargetFactory?
• Whenever we want to use Feign we have to go through atlasdb-feign now. Referencing the shaded jar is unfortunately less pleasant than I hoped - IntelliJ IDEA unable to resolve classes of project dependency shadow jar GradleUp/shadow#264 and https://youtrack.jetbrains.com/issue/IDEA-163411
• atlasdb-service-server's tests are a little janky, but I didn't feel like exposing a custom Retryer other than our FailoverFeignTarget for the feign target factory.
• Is there a better way to do this?

Where should we start reviewing?: atlasdb-feign/build.gradle

Priority (whenever / two weeks / yesterday): ASAP, release blocking

---

This change is 

[codecov-io]

Codecov Report

Merging #2061 into develop will decrease coverage by 0.06%.
The diff coverage is 5.88%.

@@              Coverage Diff              @@
##             develop    #2061      +/-   ##
=============================================
- Coverage      57.73%   57.67%   -0.07%     
- Complexity      4076     4128      +52     
=============================================
  Files            754      755       +1     
  Lines          37256    37262       +6     
  Branches        4031     4031              
=============================================
- Hits           21511    21491      -20     
- Misses         14310    14340      +30     
+ Partials        1435     1431       -4

Impacted Files	Coverage Δ	Complexity Δ
...com/palantir/atlasdb/http/TextDelegateDecoder.java	100% <ø> (ø)	0 <0> (?)
...ain/java/com/palantir/atlasdb/http/UserAgents.java	77.77% <ø> (ø)	0 <0> (?)
...alantir/atlasdb/http/SerializableErrorDecoder.java	78.57% <ø> (ø)	0 <0> (?)
...com/palantir/atlasdb/http/AtlasDbErrorDecoder.java	90.47% <ø> (ø)	0 <0> (?)
...com/palantir/atlasdb/http/FailoverFeignTarget.java	68.81% <ø> (ø)	0 <0> (?)
...palantir/atlasdb/http/ExceptionRetryBehaviour.java	94.44% <ø> (ø)	0 <0> (?)
.../com/palantir/atlasdb/http/FeignOkHttpClients.java	25.92% <ø> (ø)	0 <0> (?)
.../atlasdb/http/errors/AtlasDbStackTraceElement.java	100% <ø> (ø)	0 <0> (?)
...ir/atlasdb/http/errors/AtlasDbRemoteException.java	92.3% <ø> (ø)	0 <0> (?)
...lantir/atlasdb/http/AtlasDbFeignTargetFactory.java	0% <0%> (ø)	0 <0> (?)
... and 25 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 14526c5...4ab842f. Read the comment docs.

[nziebart]

what is the difference between shadow, explicitShadow, and toBeShaded here?

[jeremyk-91]

Good question. It's a bit messy:

• shadow refers to dependencies that we expect to be available on the classpath at runtime, if one is running with the shadow JAR.
• compile refers to dependencies needed at compile time.
• explicitShadow is a thing we use to mean "needed at both compile time and runtime-with-shadow-JAR" - the shadow configuration doesn't automatically inherit from compile. Usually this is for dependencies we are not shading. If not we have to write each dependency twice.
• toBeShaded is purely for readability (I find 'compile' meaning 'not in the shadow jar at runtime' pretty unintuitive, especially since normal runtime inherits from compile). This would be functionally unchanged if we just used compile.

[gmaretic]

You should probably document this somewhere.

[jeremyk-91]

I've integrated the above into the feign project's build.gradle.

[nziebart]

The AtlasDbHttpClients -> AtlasDbFeignTargetFactory changes look good. Is the reason for that change that we want to contain all shaded feign references to that project?

[jeremyk-91]

@nziebart Yep. Dealing with shaded classes outside a place where it's shaded is OK in Gradle, but Idea will complain horribly - see GradleUp/shadow#264 and https://youtrack.jetbrains.com/issue/IDEA-163411.

We could have just shaded it in the projects they already are in (that's client, config and tests-shared) but I prefer this arrangement, mainly for 2 reasons:

• Keep the logic in one place. This can also be done with a gradle header file of some kind though.
• It's perfectly reasonable for someone to pull in both of these (timelock pulls both client and config) and I'd be wary of class-loading problems.

Also I always found it a bit strange that a lot of the http stuff was in a project called config, so I think sticking it in its own project makes sense, and was something worth doing as part of this issue.

[gmaretic]

LGTM! Of course, provided we made sure that internal products pass their tests with some reasonable effort -- will discuss offline.