TINKERPOP-1913 Refactored how response status attributes are set Didn't seem necessary to set them directly into a member variable on ResultQueue when they could be passed more safely by just including them with the completion state update
Project: http://git-wip-us.apache.org/repos/asf/tinkerpop/repo Commit: http://git-wip-us.apache.org/repos/asf/tinkerpop/commit/7fca7af7 Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/7fca7af7 Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/7fca7af7 Branch: refs/heads/TINKERPOP-1913 Commit: 7fca7af74e5bd2695ba6035cf341543f9750fc4d Parents: 956b177 Author: Stephen Mallette <[email protected]> Authored: Thu Aug 16 11:38:09 2018 -0400 Committer: Stephen Mallette <[email protected]> Committed: Tue Sep 18 12:58:42 2018 -0400 ---------------------------------------------------------------------- .../tinkerpop/gremlin/driver/Handler.java | 11 +------ .../tinkerpop/gremlin/driver/ResultQueue.java | 11 +++++-- .../tinkerpop/gremlin/driver/ResultSet.java | 2 +- .../gremlin/driver/AbstractResultQueueTest.java | 3 +- .../gremlin/driver/ResultQueueTest.java | 30 +++++++++++++++----- .../tinkerpop/gremlin/driver/ResultSetTest.java | 28 +++++++----------- 6 files changed, 46 insertions(+), 39 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/7fca7af7/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Handler.java ---------------------------------------------------------------------- diff --git a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Handler.java b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Handler.java index d79bed5..a77ec84 100644 --- a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Handler.java +++ b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Handler.java @@ -260,18 +260,9 @@ final class Handler { } } - // the last message in a OK stream could have meta-data that is useful to the result. note that error - // handling of the status attributes is handled separately above - if (statusCode == ResponseStatusCode.SUCCESS || statusCode == ResponseStatusCode.NO_CONTENT) { - // in 3.4.0 this should get refactored. i think the that the markComplete() could just take the - // status attributes as its argument - need to investigate that further - queue.statusAttributes = response.getStatus().getAttributes(); - } - // as this is a non-PARTIAL_CONTENT code - the stream is done. if (statusCode != ResponseStatusCode.PARTIAL_CONTENT) { - queue.statusAttributes = response.getStatus().getAttributes(); - pending.remove(response.getRequestId()).markComplete(); + pending.remove(response.getRequestId()).markComplete(response.getStatus().getAttributes()); } } finally { // in the event of an exception above the exception is tossed and handled by whatever channelpipeline http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/7fca7af7/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ResultQueue.java ---------------------------------------------------------------------- diff --git a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ResultQueue.java b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ResultQueue.java index 7340763..59b4617 100644 --- a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ResultQueue.java +++ b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ResultQueue.java @@ -26,6 +26,7 @@ import org.javatuples.Pair; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -56,7 +57,7 @@ final class ResultQueue { private final Queue<Pair<CompletableFuture<List<Result>>,Integer>> waiting = new ConcurrentLinkedQueue<>(); - Map<String,Object> statusAttributes = null; + private Map<String,Object> statusAttributes = null; public ResultQueue(final LinkedBlockingQueue<Result> resultLinkedBlockingQueue, final CompletableFuture<Void> readComplete) { this.resultLinkedBlockingQueue = resultLinkedBlockingQueue; @@ -165,12 +166,14 @@ final class ResultQueue { resultLinkedBlockingQueue.drainTo(collection); } - void markComplete() { + void markComplete(final Map<String,Object> statusAttributes) { // if there was some aggregation performed in the queue then the full object is hanging out waiting to be // added to the ResultSet if (aggregatedResult != null) add(new Result(aggregatedResult)); + this.statusAttributes = null == statusAttributes ? Collections.emptyMap() : statusAttributes; + this.readComplete.complete(null); this.drainAllWaiting(); @@ -182,6 +185,10 @@ final class ResultQueue { this.drainAllWaiting(); } + Map<String,Object> getStatusAttributes() { + return statusAttributes; + } + /** * Completes the next waiting future if there is one. */ http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/7fca7af7/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ResultSet.java ---------------------------------------------------------------------- diff --git a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ResultSet.java b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ResultSet.java index f608f06..85c74f3 100644 --- a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ResultSet.java +++ b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ResultSet.java @@ -81,7 +81,7 @@ public final class ResultSet implements Iterable<Result> { */ public CompletableFuture<Map<String,Object>> statusAttributes() { final CompletableFuture<Map<String,Object>> attrs = new CompletableFuture<>(); - readCompleted.thenRun(() -> attrs.complete(null == resultQueue.statusAttributes ? Collections.emptyMap() : resultQueue.statusAttributes)); + readCompleted.thenRun(() -> attrs.complete(null == resultQueue.getStatusAttributes() ? Collections.emptyMap() : resultQueue.getStatusAttributes())); return attrs; } http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/7fca7af7/gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/AbstractResultQueueTest.java ---------------------------------------------------------------------- diff --git a/gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/AbstractResultQueueTest.java b/gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/AbstractResultQueueTest.java index 3c84ca6..302fda9 100644 --- a/gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/AbstractResultQueueTest.java +++ b/gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/AbstractResultQueueTest.java @@ -20,6 +20,7 @@ package org.apache.tinkerpop.gremlin.driver; import org.junit.Before; +import java.util.Collections; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; @@ -84,7 +85,7 @@ public abstract class AbstractResultQueueTest { } } - if (markDone) resultQueue.markComplete(); + if (markDone) resultQueue.markComplete(Collections.emptyMap()); }, "ResultQueueTest-job-submitter"); http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/7fca7af7/gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/ResultQueueTest.java ---------------------------------------------------------------------- diff --git a/gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/ResultQueueTest.java b/gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/ResultQueueTest.java index 43442be..436d659 100644 --- a/gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/ResultQueueTest.java +++ b/gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/ResultQueueTest.java @@ -45,6 +45,10 @@ import static org.junit.Assert.fail; */ public class ResultQueueTest extends AbstractResultQueueTest { + private static final Map<String,Object> ATTRIBUTES = new HashMap<String,Object>() {{ + put("this", "that"); + }}; + @Test public void shouldGetSizeUntilError() throws Exception { final Thread t = addToQueue(100, 10, true, false, 1); @@ -135,7 +139,7 @@ public class ResultQueueTest extends AbstractResultQueueTest { resultQueue.add(new Result("test3")); assertThat(future.isDone(), is(false)); - resultQueue.markComplete(); + resultQueue.markComplete(ATTRIBUTES); assertThat(future.isDone(), is(true)); final List<Result> results = future.get(); @@ -145,6 +149,7 @@ public class ResultQueueTest extends AbstractResultQueueTest { assertEquals(3, results.size()); assertThat(resultQueue.isEmpty(), is(true)); + assertEquals("that", resultQueue.getStatusAttributes().get("this")); } @Test @@ -242,7 +247,7 @@ public class ResultQueueTest extends AbstractResultQueueTest { resultQueue.add(new Result("test2")); resultQueue.add(new Result("test3")); - resultQueue.markComplete(); + resultQueue.markComplete(ATTRIBUTES); // you might want 30 but there are only three final CompletableFuture<List<Result>> future = resultQueue.await(30); @@ -255,6 +260,7 @@ public class ResultQueueTest extends AbstractResultQueueTest { assertEquals(3, results.size()); assertThat(resultQueue.isEmpty(), is(true)); + assertEquals("that", resultQueue.getStatusAttributes().get("this")); } @Test @@ -307,12 +313,14 @@ public class ResultQueueTest extends AbstractResultQueueTest { resultQueue.addSideEffect(Tokens.VAL_AGGREGATE_TO_BULKSET, new DefaultRemoteTraverser<>("belinda", 6)); assertThat(o.isDone(), is(false)); - resultQueue.markComplete(); + resultQueue.markComplete(ATTRIBUTES); assertThat(o.isDone(), is(true)); final BulkSet<String> bulkSet = o.get().get(0).get(BulkSet.class); assertEquals(4, bulkSet.get("brian")); assertEquals(6, bulkSet.get("belinda")); + + assertEquals("that", resultQueue.getStatusAttributes().get("this")); } @Test @@ -329,13 +337,15 @@ public class ResultQueueTest extends AbstractResultQueueTest { resultQueue.addSideEffect(Tokens.VAL_AGGREGATE_TO_LIST, "dave"); assertThat(o.isDone(), is(false)); - resultQueue.markComplete(); + resultQueue.markComplete(ATTRIBUTES); assertThat(o.isDone(), is(true)); final List<String> list = o.get().get(0).get(ArrayList.class); assertEquals("stephen", list.get(0)); assertEquals("daniel", list.get(1)); assertEquals("dave", list.get(2)); + + assertEquals("that", resultQueue.getStatusAttributes().get("this")); } @Test @@ -352,13 +362,15 @@ public class ResultQueueTest extends AbstractResultQueueTest { resultQueue.addSideEffect(Tokens.VAL_AGGREGATE_TO_SET, "dave"); assertThat(o.isDone(), is(false)); - resultQueue.markComplete(); + resultQueue.markComplete(ATTRIBUTES); assertThat(o.isDone(), is(true)); final Set<String> set = o.get().get(0).get(HashSet.class); assertThat(set.contains("stephen"), is(true)); assertThat(set.contains("daniel"), is(true)); assertThat(set.contains("dave"), is(true)); + + assertEquals("that", resultQueue.getStatusAttributes().get("this")); } @Test @@ -376,13 +388,15 @@ public class ResultQueueTest extends AbstractResultQueueTest { assertThat(o.isDone(), is(false)); }); - resultQueue.markComplete(); + resultQueue.markComplete(ATTRIBUTES); assertThat(o.isDone(), is(true)); final Map<String, String> list = o.get().get(0).get(HashMap.class); assertEquals("stephen", list.get("s")); assertEquals("daniel", list.get("d")); assertEquals("marko", list.get("m")); + + assertEquals("that", resultQueue.getStatusAttributes().get("this")); } @@ -398,12 +412,14 @@ public class ResultQueueTest extends AbstractResultQueueTest { resultQueue.addSideEffect(Tokens.VAL_AGGREGATE_TO_NONE, m); - resultQueue.markComplete(); + resultQueue.markComplete(ATTRIBUTES); assertThat(o.isDone(), is(true)); final Map<String, String> list = o.get().get(0).get(HashMap.class); assertEquals("stephen", list.get("s")); assertEquals("daniel", list.get("d")); assertEquals("marko", list.get("m")); + + assertEquals("that", resultQueue.getStatusAttributes().get("this")); } } http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/7fca7af7/gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/ResultSetTest.java ---------------------------------------------------------------------- diff --git a/gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/ResultSetTest.java b/gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/ResultSetTest.java index 3163ffe..44f13e4 100644 --- a/gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/ResultSetTest.java +++ b/gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/ResultSetTest.java @@ -43,6 +43,10 @@ import static org.junit.Assert.fail; */ public class ResultSetTest extends AbstractResultQueueTest { + private static final Map<String,Object> ATTRIBUTES = new HashMap<String,Object>() {{ + put("this", "that"); + }}; + private ResultSet resultSet; @Before @@ -51,22 +55,6 @@ public class ResultSetTest extends AbstractResultQueueTest { } @Test - public void shouldReturnResponseAttributes() throws Exception { - resultQueue.statusAttributes = new HashMap<String,Object>() {{ - put("test",123); - put("junk","here"); - }}; - - final CompletableFuture<Map<String,Object>> attrs = resultSet.statusAttributes(); - readCompleted.complete(null); - - final Map<String,Object> m = attrs.get(); - assertEquals(123, m.get("test")); - assertEquals("here", m.get("junk")); - assertEquals(2, m.size()); - } - - @Test public void shouldReturnEmptyMapForNoResponseAttributes() throws Exception { final CompletableFuture<Map<String,Object>> attrs = resultSet.statusAttributes(); readCompleted.complete(null); @@ -132,7 +120,7 @@ public class ResultSetTest extends AbstractResultQueueTest { resultQueue.add(new Result("test3")); assertThat(future.isDone(), is(false)); - resultQueue.markComplete(); + resultQueue.markComplete(ATTRIBUTES); assertThat(future.isDone(), is(true)); final List<Result> results = future.get(); @@ -143,6 +131,8 @@ public class ResultSetTest extends AbstractResultQueueTest { assertThat(resultSet.allItemsAvailable(), is(true)); assertEquals(0, resultSet.getAvailableItemCount()); + + assertEquals("that", resultQueue.getStatusAttributes().get("this")); } @Test @@ -153,7 +143,7 @@ public class ResultSetTest extends AbstractResultQueueTest { resultQueue.add(new Result("test3")); assertThat(future.isDone(), is(false)); - resultQueue.markComplete(); + resultQueue.markComplete(ATTRIBUTES); final List<Result> results = future.get(); assertEquals("test1", results.get(0).getString()); @@ -164,6 +154,8 @@ public class ResultSetTest extends AbstractResultQueueTest { assertThat(future.isDone(), is(true)); assertThat(resultSet.allItemsAvailable(), is(true)); assertEquals(0, resultSet.getAvailableItemCount()); + + assertEquals("that", resultQueue.getStatusAttributes().get("this")); } @Test
