Java 6 compatibility by felixbarny · Pull Request #132 · jaegertracing/jaeger-client-java

felixbarny · 2017-03-11T11:52:32Z

Checked with IntelliJ:
Run Inspection by Name >
Usages of API which isn't available at the configured language level

jaeger-crossdock still requires Java 7 because it calls
InetAddress#getLoopbackAddress()

(closes #129)

Checked with IntelliJ: Run Inspection by Name > Usages of API which isn't available at the configured language level jaeger-crossdock still requires Java 7 because it calls InetAddress#getLoopbackAddress() (closes jaegertracing#129)

CLAassistant · 2017-03-11T11:53:33Z

All committers have signed the CLA.

codecov-io · 2017-03-11T11:58:33Z

Codecov Report

Merging #132 into master will increase coverage by 0.02%.
The diff coverage is 81.91%.

@@             Coverage Diff              @@
##             master     #132      +/-   ##
============================================
+ Coverage     81.41%   81.43%   +0.02%     
- Complexity      523      529       +6     
============================================
  Files            90       91       +1     
  Lines          1964     1988      +24     
  Branches        196      194       -2     
============================================
+ Hits           1599     1619      +20     
- Misses          271      277       +6     
+ Partials         94       92       -2

Impacted Files	Coverage Δ	Complexity Δ
...uber/jaeger/reporters/protocols/TUDPTransport.java	`83.05% <ø> (ø)`	`16 <0> (ø)`	⬇️
...java/com/uber/jaeger/dropwizard/JaegerFeature.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...reporters/protocols/JaegerThriftSpanConverter.java	`91.07% <100%> (ø)`	`16 <0> (ø)`	⬇️
...in/java/com/uber/jaeger/samplers/ConstSampler.java	`64.28% <100%> (ø)`	`3 <0> (ø)`	⬇️
.../com/uber/jaeger/samplers/RateLimitingSampler.java	`68.75% <100%> (ø)`	`5 <0> (ø)`	⬇️
...com/uber/jaeger/context/TracedExecutorService.java	`88% <100%> (ø)`	`13 <1> (ø)`	⬇️
...c/main/java/com/uber/jaeger/senders/UDPSender.java	`67.34% <100%> (ø)`	`10 <0> (ø)`	⬇️
...com/uber/jaeger/metrics/InMemoryStatsReporter.java	`69.23% <100%> (ø)`	`5 <1> (ø)`	⬇️
...n/java/com/uber/jaeger/senders/InMemorySender.java	`94.11% <100%> (ø)`	`7 <3> (ø)`	⬇️
.../com/uber/jaeger/samplers/HTTPSamplingManager.java	`96.66% <100%> (+3.56%)`	`6 <0> (ø)`	⬇️
... and 15 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 1b13636...8f42a05. Read the comment docs.

yurishkuro · 2017-03-11T18:07:20Z

lgtm (with we could do reformatting separately).

it's ok to keep crossdock at 1.7, since it's just an integration test.

@vprithvi could you please review as well?

vprithvi · 2017-03-11T18:23:04Z

Sure, I'll take a look later in the day

vprithvi

LGTM, but I'm not convinced of the value of moving the entire project to Java 6. Wouldn't moving jaeger-core and jaeger-context satisfy your use case?

Run Inspection by Name >
Usages of API which isn't available at the configured language level

This should be automated so that a future commit doesn't introduce such usages. I've heard good things about the AnimalSniffer plugin for this purpose.

vprithvi · 2017-03-12T14:08:17Z

jaeger-core/src/main/java/com/uber/jaeger/utils/Java6CompatibleThreadLocalRandom.java

+import java.util.Random;
+import java.util.concurrent.ThreadLocalRandom;
+
+public final class Java6CompatibleThreadLocalRandom {


Could you add tests for this?

Hard to test this in a meaningful way. Especially since Gradle 3 does not run with Java 6.

felixbarny · 2017-03-19T09:08:26Z

LGTM, but I'm not convinced of the value of moving the entire project to Java 6. Wouldn't moving jaeger-core and jaeger-context satisfy your use case?

If jaeger-zipkin also was 1.6 it would satisfy my use case. But I think it's a bit confusing and inconsistent for developers and users to have a mix of java versions in published artifacts. Also, in the other artifacts I almost had to change nothing. All in all it would make jaeger accessible to a larger user base without much effort.

This should be automated so that a future commit doesn't introduce such usages. I've heard good things about the AnimalSniffer plugin for this purpose.

I tried this plugin but didn't get it to work at first. Was a layer 8 problem ;)

felixbarny · 2017-03-19T09:49:22Z

#131 also is important for java 6 compatibility as the currently used ot-api version (0.15.0) is not java 6 compatible while the recent one is.

yurishkuro · 2017-03-19T15:29:41Z

jaeger-thrift/build.gradle

+ * Note that you need to install thrift 0.9.2 on your system for this task to work.
+ *
+ * If you are using macOS, you can install this specific version via homebrew like so:
+ * `brew install https://raw.githubusercontent.com/Homebrew/homebrew-core/56d8c1eba1e5ac30290dd0c486f4bba37f821e42/Formula/thrift.rb`


Nice touch!

That took me quite some time to figure out 😅

yurishkuro · 2017-03-19T15:33:11Z

jaeger-zipkin/src/main/java/com/uber/jaeger/senders/zipkin/ZipkinSender.java

    int messageSizeOfNextSpan = delegate.messageSizeInBytes(Collections.singletonList(next));
    // don't enqueue something larger than we can drain
-    if (Integer.compare(messageSizeOfNextSpan, delegate.messageMaxBytes()) > 0) {
+    if (Integer.valueOf(messageSizeOfNextSpan).compareTo(delegate.messageMaxBytes()) > 0) {


how is this different from messageSizeOfNextSpan > delegate.messageMaxBytes() ?

you're right. done.

The downside is that it's probably less JIT friendly as I've removed the final keyword on the threadLocalRandomPresent flag

felixbarny · 2017-03-21T08:29:48Z

Conflicts are resolved now. I also tried to improve the Java6CompatibleThreadLocalRandomTest.

Java 6 compatibility

10f378a

Checked with IntelliJ: Run Inspection by Name > Usages of API which isn't available at the configured language level jaeger-crossdock still requires Java 7 because it calls InetAddress#getLoopbackAddress() (closes jaegertracing#129)

vprithvi suggested changes Mar 12, 2017

View reviewed changes

Add animal sniffer plugin and test

d8cb0d6

yurishkuro approved these changes Mar 19, 2017

View reviewed changes

Simplify integer comparison

5638786

vprithvi approved these changes Mar 19, 2017

View reviewed changes

Felix Barnsteiner added 2 commits March 21, 2017 09:20

Test Java6CompatibleThreadLocalRandom in a more meaningful way

3967221

The downside is that it's probably less JIT friendly as I've removed the final keyword on the threadLocalRandomPresent flag

Merge remote-tracking branch 'original/master' into java6

8f42a05

vprithvi merged commit 7839d9f into jaegertracing:master Mar 21, 2017

felixbarny deleted the java6 branch March 21, 2017 14:09

isaachier mentioned this pull request Aug 8, 2018

Drop Java 6 Compatibility #511

Closed

pavolloffay mentioned this pull request May 10, 2019

Automatically generate changelog #629

Closed

Conversation

felixbarny commented Mar 11, 2017

Uh oh!

CLAassistant commented Mar 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Mar 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yurishkuro commented Mar 11, 2017

Uh oh!

vprithvi commented Mar 11, 2017

Uh oh!

vprithvi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vprithvi Mar 12, 2017

Choose a reason for hiding this comment

Uh oh!

felixbarny Mar 19, 2017

Choose a reason for hiding this comment

Uh oh!

felixbarny commented Mar 19, 2017

Uh oh!

felixbarny commented Mar 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yurishkuro Mar 19, 2017

Choose a reason for hiding this comment

Uh oh!

felixbarny Mar 19, 2017

Choose a reason for hiding this comment

Uh oh!

yurishkuro Mar 19, 2017

Choose a reason for hiding this comment

Uh oh!

felixbarny Mar 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixbarny commented Mar 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CLAassistant commented Mar 11, 2017 •

edited

Loading

codecov-io commented Mar 11, 2017 •

edited

Loading

vprithvi left a comment •

edited

Loading

felixbarny commented Mar 19, 2017 •

edited

Loading

felixbarny Mar 19, 2017 •

edited

Loading