You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by davebshow <gi...@git.apache.org> on 2016/10/05 22:31:54 UTC
[GitHub] tinkerpop pull request #451: Tinkerpop 1458 Gremlin Server doesn't return co...
GitHub user davebshow opened a pull request:
https://github.com/apache/tinkerpop/pull/451
Tinkerpop 1458 Gremlin Server doesn't return confirmation upon Traversal OpProcessor "close" op
https://issues.apache.org/jira/browse/TINKERPOP-1458
This PR updates the Gremlin Server protocol to send a no content confirmation when a client submits a `close` Op with the `traversal` OpProcessor.
It also adds close methods to the Java driver `DriverRemoteTraversalSideEffects` class and the gremlin-python `RemoteTraversalSideEffects` class. Furthermore, `RemoteTraversalSideEffects` now caches side effects locally in order to maintain consistent behavior between the two implementations.
This functionality is tested using integration tests in both the gremlin-server and gremlin-python module, as well as in the gremlin-driver unit tests with mocked responses.
Thanks to @spmallette for helping me through the Java stuff.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/tinkerpop TINKERPOP-1458
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/451.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #451
----
commit 8ab7e50824ee72393c23d416ee2ea8348145b4bf
Author: davebshow <da...@apache.org>
Date: 2016-09-22T18:51:21Z
TraversalOpProcessor returns a success message upon receiveing a close command
commit 733bd7e29d7da1a4b972d208cb3ef6f32861e67e
Author: davebshow <da...@apache.org>
Date: 2016-09-26T17:25:17Z
added close method to gremlin python sideeffects
commit ef553d5f72fe10bfc10342981452a93d75deb203
Author: davebshow <da...@gmail.com>
Date: 2016-09-29T15:55:09Z
added close method to DriverRemoteTraversalSideEffects, implement AutoCloseable on TraversalSideEffects, add test for close method
commit e574bbf867e1ec6c7bbba59f1395648e5cd0fc5a
Author: davebshow <da...@gmail.com>
Date: 2016-09-29T16:04:03Z
got rid of wildcard set by intellij
commit 10779228ba5ed07ab43e84bef458e17fdfb9deb8
Author: davebshow <da...@gmail.com>
Date: 2016-09-29T17:04:57Z
fixed logic in DriverRemoteSideEffects, don't clear local side effect cache
commit d60def3d45d2618c681d6b2f88f1a7017b09f407
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-09-30T13:23:29Z
Add some tests for DriverRemoteTraversalSideEffects.
commit 790aa060ce828b8d76f90b06f59e770431e7b732
Author: davebshow <da...@gmail.com>
Date: 2016-10-05T19:01:59Z
added integration tests for DriverRemoteTraversalSideEffects methods
commit fd2d6eb86f9d45a709d0684a23d57c83c18f5826
Author: davebshow <da...@gmail.com>
Date: 2016-10-05T21:59:15Z
fixed side effect methods and updated tests
commit f3baae8ba2415191e700c3842f983001096168e5
Author: davebshow <da...@gmail.com>
Date: 2016-10-05T22:00:27Z
updated driver to cache side effects locally
commit ff41aafd2563d59e3fc95bb310708c5ebd61a4ce
Author: davebshow <da...@gmail.com>
Date: 2016-10-05T22:06:02Z
removed extra lines
commit 89a5f37724833d45618fdc4f7eb6fa22ddaaf783
Author: davebshow <da...@gmail.com>
Date: 2016-10-05T22:10:15Z
updated changelog
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #451: TINKERPOP-1458 Gremlin Server doesn't return co...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/451#discussion_r82211073
--- Diff: gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/DriverRemoteTraversalSideEffects.java ---
@@ -54,22 +57,30 @@ public DriverRemoteTraversalSideEffects(final Client client, final UUID serverSi
@Override
public <V> V get(final String key) throws IllegalArgumentException {
if (!sideEffects.containsKey(key)) {
- // specify the ARGS_HOST so that the LoadBalancingStrategy is subverted and the connection is forced
- // from the specified host (i.e. the host from the previous request as that host will hold the side-effects)
- final RequestMessage msg = RequestMessage.build(Tokens.OPS_GATHER)
- .addArg(Tokens.ARGS_SIDE_EFFECT, serverSideEffect)
- .addArg(Tokens.ARGS_SIDE_EFFECT_KEY, key)
- .addArg(Tokens.ARGS_HOST, host)
- .processor("traversal").create();
- try {
- final Result result = client.submitAsync(msg).get().one();
- sideEffects.put(key, null == result ? null : result.getObject());
- } catch (Exception ex) {
- final Throwable root = ExceptionUtils.getRootCause(ex);
- if (root.getMessage().equals("Could not find side-effects for " + serverSideEffect + "."))
- sideEffects.put(key, null);
- else
- throw new RuntimeException("Could not get keys", root);
+ if (!closed) {
+ // specify the ARGS_HOST so that the LoadBalancingStrategy is subverted and the connection is forced
+ // from the specified host (i.e. the host from the previous request as that host will hold the side-effects)
+ final RequestMessage msg = RequestMessage.build(Tokens.OPS_GATHER)
+ .addArg(Tokens.ARGS_SIDE_EFFECT, serverSideEffect)
+ .addArg(Tokens.ARGS_SIDE_EFFECT_KEY, key)
+ .addArg(Tokens.ARGS_HOST, host)
+ .processor("traversal").create();
+ try {
+ final Result result = client.submitAsync(msg).get().one();
+ sideEffects.put(key, null == result ? null : result.getObject());
+ if (keys.isEmpty())
--- End diff --
Not sure if you should check `isEmpty()`. Maybe better to check:
```java
if (keys == Collections.emptySet())
```
That's really when you want to new up a fresh `HashSet`, not just when its empty.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #451: TINKERPOP-1458 Gremlin Server doesn't return co...
Posted by davebshow <gi...@git.apache.org>.
Github user davebshow commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/451#discussion_r82239737
--- Diff: gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/remote/DriverRemoteTraversalSideEffectsTest.java ---
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.driver.remote;
+
+import org.apache.tinkerpop.gremlin.driver.AbstractResultQueueTest;
+import org.apache.tinkerpop.gremlin.driver.Client;
+import org.apache.tinkerpop.gremlin.driver.ResultSet;
+import org.apache.tinkerpop.gremlin.driver.message.RequestMessage;
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalSideEffects;
+import org.junit.Test;
+
+import java.util.UUID;
+import java.util.concurrent.CompletableFuture;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+/**
+ * @author Stephen Mallette (http://stephen.genoprime.com)
+ */
+public class DriverRemoteTraversalSideEffectsTest extends AbstractResultQueueTest {
--- End diff --
Well, I did add one test for `get()` for before and after `close()`: https://github.com/apache/tinkerpop/blob/TINKERPOP-1458/gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/remote/DriverRemoteTraversalSideEffectsTest.java#L67-L84
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #451: TINKERPOP-1458 Gremlin Server doesn't return confirmat...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/451
All tests pass with `docker/build.sh -t -n -i`
VOTE +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #451: TINKERPOP-1458 Gremlin Server doesn't return confirmat...
Posted by PommeVerte <gi...@git.apache.org>.
Github user PommeVerte commented on the issue:
https://github.com/apache/tinkerpop/pull/451
Hmm do we know why's travis is choking?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #451: TINKERPOP-1458 Gremlin Server doesn't return co...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/451#discussion_r82243790
--- Diff: gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/remote/DriverRemoteTraversalSideEffectsTest.java ---
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.driver.remote;
+
+import org.apache.tinkerpop.gremlin.driver.AbstractResultQueueTest;
+import org.apache.tinkerpop.gremlin.driver.Client;
+import org.apache.tinkerpop.gremlin.driver.ResultSet;
+import org.apache.tinkerpop.gremlin.driver.message.RequestMessage;
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalSideEffects;
+import org.junit.Test;
+
+import java.util.UUID;
+import java.util.concurrent.CompletableFuture;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+/**
+ * @author Stephen Mallette (http://stephen.genoprime.com)
+ */
+public class DriverRemoteTraversalSideEffectsTest extends AbstractResultQueueTest {
--- End diff --
ah - sorry - i didn't notice that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #451: TINKERPOP-1458 Gremlin Server doesn't return co...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/451#discussion_r82210120
--- Diff: gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/DriverRemoteTraversalSideEffects.java ---
@@ -89,9 +100,8 @@ public DriverRemoteTraversalSideEffects(final Client client, final UUID serverSi
keys = client.submitAsync(msg).get().all().get().stream().map(r -> r.getString()).collect(Collectors.toSet());
--- End diff --
At first, I didn't like that this because it replaced the whole `Set` - if you'd called `get()` first then this the set would get re-initialized when it already had keys in it. But I suppose that's not really a problem. If you don't do that then you have to check if the `Set` was initialized or not and if not new one up:
```java
if (keys == Collections.emptySet())
keys = new HashSet<>();
client.submitAsync(msg).get().all().get().forEach(r -> keys.add(r.getString())
````
A tad nicer because it gets rid of `stream()`....i dunno either way is fine i guess.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #451: TINKERPOP-1458 Gremlin Server doesn't return confirmat...
Posted by davebshow <gi...@git.apache.org>.
Github user davebshow commented on the issue:
https://github.com/apache/tinkerpop/pull/451
Alright I think this branch is ready to go. VOTE + 1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #451: TINKERPOP-1458 Gremlin Server doesn't return co...
Posted by davebshow <gi...@git.apache.org>.
Github user davebshow closed the pull request at:
https://github.com/apache/tinkerpop/pull/451
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #451: TINKERPOP-1458 Gremlin Server doesn't return co...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/451#discussion_r82210649
--- Diff: gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/DriverRemoteTraversalSideEffects.java ---
@@ -89,9 +100,8 @@ public DriverRemoteTraversalSideEffects(final Client client, final UUID serverSi
keys = client.submitAsync(msg).get().all().get().stream().map(r -> r.getString()).collect(Collectors.toSet());
} catch (Exception ex) {
final Throwable root = ExceptionUtils.getRootCause(ex);
- if (root.getMessage().equals("Could not find side-effects for " + serverSideEffect + "."))
- keys = Collections.emptySet();
- else
+ final String exMsg = null == root ? "" : root.getMessage();
+ if (!exMsg.equals("Could not find side-effects for " + serverSideEffect + "."))
--- End diff --
I know you didn't add this, but I really hate that we had to do this message checking - need a JIRA ticket to improve this (or perhaps a comment on an existing ticket regarding exceptions coming back from the server if there's a relevant one like that).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #451: TINKERPOP-1458 Gremlin Server doesn't return co...
Posted by davebshow <gi...@git.apache.org>.
Github user davebshow commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/451#discussion_r82311244
--- Diff: gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/DriverRemoteTraversalSideEffects.java ---
@@ -39,12 +40,14 @@
public class DriverRemoteTraversalSideEffects extends AbstractRemoteTraversalSideEffects {
private final Client client;
- private Set<String> keys = null;
+ private Set<String> keys = Collections.emptySet();
private final UUID serverSideEffect;
private final Host host;
private final Map<String, Object> sideEffects = new HashMap<>();
+ private boolean closed = false;
--- End diff --
How would you remedy this? Is it worth using `AtomicBoolean`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #451: TINKERPOP-1458 Gremlin Server doesn't return co...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/451#discussion_r82480587
--- Diff: gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/DriverRemoteTraversalSideEffects.java ---
@@ -28,23 +28,27 @@
import java.util.Collections;
import java.util.HashMap;
+import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import java.util.UUID;
import java.util.stream.Collectors;
/**
* @author Stephen Mallette (http://stephen.genoprime.com)
+ * This class is not thread safe.
--- End diff --
this line should go above the author line, but i'll do that after merge. not a big deal
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #451: TINKERPOP-1458 Gremlin Server doesn't return confirmat...
Posted by davebshow <gi...@git.apache.org>.
Github user davebshow commented on the issue:
https://github.com/apache/tinkerpop/pull/451
Manually closing post merge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #451: TINKERPOP-1458 Gremlin Server doesn't return co...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/451#discussion_r82212090
--- Diff: gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerIntegrateTest.java ---
@@ -814,18 +829,88 @@ public void shouldStillSupportDeprecatedRebindingsParameterOnServer() throws Exc
@Test
public void shouldSupportLambdasUsingWithRemote() throws Exception {
- final Supplier<Graph> graphGetter = () -> server.getServerGremlinExecutor().getGraphManager().getGraphs().get("graph");
- final Configuration conf = new BaseConfiguration() {{
- setProperty(Graph.GRAPH, RemoteGraph.class.getName());
- setProperty(GREMLIN_REMOTE_CONNECTION_CLASS, DriverRemoteConnection.class.getName());
- setProperty(DriverRemoteConnection.GREMLIN_REMOTE_DRIVER_SOURCENAME, "g");
- setProperty("hidden.for.testing.only", graphGetter);
- }};
-
final Graph graph = EmptyGraph.instance();
final GraphTraversalSource g = graph.traversal().withRemote(conf);
g.addV("person").property("age", 20).iterate();
g.addV("person").property("age", 10).iterate();
assertEquals(50L, g.V().hasLabel("person").map(Lambda.function("it.get().value('age') + 10")).sum().next());
}
+
+ @Test
+ public void shouldGetSideEffectKeysUsingWithRemote() throws Exception {
+ final Graph graph = EmptyGraph.instance();
+ final GraphTraversalSource g = graph.traversal().withRemote(conf);
+ g.addV("person").property("age", 20).iterate();
+ g.addV("person").property("age", 10).iterate();
+ final GraphTraversal traversal = g.V().aggregate("a").aggregate("b");
+ traversal.iterate();
+ final DriverRemoteTraversalSideEffects se = (DriverRemoteTraversalSideEffects) traversal.asAdmin().getSideEffects();
+
+ // Get keys
+ final Set<String> sideEffectKeys = se.keys();
+ assertEquals(2, sideEffectKeys.size());
+
+ // Get side effects
+ final BulkSet aSideEffects = se.get("a");
+ assertThat(aSideEffects.isEmpty(), is(false));
+ final BulkSet bSideEffects = se.get("b");
+ assertThat(bSideEffects.isEmpty(), is(false));
+
+ // Should get local keys/side effects after close
+ se.close();
+
+ final Set<String> localSideEffectKeys = se.keys();
+ assertEquals(2, localSideEffectKeys.size());
+
+ final BulkSet localASideEffects = se.get("a");
+ assertThat(localASideEffects.isEmpty(), is(false));
+
+ final BulkSet localBSideEffects = se.get("b");
+ assertThat(localBSideEffects.isEmpty(), is(false));
+ }
+
+ @Test
+ public void shouldCloseSideEffectsUsingWithRemote() throws Exception {
+ final Graph graph = EmptyGraph.instance();
+ final GraphTraversalSource g = graph.traversal().withRemote(conf);
+ g.addV("person").property("age", 20).iterate();
+ g.addV("person").property("age", 10).iterate();
+ final GraphTraversal traversal = g.V().aggregate("a").aggregate("b");
+ traversal.iterate();
+ final DriverRemoteTraversalSideEffects se = (DriverRemoteTraversalSideEffects) traversal.asAdmin().getSideEffects();
+ final BulkSet sideEffects = se.get("a");
+ assertThat(sideEffects.isEmpty(), is(false));
+ se.close();
+
+ // Can't get new side effects after close
+ assertNull(se.get("b"));
+
+ // Earlier keys should be cached locally
+ final Set<String> localSideEffectKeys = se.keys();
+ assertEquals(1, localSideEffectKeys.size());
+ final BulkSet localSideEffects = se.get("a");
+ assertThat(localSideEffects.isEmpty(), is(false));
+
+ // Try to get side effect from server
+ final Cluster cluster = Cluster.build("localhost").create();
+ final Client client = cluster.connect();
+ Field field = DriverRemoteTraversalSideEffects.class.getDeclaredField("serverSideEffect");
--- End diff --
please `final` your `Field` and `UUID` variables. glad the reflection trick worked for this test.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #451: TINKERPOP-1458 Gremlin Server doesn't return confirmat...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/451
VOTE +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #451: Tinkerpop 1458 Gremlin Server doesn't return co...
Posted by davebshow <gi...@git.apache.org>.
Github user davebshow commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/451#discussion_r82095012
--- Diff: gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/DriverRemoteTraversalSideEffects.java ---
@@ -54,22 +57,30 @@ public DriverRemoteTraversalSideEffects(final Client client, final UUID serverSi
@Override
public <V> V get(final String key) throws IllegalArgumentException {
if (!sideEffects.containsKey(key)) {
- // specify the ARGS_HOST so that the LoadBalancingStrategy is subverted and the connection is forced
- // from the specified host (i.e. the host from the previous request as that host will hold the side-effects)
- final RequestMessage msg = RequestMessage.build(Tokens.OPS_GATHER)
- .addArg(Tokens.ARGS_SIDE_EFFECT, serverSideEffect)
- .addArg(Tokens.ARGS_SIDE_EFFECT_KEY, key)
- .addArg(Tokens.ARGS_HOST, host)
- .processor("traversal").create();
- try {
- final Result result = client.submitAsync(msg).get().one();
- sideEffects.put(key, null == result ? null : result.getObject());
- } catch (Exception ex) {
- final Throwable root = ExceptionUtils.getRootCause(ex);
- if (root.getMessage().equals("Could not find side-effects for " + serverSideEffect + "."))
- sideEffects.put(key, null);
- else
- throw new RuntimeException("Could not get keys", root);
+ if (!closed) {
+ // specify the ARGS_HOST so that the LoadBalancingStrategy is subverted and the connection is forced
+ // from the specified host (i.e. the host from the previous request as that host will hold the side-effects)
+ final RequestMessage msg = RequestMessage.build(Tokens.OPS_GATHER)
+ .addArg(Tokens.ARGS_SIDE_EFFECT, serverSideEffect)
+ .addArg(Tokens.ARGS_SIDE_EFFECT_KEY, key)
+ .addArg(Tokens.ARGS_HOST, host)
+ .processor("traversal").create();
+ try {
+ final Result result = client.submitAsync(msg).get().one();
--- End diff --
Should this be `all` instead of `one`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #451: TINKERPOP-1458 Gremlin Server doesn't return confirmat...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/451
yeah - will figure it out next week - it's this nonsense:
```text
TraversalInterruptionTest.shouldRespectThreadInterruptionInVertexStep:111
Expected: is <true>
but: was <false>
TraversalInterruptionTest.shouldRespectThreadInterruptionInVertexStep:111
Expected: is <true>
but: was <false>
```
i mentioned it in the "code freeze" post to dev list. not related to this PR (which is already merged btw - @davebshow needs to close it on his end because i flubbed the merge a little bit and rebased after some conflicts on master)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #451: TINKERPOP-1458 Gremlin Server doesn't return co...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/451#discussion_r82211005
--- Diff: gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/DriverRemoteTraversalSideEffects.java ---
@@ -39,12 +40,14 @@
public class DriverRemoteTraversalSideEffects extends AbstractRemoteTraversalSideEffects {
private final Client client;
- private Set<String> keys = null;
+ private Set<String> keys = Collections.emptySet();
private final UUID serverSideEffect;
private final Host host;
private final Map<String, Object> sideEffects = new HashMap<>();
+ private boolean closed = false;
--- End diff --
I suppose it is ok that the close status isn't really thread safe. I guess it would be weird for someone to make multi-threaded calls over the side-effect object. That seems unusual. Just thought i'd point that out.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---