[incubator-zipkin-reporter-java] 01/01: Fixes socket reset problem in libthrift sender

[ad...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

adriancole pushed a commit to branch fix-libthrift
in repository https://gitbox.apache.org/repos/asf/incubator-zipkin-reporter-java.git

commit edbfbceab098e0abc4f14fcb2141bba6973f43a2
Author: Adrian Cole <ac...@pivotal.io>
AuthorDate: Wed May 8 21:00:47 2019 +0800

    Fixes socket reset problem in libthrift sender
    
    Before, we didn't fully read the scribe response, resulting in socket
    resets.
---
 .../test/java/zipkin2/reporter/TestObjects.java    | 26 ++++++++++++++++++++++
 .../reporter/libthrift/InternalScribeCodec.java    |  4 +++-
 .../reporter/libthrift/LibthriftSenderTest.java    | 13 +++++++++++
 3 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/core/src/test/java/zipkin2/reporter/TestObjects.java b/core/src/test/java/zipkin2/reporter/TestObjects.java
index 4a5d921..0cfe894 100644
--- a/core/src/test/java/zipkin2/reporter/TestObjects.java
+++ b/core/src/test/java/zipkin2/reporter/TestObjects.java
@@ -18,11 +18,13 @@ package zipkin2.reporter;
 
 import java.nio.charset.Charset;
 import java.util.Calendar;
+import java.util.Random;
 import java.util.TimeZone;
 import java.util.concurrent.TimeUnit;
 import zipkin2.Endpoint;
 import zipkin2.Span;
 
+// TODO: replace with zipkin-tests jar!
 public final class TestObjects {
   public static final Charset UTF_8 = Charset.forName("UTF-8");
   /** Notably, the cassandra implementation has day granularity */
@@ -67,4 +69,28 @@ public final class TestObjects {
     .putTag("http.path", "/api")
     .putTag("clnt/finagle.version", "6.45.0")
     .build();
+
+  static final Span.Builder spanBuilder = spanBuilder();
+
+  /** Reuse a builder as it is significantly slows tests to create 100000 of these! */
+  static Span.Builder spanBuilder() {
+    return Span.newBuilder()
+        .name("get /foo")
+        .timestamp(System.currentTimeMillis() * 1000)
+        .duration(1000)
+        .kind(Span.Kind.SERVER)
+        .localEndpoint(BACKEND)
+        .putTag("http.method", "GET");
+  }
+
+  /**
+   * Zipkin trace ids are random 64bit numbers. This creates a relatively large input to avoid
+   * flaking out due to PRNG nuance.
+   */
+  public static final Span[] LOTS_OF_SPANS =
+      new Random().longs(100_000).mapToObj(TestObjects::span).toArray(Span[]::new);
+
+  public static Span span(long traceId) {
+    return spanBuilder.traceId(Long.toHexString(traceId)).id(traceId).build();
+  }
 }
diff --git a/libthrift/src/main/java/zipkin2/reporter/libthrift/InternalScribeCodec.java b/libthrift/src/main/java/zipkin2/reporter/libthrift/InternalScribeCodec.java
index eb6199d..6e643e4 100644
--- a/libthrift/src/main/java/zipkin2/reporter/libthrift/InternalScribeCodec.java
+++ b/libthrift/src/main/java/zipkin2/reporter/libthrift/InternalScribeCodec.java
@@ -78,15 +78,17 @@ public final class InternalScribeCodec { // public for zipkin-finagle
   }
 
   static boolean parseResponse(TBinaryProtocol iprot) throws TException {
+    Boolean result = null;
     iprot.readStructBegin();
     TField schemeField;
     while ((schemeField = iprot.readFieldBegin()).type != TType.STOP) {
       if (schemeField.id == 0 /* SUCCESS */ && schemeField.type == TType.I32) {
-        return iprot.readI32() == 0;
+        result = iprot.readI32() == 0;
       } else {
         TProtocolUtil.skip(iprot, schemeField.type);
       }
     }
+    if (result != null) return result;
     throw new TApplicationException(MISSING_RESULT, "Log failed: unknown result");
   }
 
diff --git a/libthrift/src/test/java/zipkin2/reporter/libthrift/LibthriftSenderTest.java b/libthrift/src/test/java/zipkin2/reporter/libthrift/LibthriftSenderTest.java
index c84490b..a2f3b6f 100644
--- a/libthrift/src/test/java/zipkin2/reporter/libthrift/LibthriftSenderTest.java
+++ b/libthrift/src/test/java/zipkin2/reporter/libthrift/LibthriftSenderTest.java
@@ -17,6 +17,7 @@
 package zipkin2.reporter.libthrift;
 
 import java.io.IOException;
+import java.util.Arrays;
 import java.util.List;
 import java.util.stream.Stream;
 import org.junit.After;
@@ -35,6 +36,7 @@ import static java.util.stream.Collectors.toList;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.failBecauseExceptionWasNotThrown;
 import static zipkin2.reporter.TestObjects.CLIENT_SPAN;
+import static zipkin2.reporter.TestObjects.LOTS_OF_SPANS;
 
 public class LibthriftSenderTest {
   @Rule public ExpectedException thrown = ExpectedException.none();
@@ -68,6 +70,17 @@ public class LibthriftSenderTest {
     assertThat(storage.spanStore().getTraces()).containsExactly(asList(CLIENT_SPAN));
   }
 
+  /** This will help verify sequence ID and response parsing logic works */
+  @Test
+  public void sendsSpans_multipleTimes() throws Exception {
+    for (int i = 0; i < 5; i++) { // Have client send 5 messages
+      send(Arrays.copyOfRange(LOTS_OF_SPANS, i, (i * 10) + 10));
+    }
+
+    assertThat(storage.getTraces()).flatExtracting(l -> l)
+        .contains(Arrays.copyOfRange(LOTS_OF_SPANS, 0, 50));
+  }
+
   @Test
   public void sendsSpansExpectedMetrics() throws Exception {
     send(CLIENT_SPAN, CLIENT_SPAN);