[incubator-zipkin-reporter-java] branch fix-libthrift created (now edbfbce)

[ad...@apache.org]

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

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


      at edbfbce  Fixes socket reset problem in libthrift sender

This branch includes the following new commits:

     new edbfbce  Fixes socket reset problem in libthrift sender

The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.

[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);