[incubator-zipkin] branch collector-normalization updated: Adds some clarifying notes about the callback swap

[ad...@apache.org]

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

adriancole pushed a commit to branch collector-normalization
in repository https://gitbox.apache.org/repos/asf/incubator-zipkin.git


The following commit(s) were added to refs/heads/collector-normalization by this push:
     new 9ce4a02  Adds some clarifying notes about the callback swap
9ce4a02 is described below

commit 9ce4a02203a856628106e3191139f3b5620454bf
Author: Adrian Cole <ac...@pivotal.io>
AuthorDate: Wed May 1 12:30:07 2019 +0800

    Adds some clarifying notes about the callback swap
---
 .../src/main/java/zipkin2/collector/Collector.java | 25 ++++++++++++++--------
 .../test/java/zipkin2/collector/CollectorTest.java | 20 ++++++++---------
 2 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/zipkin-collector/core/src/main/java/zipkin2/collector/Collector.java b/zipkin-collector/core/src/main/java/zipkin2/collector/Collector.java
index 9ddf348..dd54d39 100644
--- a/zipkin-collector/core/src/main/java/zipkin2/collector/Collector.java
+++ b/zipkin-collector/core/src/main/java/zipkin2/collector/Collector.java
@@ -112,17 +112,24 @@ public class Collector { // not final for mock
     }
     metrics.incrementSpans(spans.size());
 
-    List<Span> sampled = sample(spans);
-    if (sampled.isEmpty()) {
+    List<Span> sampledSpans = sample(spans);
+    if (sampledSpans.isEmpty()) {
       callback.onSuccess(null);
       return;
     }
 
+    // In order to ensure callers are not blocked, we swap callbacks when we get to the storage
+    // phase of this process. Here, we create a callback whose sole purpose is classifying later
+    // errors on this bundle of spans in the same log category. This allows people to only turn on
+    // debug logging in one place.
+    Callback<Void> logOnErrorCallback = storeSpansCallback(sampledSpans);
     try {
-      // adding a separate callback intentionally decouples collection from storage
-      record(sampled, acceptSpansCallback(sampled));
-      callback.onSuccess(null);
+      store(sampledSpans, logOnErrorCallback);
+      callback.onSuccess(null); // release the callback given to the collector
     } catch (RuntimeException | Error e) {
+      // While unexpected, invoking the storage command could raise an error synchronously. When
+      // that's the case, we wouldn't have invoked callback.onSuccess, so we need to handle the
+      // error here.
       handleStorageError(spans, e, callback);
     }
   }
@@ -170,8 +177,8 @@ public class Collector { // not final for mock
     return out;
   }
 
-  void record(List<Span> sampled, Callback<Void> callback) {
-    storage.spanConsumer().accept(sampled).enqueue(callback);
+  void store(List<Span> sampledSpans, Callback<Void> callback) {
+    storage.spanConsumer().accept(sampledSpans).enqueue(callback);
   }
 
   String idString(Span span) {
@@ -191,7 +198,7 @@ public class Collector { // not final for mock
     return sampled;
   }
 
-  Callback<Void> acceptSpansCallback(final List<Span> spans) {
+  Callback<Void> storeSpansCallback(final List<Span> spans) {
     return new Callback<Void>() {
       @Override public void onSuccess(Void value) {
       }
@@ -201,7 +208,7 @@ public class Collector { // not final for mock
       }
 
       @Override public String toString() {
-        return appendSpanIds(spans, new StringBuilder("AcceptSpans(")) + ")";
+        return appendSpanIds(spans, new StringBuilder("StoreSpans(")) + ")";
       }
     };
   }
diff --git a/zipkin-collector/core/src/test/java/zipkin2/collector/CollectorTest.java b/zipkin-collector/core/src/test/java/zipkin2/collector/CollectorTest.java
index d0ee433..aabcbcb 100644
--- a/zipkin-collector/core/src/test/java/zipkin2/collector/CollectorTest.java
+++ b/zipkin-collector/core/src/test/java/zipkin2/collector/CollectorTest.java
@@ -151,23 +151,23 @@ public class CollectorTest {
   }
 
   @Test
-  public void acceptSpansCallback_toStringIncludesSpanIds() {
+  public void storeSpansCallback_toStringIncludesSpanIds() {
     Span span2 = CLIENT_SPAN.toBuilder().id("3").build();
     when(collector.idString(span2)).thenReturn("3");
 
-    assertThat(collector.acceptSpansCallback(asList(CLIENT_SPAN, span2)))
-      .hasToString("AcceptSpans([1, 3])");
+    assertThat(collector.storeSpansCallback(asList(CLIENT_SPAN, span2)))
+      .hasToString("StoreSpans([1, 3])");
   }
 
   @Test
-  public void acceptSpansCallback_toStringIncludesSpanIds_noMoreThan3() {
-    assertThat(unprefixIdString(collector.acceptSpansCallback(TRACE).toString()))
-      .hasToString("AcceptSpans([1, 1, 2, ...])");
+  public void storeSpansCallback_toStringIncludesSpanIds_noMoreThan3() {
+    assertThat(unprefixIdString(collector.storeSpansCallback(TRACE).toString()))
+      .hasToString("StoreSpans([1, 1, 2, ...])");
   }
 
   @Test
-  public void acceptSpansCallback_onErrorWithNullMessage() {
-    Callback<Void> callback = collector.acceptSpansCallback(TRACE);
+  public void storeSpansCallback_onErrorWithNullMessage() {
+    Callback<Void> callback = collector.storeSpansCallback(TRACE);
     callback.onError(new RuntimeException());
 
     assertThat(messages)
@@ -176,8 +176,8 @@ public class CollectorTest {
   }
 
   @Test
-  public void acceptSpansCallback_onErrorWithMessage() {
-    Callback<Void> callback = collector.acceptSpansCallback(TRACE);
+  public void storeSpansCallback_onErrorWithMessage() {
+    Callback<Void> callback = collector.storeSpansCallback(TRACE);
     callback.onError(new IllegalArgumentException("no beer"));
 
     assertThat(messages)