Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

Make jaeger-thrift's shaded JAR the default one #461

Conversation

jpkrohling
Copy link
Collaborator

Signed-off-by: Juraci Paixão Kröhling [email protected]

Which problem is this PR solving?

Short description of the changes

  • Changed classifiers for both the jar task and shadowJar
  • Changed shadowed dependencies to compileOnly so that depending projects won't transitively depend on Thrift and OkHttp.
  • Changed the README to be accurate with the current situation (plus some minor improvements)

A project depending on jaeger-thrift with this change will have this automatically pulled:

[INFO] +- io.jaegertracing:jaeger-thrift:jar:0.29.1-SNAPSHOT:compile
[INFO] |  \- io.jaegertracing:jaeger-core:jar:0.29.1-SNAPSHOT:compile
[INFO] |     +- io.opentracing:opentracing-api:jar:0.31.0:compile
[INFO] |     +- io.opentracing:opentracing-util:jar:0.31.0:compile
[INFO] |     +- com.google.code.gson:gson:jar:2.8.2:compile
[INFO] |     \- org.slf4j:slf4j-api:jar:1.7.25:compile

And without this change:

[INFO] +- io.jaegertracing:jaeger-thrift:jar:0.29.1-SNAPSHOT:compile
[INFO] |  +- io.jaegertracing:jaeger-core:jar:0.29.1-SNAPSHOT:compile
[INFO] |  |  +- io.opentracing:opentracing-api:jar:0.31.0:compile
[INFO] |  |  +- io.opentracing:opentracing-util:jar:0.31.0:compile
[INFO] |  |  +- com.google.code.gson:gson:jar:2.8.2:compile
[INFO] |  |  \- org.slf4j:slf4j-api:jar:1.7.25:compile
[INFO] |  +- org.apache.thrift:libthrift:jar:0.11.0:compile
[INFO] |  |  +- org.apache.httpcomponents:httpclient:jar:4.5.3:compile
[INFO] |  |  |  \- commons-codec:commons-codec:jar:1.10:compile
[INFO] |  |  \- org.apache.httpcomponents:httpcore:jar:4.4.8:compile
[INFO] |  \- com.squareup.okhttp3:okhttp:jar:3.9.0:compile
[INFO] |     \- com.squareup.okio:okio:jar:1.13.0:compile

@objectiser
Copy link
Contributor

@jpkrohling May not have time to review until later this week. One question - why not shade gson?

@jpkrohling
Copy link
Collaborator Author

We do shade it, but it's a transitive dependency coming from jaeger-core. We could declare it directly as a dependency of jaeger-thrift as compileOnly as well, even though the module itself never directly consumes it.

@objectiser
Copy link
Contributor

Sorry, ignore my comment - didn't look closely enough to the dependency tree.

@jpkrohling jpkrohling force-pushed the 460-Make-thrift-shaded-jar-as-default branch from 03b0900 to e95cf8f Compare June 20, 2018 13:44
Copy link
Contributor

@objectiser objectiser left a comment

Choose a reason for hiding this comment

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

LGTM - do we need tests for the no-shadow jar?

@jpkrohling
Copy link
Collaborator Author

LGTM - do we need tests for the no-shadow jar?

I think so, probably as part of the crossdock. @jkandasa, would you like to do it, so that you get familiar with how this kind of test is done upstream?

@jpkrohling jpkrohling changed the title Make jaeger-thrift's shaded JAR the default one [WIP] Make jaeger-thrift's shaded JAR the default one Jun 20, 2018
@jpkrohling
Copy link
Collaborator Author

By the way: I'm marking this as WIP, as the crossdock seem to be failing consistently.

@jpkrohling jpkrohling force-pushed the 460-Make-thrift-shaded-jar-as-default branch from e95cf8f to 12f7cc3 Compare June 21, 2018 08:56
@codecov
Copy link

codecov bot commented Jun 21, 2018

Codecov Report

Merging #461 into master will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #461      +/-   ##
============================================
+ Coverage     88.24%   88.29%   +0.05%     
- Complexity      488      489       +1     
============================================
  Files            63       63              
  Lines          1846     1846              
  Branches        240      240              
============================================
+ Hits           1629     1630       +1     
  Misses          139      139              
+ Partials         78       77       -1
Impacted Files Coverage Δ Complexity Δ
...aegertracing/samplers/RemoteControlledSampler.java 90.69% <0%> (+1.16%) 18% <0%> (+1%) ⬆️

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 e76b6a9...f5a1097. Read the comment docs.

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
@jpkrohling jpkrohling force-pushed the 460-Make-thrift-shaded-jar-as-default branch from 12f7cc3 to f5a1097 Compare June 21, 2018 09:39
@jpkrohling jpkrohling changed the title [WIP] Make jaeger-thrift's shaded JAR the default one Make jaeger-thrift's shaded JAR the default one Jun 21, 2018
@jpkrohling
Copy link
Collaborator Author

For the record, I tested this manually by running the customer service from the istio-tutorial with the following dependencies, after running a ./gradlew clean check install:

        <dependency>
            <groupId>io.jaegertracing</groupId>
            <artifactId>jaeger-thrift</artifactId>
            <version>0.29.1-SNAPSHOT</version>
        </dependency>
        <dependency>
            <groupId>io.jaegertracing</groupId>
            <artifactId>jaeger-tracerresolver</artifactId>
            <version>0.29.1-SNAPSHOT</version>
        </dependency>

This is the relevant dependency:tree section:

[INFO] +- io.jaegertracing:jaeger-thrift:jar:0.29.1-SNAPSHOT:compile
[INFO] |  \- io.jaegertracing:jaeger-core:jar:0.29.1-SNAPSHOT:compile
[INFO] |     +- io.opentracing:opentracing-api:jar:0.31.0:compile
[INFO] |     +- io.opentracing:opentracing-util:jar:0.31.0:compile
[INFO] |     +- com.google.code.gson:gson:jar:2.8.2:compile
[INFO] |     \- org.slf4j:slf4j-api:jar:1.7.25:compile
[INFO] +- io.jaegertracing:jaeger-tracerresolver:jar:0.29.1-SNAPSHOT:compile
[INFO] |  \- io.opentracing.contrib:opentracing-tracerresolver:jar:0.1.4:compile

@jpkrohling jpkrohling merged commit f5a1097 into jaegertracing:master Jun 21, 2018
@ghost ghost removed the review label Jun 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants