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

Update errorprone and lombok and fix warnings and errors #270

Merged
merged 3 commits into from
Oct 18, 2017
Merged

Update errorprone and lombok and fix warnings and errors #270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
25c2aba
Update errorprone and lombok plugin
vprithvi Oct 12, 2017
9e2c98c
Update errorprone and lombok and fix warnings and errors
vprithvi Oct 12, 2017
9ad9b8a
Remove unused import
vprithvi Oct 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ plugins {
id "com.github.hierynomus.license" version "0.14.0"
id 'com.github.sherter.google-java-format' version '0.3.2'
id "com.github.johnrengelman.shadow" version "2.0.1"
id "net.ltgt.errorprone" version "0.0.10"
id "net.ltgt.errorprone" version "0.0.12"
id 'ru.vyarus.animalsniffer' version '1.3.0'
id 'io.codearte.nexus-staging' version '0.11.0'
}
Expand Down Expand Up @@ -126,7 +126,7 @@ subprojects {
}

dependencies {
compileOnly 'org.projectlombok:lombok:1.16.12'
compileOnly 'org.projectlombok:lombok:1.16.18'
compileOnly 'org.codehaus.mojo:animal-sniffer-annotations:1.15'
}

Expand Down Expand Up @@ -165,8 +165,12 @@ task printVersion {
configure(subprojects.findAll {it.name != 'jaeger-thrift'}) {
apply plugin: 'net.ltgt.errorprone'
dependencies {
errorprone 'com.google.errorprone:error_prone_core:2.0.15'
errorprone 'com.google.errorprone:error_prone_core:2.1.1'
}
tasks.withType(JavaCompile) {
// These are disabled because of a bug with errorprone. See https://github.com/google/error-prone/issues/750
Copy link
Member

Choose a reason for hiding this comment

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

not following how that issue is related to disabling checks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enabling those checks causes a runtime exception in the error prone compiler

Copy link
Member

Choose a reason for hiding this comment

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

so using @Data is a convenience, but turning off InstanceOfAndCastMatchWrongType sounds like it could mask real errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

InstanceOfAndCastMatchWrongType doesn't exist in the current version of errorprone that the project uses.
Could you please justify your statement of using @Data is a convenience. Have there been no bugs made in manually writing out the convenience functions that @Data/@Value provides? If there have, is the cost of fixing the bugs introduced by incorrect hashcode/tostring/getters less than the potential time saved by having these new error checks enabled?
Has the time taken to write boilerplate code been factored into your argument?

Copy link
Member

Choose a reason for hiding this comment

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

the methods that @Data/@Value generate can be just as easily created by the IDE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really feel that you are arguing with me for the sake for arguing.
How would you maintain consistency between the IDE generated code?

Should bumping a couple of versions have so much friction?

options.compilerArgs += [ '-Xep:NestedInstanceOfConditions:OFF', '-Xep:InstanceOfAndCastMatchWrongType:OFF' ]
}
}

def getVersionForBuild() {
Expand Down
2 changes: 1 addition & 1 deletion jaeger-core/src/main/java/com/uber/jaeger/Tracer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ private SpanContext createNewContext(String debugId) {

byte flags = 0;
if (debugId != null) {
flags |= SpanContext.flagSampled | SpanContext.flagDebug;
flags = (byte) (flags | SpanContext.flagSampled | SpanContext.flagDebug);
Copy link
Member

Choose a reason for hiding this comment

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

all operands are already of type byte, why do we need to re-cast?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because of assignment conversion. See http://docs.oracle.com/javase/specs/jls/se7/html/jls-5.html#jls-5.2 and http://errorprone.info/bugpattern/NarrowingCompoundAssignment

The result of the bitwise OR is being promoted to a int which was previously implicitly cast to a byte.

tags.put(Constants.DEBUG_ID_HEADER_KEY, debugId);
metrics.traceStartedSampled.inc(1);
} else {
Expand Down
3 changes: 3 additions & 0 deletions jaeger-core/src/test/java/com/uber/jaeger/samplers/GuaranteedThroughputSamplerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@

import com.uber.jaeger.Constants;
import java.util.Map;

import org.junit.After;
import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand All @@ -30,6 +32,7 @@ public class GuaranteedThroughputSamplerTest {

private GuaranteedThroughputSampler undertest;

@After
public void tearDown() {
undertest.close();
}
Expand Down
5 changes: 3 additions & 2 deletions jaeger-crossdock/src/main/java/com/uber/jaeger/crossdock/JerseyServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.concurrent.ExecutionException;
import javax.ws.rs.client.Client;
import javax.ws.rs.client.ClientBuilder;
import org.apache.log4j.BasicConfigurator;
Expand Down Expand Up @@ -117,8 +118,8 @@ protected void configure() {
.register(JacksonFeature.class);
}

public void shutdown() {
server.shutdown();
public void shutdown() throws ExecutionException, InterruptedException {
server.shutdown().get();
}

public Tracer getTracer() {
Expand Down
6 changes: 5 additions & 1 deletion ...k/src/main/java/com/uber/jaeger/crossdock/resources/behavior/tchannel/TChannelServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.uber.jaeger.crossdock.resources.behavior.TraceBehavior;
import com.uber.tchannel.api.TChannel;
import com.uber.tchannel.tracing.TracingContext;
import io.netty.channel.ChannelFuture;
import io.opentracing.Span;
import io.opentracing.Tracer;
import java.util.EmptyStackException;
Expand Down Expand Up @@ -45,7 +46,10 @@ public TChannel getChannel() {

public void start() throws InterruptedException {
// listen for incoming connections
server.listen().channel().closeFuture();
ChannelFuture serverFuture = server.listen().awaitUninterruptibly();
if (!serverFuture.isSuccess()) {
throw new RuntimeException("Server future unsuccessful");
}
}

public void shutdown() {
Expand Down
1 change: 1 addition & 0 deletions jaeger-dropwizard/src/test/java/com/uber/jaeger/dropwizard/JerseyServerFilterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ protected Application configure() {
return resourceConfig;
}

@Override
@After
public void tearDown() throws Exception {
super.tearDown();
Expand Down