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

Updates of runtime components #286

Merged
merged 1 commit into from
Nov 20, 2017
Merged

Updates of runtime components #286

merged 1 commit into from
Nov 20, 2017

Conversation

alshopov
Copy link
Contributor

@alshopov alshopov commented Nov 9, 2017

This is up for discussion, we could do it partially

@yurishkuro
Copy link
Member

what is the motivation for these updates? Some I don't have problems with, like mockito or brave/zipkin, as they are implementation details. Others are a bit more problematic, e.g. tchannel upgrade in crossdock needs testing (if works, then ok), or Jersey upgrades that appears to be actually breaking (since you had to change code).

@alshopov
Copy link
Contributor Author

Bugfixes and performance in the underlying libs is the main reason. As jaeger is used throughout Uber's services depending on old version causes problems.
I have removed the jersey update.
The tchannel 0.7.7 is far better than 0.5 as many memory leaks have been eliminated both in it and underlying netty.
What testing do you expect for crossdock? instantpay uses latest tchannel-java and xtchannel-java with much less problems than previous versions.

@yurishkuro
Copy link
Member

As jaeger is used throughout Uber's services depending on old version causes problems.

that's the point I don't understand - unless there are API differences, the versions that Jaeger lib is using should be non-binding to the actual applications; if they want to use newer versions the build system should resolve accordingly.

@alshopov
Copy link
Contributor Author

I get your point as a maintainer - you would rather ensure long term stability and minimize changes.
However you have to weigh this against other goals.
I do point releases updates because they usually contain bugfixes, especially security ones. I've had instances when due to an outdated library we have relied on a bug as a feature. These things get ironed out with the updates.
Larger than point releases updates are to minimize the usage of old, deprecated APIs. Unless you do these regularly, they keep piling up and it becomes increasingly more difficult to finally fix them.
While what you say is true - a project may declare updated dependencies, it also may not and that also causes maintenance burden. Additionally it becomes harder to convince a project to move to a new version if the library it depends on has not been verified to work with that version.
But yeah - these decisions rest finally on the maintainers.
I may reduce the number of updates or you can deny the pull request.

@yurishkuro
Copy link
Member

Given the current list of upgrades I don't have objections to them. But it looks like they are already introducing incompatibility as the build is broken with NoSuchMethod

@codecov
Copy link

codecov bot commented Nov 17, 2017

Codecov Report

Merging #286 into master will decrease coverage by 0.14%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #286      +/-   ##
============================================
- Coverage     82.62%   82.48%   -0.15%     
+ Complexity      552      550       -2     
============================================
  Files            91       91              
  Lines          2072     2072              
  Branches        237      237              
============================================
- Hits           1712     1709       -3     
- Misses          264      265       +1     
- Partials         96       98       +2
Impacted Files Coverage Δ Complexity Δ
...jaeger/reporters/protocols/ThriftUdpTransport.java 77.96% <0%> (-5.09%) 14% <0%> (-2%)

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 916de0c...d1b8e55. Read the comment docs.

@alshopov
Copy link
Contributor Author

@yurishkuro - is the patch fine now?

Bugfixes for sl4j, gson, opentracing-tracerresolver,
jackson, zipkin, mockito, bravehttp, httpcomponents, okhttp

Update dropwizard staying on the same release series

Update tchannel-core to 0.7.7 - many performance and memory
fixes

Signed-off-by: Alexander Shopov <ashopov@uber.com>
@yurishkuro
Copy link
Member

looks like we have flaky test coverage, this didn't execute

if (!this.isOpen()) {
 throw new TTransportException(TTransportException.NOT_OPEN);

@yurishkuro yurishkuro merged commit 7acf315 into jaegertracing:master Nov 20, 2017
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