Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[tchannel-java] Upgrade OT to 0.31; Jaeger to 0.23 #189

Merged
merged 2 commits into from
Jan 21, 2018
Merged

[tchannel-java] Upgrade OT to 0.31; Jaeger to 0.23 #189

merged 2 commits into from
Jan 21, 2018

Conversation

dray92
Copy link
Contributor

@dray92 dray92 commented Jan 20, 2018

Signed-off-by: Debosmit Ray debo@uber.com

@debosmit
[tchannel-java] Upgrade OT to 0.31; Jaeger to 0.23
b5b26c0 
Signed-off-by: Debosmit Ray <debo@uber.com>
@dray92
Copy link
Contributor Author

dray92 commented Jan 20, 2018

Solves #188

yurishkuro
yurishkuro approved these changes Jan 21, 2018
View reviewed changes
pom.xml Outdated
</dependency>
<dependency>
<groupId>com.uber.jaeger</groupId>
<artifactId>jaeger-core</artifactId>
<version>0.21.0</version>
<version>0.23.0</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

we should consider moving this into tchannel-crossdock module, rather than claiming it a dependency of the whole tchannel

@debosmit
reduce jaeger scope to tchannel-core and tchannel-crossdock modules
0af08d7 
Signed-off-by: Debosmit Ray <debo@uber.com>
dray92
dray92 commented Jan 21, 2018
View reviewed changes
tchannel-crossdock/pom.xml
@@ -23,6 +23,13 @@
<dependency>
<groupId>com.uber.jaeger</groupId>
<artifactId>jaeger-core</artifactId>
<version>${com.uber.jaeger.version}</version>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yurishkuro looks better now?

@codecov-io
Copy link

Codecov Report

Merging #189 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #189   +/-   ##
=========================================
  Coverage     70.98%   70.98%           
  Complexity       10       10           
=========================================
  Files            87       87           
  Lines          2653     2653           
  Branches        314      314           
=========================================
  Hits           1883     1883           
  Misses          551      551           
  Partials        219      219

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 5a8de89...0af08d7. Read the comment docs.

@yurishkuro
Copy link
Contributor

:shipit:

@yurishkuro yurishkuro merged commit 0a63b21 into master Jan 21, 2018
yborovikov
yborovikov reviewed Jan 21, 2018
View reviewed changes
Copy link
Contributor

@yborovikov yborovikov left a comment

Choose a reason for hiding this comment

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

the only change required was to update the jaeger version (and add the gson exclusion, if needed).

pom.xml
@@ -88,6 +88,7 @@
<jmh.version>1.19</jmh.version>
<se.eris.notnull.instrument>false</se.eris.notnull.instrument><!-- off by default -->
<slf4j.version>1.7.25</slf4j.version>
<com.uber.jaeger.version>0.23.0</com.uber.jaeger.version>
Copy link
Contributor

Choose a reason for hiding this comment

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

this indirection is redundant - use the version number where it's needed (1 place).

Copy link
Contributor

Choose a reason for hiding this comment

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

resolved

pom.xml
<dependency>
<groupId>com.uber.jaeger</groupId>
<artifactId>jaeger-core</artifactId>
<version>0.21.0</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

dependencyManagement section defines dependency artifact versions (and exclusions) for the whole hierarchy of the project poms - bring it back (with the new gson exclusion, if needed).

Copy link
Contributor

Choose a reason for hiding this comment

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

resolved

tchannel-core/pom.xml
@@ -87,6 +87,13 @@
<dependency>
<groupId>com.uber.jaeger</groupId>
<artifactId>jaeger-core</artifactId>
<version>${com.uber.jaeger.version}</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

the version (and exclusions) is defined in the parent pom dependency management section. remove it from here.

Copy link
Contributor

Choose a reason for hiding this comment

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

done

tchannel-crossdock/pom.xml
@@ -23,6 +23,13 @@
<dependency>
<groupId>com.uber.jaeger</groupId>
<artifactId>jaeger-core</artifactId>
<version>${com.uber.jaeger.version}</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the version and exclusions. these come from parent pom dependency management.

Copy link
Contributor

Choose a reason for hiding this comment

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

done

yborovikov added a commit that referenced this pull request Jan 21, 2018
@yborovikov
Revert "Upgrade to OpenTracing 0.31; Jaeger 0.23 (#189)"
d0695c4 
This reverts commit 0a63b21.
@yborovikov yborovikov mentioned this pull request Jan 21, 2018
Closed
@dray92 dray92 mentioned this pull request Jan 21, 2018
Merged
debosmit added a commit that referenced this pull request Jan 21, 2018
@debosmit
Merge pull request #191 from uber/jaeger_0.23
751c3c6 
addendum to #189, ref #190
debosmit
debosmit reviewed Jan 21, 2018
View reviewed changes
Copy link
Contributor

@debosmit debosmit left a comment

Choose a reason for hiding this comment

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

addressed in #191

pom.xml
@@ -88,6 +88,7 @@
<jmh.version>1.19</jmh.version>
<se.eris.notnull.instrument>false</se.eris.notnull.instrument><!-- off by default -->
<slf4j.version>1.7.25</slf4j.version>
<com.uber.jaeger.version>0.23.0</com.uber.jaeger.version>
Copy link
Contributor

Choose a reason for hiding this comment

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

resolved

pom.xml
<dependency>
<groupId>com.uber.jaeger</groupId>
<artifactId>jaeger-core</artifactId>
<version>0.21.0</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

resolved

tchannel-core/pom.xml
@@ -87,6 +87,13 @@
<dependency>
<groupId>com.uber.jaeger</groupId>
<artifactId>jaeger-core</artifactId>
<version>${com.uber.jaeger.version}</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

done

tchannel-crossdock/pom.xml
@@ -23,6 +23,13 @@
<dependency>
<groupId>com.uber.jaeger</groupId>
<artifactId>jaeger-core</artifactId>
<version>${com.uber.jaeger.version}</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@debosmit debosmit debosmit left review comments

@yborovikov yborovikov yborovikov left review comments

@yurishkuro yurishkuro yurishkuro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dray92 @codecov-io @yurishkuro @yborovikov @debosmit