Bind all graphs to gremlin executor context #884

dpitera · 2018-01-11T22:11:00Z

Issues: #883

Signed-off-by: David Pitera [email protected]

Thank you for contributing to JanusGraph!

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there an issue associated with this PR? Is it referenced in the commit message?
Does your PR body contain #xyz where xyz is the issue number you are trying to resolve?
Has your PR been rebased against the latest commit within the target branch (typically master)?
Is your initial contribution a single, squashed commit?

For code changes:

Have you written and/or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE.txt file, including the main LICENSE.txt file in the root of this repository?
If applicable, have you updated the NOTICE.txt file, including the main NOTICE.txt file found in the root of this repository?

For documentation related changes:

Have you ensured that format looks appropriate for the output in which it is rendered?
If this PR is a documentation-only change, have you added a [skip ci]
tag to the first line of your commit message to avoid spending CPU cycles in
Travis CI when no code, tests, or build configuration are modified?

Note:

Please ensure that once the PR is submitted, you check Travis CI for build issues and submit an update to your PR as soon as possible.

sjudeng

Can these classes be moved to the new janusgraph-server module?

sjudeng · 2018-01-27T22:51:54Z

janusgraph-core/src/main/java/org/janusgraph/core/channelizers/JanusGraphChannelizer.java

@@ -0,0 +1,27 @@
+// Copyright 2017 JanusGraph Authors


@mbrukman For files updated/added in 2018 should copyright be updated to "2018" or "2017-2018"?

@mbrukman What is the final word here?

If this is a brand new file created in 2018 (which is what it appears to be since it's not branched and appears to be added in this PR), it should be (c) 2018.

If this is a modification of an existing file which as created in 2017 and (c) 2017, there's no need to update the year of copyright, just keep it as it was originally.

sjudeng · 2018-01-27T22:52:06Z

janusgraph-core/src/main/java/org/janusgraph/core/channelizers/JanusGraphHttpChannelizer.java

+    public void init(final ServerGremlinExecutor serverGremlinExecutor) {
+        this.serverGremlinExecutor = serverGremlinExecutor;
+        super.init(serverGremlinExecutor);
+        GraphManager graphManager = serverGremlinExecutor.getGraphManager();


final here and throughout PR

sjudeng · 2018-01-27T23:00:04Z

janusgraph-test/src/test/java/org/janusgraph/graphdb/tinkerpop/TestClientFactory.java

+/**
+ * @author Stephen Mallette (http://stephen.genoprime.com)
+ */
+public final class TestClientFactory {


Can you just use TestClientFactory from org.apache.tinkerpop:gremlin-server directly here? This looks identical to that class and the module is already a dependency in janusgraph-test. If it does need to be duplicated here then the original copyright header should be preserved.

As a part of this work, I have pulled in the
AbstractGremlinServerIntegrationTest from TinkerPop as it is part of the
src/test/** resources under the gremlin-server distribution, and these
test resources are not packaged in the jar. As a quick short term
solution, this allows us to test inner server components, like we have
to do to test this changeset.

If this file is from Apache TinkerPop project, it should retain the original license / copyright header, not inherit the one from JanusGraph.

mbrukman · 2018-02-07T03:28:59Z

...raph-core/src/main/java/org/janusgraph/core/channelizers/JanusGraphWsAndHttpChannelizer.java

+// 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.
+


Extra blank line (one blank line between license header and package, please).

mbrukman · 2018-02-07T03:29:03Z

...raph-core/src/main/java/org/janusgraph/core/channelizers/JanusGraphWebSocketChannelizer.java

+// 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.
+


Extra blank line (one blank line between license header and package, please).

mbrukman · 2018-02-07T03:29:07Z

janusgraph-core/src/main/java/org/janusgraph/core/channelizers/JanusGraphNioChannelizer.java

+// 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.
+


Extra blank line (one blank line between license header and package, please).

mbrukman · 2018-02-07T03:29:14Z

janusgraph-core/src/main/java/org/janusgraph/core/channelizers/JanusGraphHttpChannelizer.java

+// 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.
+


Extra blank line (one blank line between license header and package, please).

dpitera · 2018-02-07T20:17:29Z

@mbrukman @sjudeng Thanks for your advice. Pushed new code with suggested updates.

mbrukman · 2018-02-08T05:14:43Z

docs/configuredgraphfactory.adoc

@@ -67,6 +67,8 @@ You can either use `ConfiguredGraphFactory.create("graphName")`
 or `ConfiguredGraphFactory.open("graphName")`. Learn more about the difference
 between these two options by reading the section below about the `ConfigurationManagementGraph`.

+You can also access your graphs by using the bindings. Read more about this in the <<graphandtraversalbindings>> section.


The name graphandtraversalbindings will become either an anchor or a full page, but it's not very readable.

Can you use dashes to separate the words, please? graph-and-traversal-bindings is a valid identifier (like listing-the-graphs you can see below), and is much more readable and scannable.

mbrukman · 2018-02-08T05:15:52Z

docs/configuredgraphfactory.adoc

+
+IMPORTANT: The JanusGraphManager rebinds every graph stored on the ConfigurationManagementGraph (or those for which you have created configurations) every 20 seconds. This means your graph and traversal bindings for graphs created using the ConfigredGraphFactory will be available on all JanusGraph nodes with a maximum of a 20 second lag. It also means that a binding will still be available on a node after a server restart.
+
+[[bindingexample]]


Similar comment for the name bindingexample -> binding-example as above.

dpitera · 2018-02-08T18:16:34Z

@mbrukman Pushed doc updates.

sjudeng

Looks good to me. Hopefully we can get another review by someone more familiar with Gremlin Server.

mbrukman · 2018-02-09T00:24:26Z

...ver/src/test/java/org/janusgraph/graphdb/tinkerpop/AbstractGremlinServerIntegrationTest.java

@@ -0,0 +1,130 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one


This file was taken (and modified, I assume?) from the TinkerPop project? Would it make sense to document:

where it came from (e.g., path to file in GitHub or ASF Git)

what local modifications were made

whether it would make sense to remove it and depend on the original source file in the future

mbrukman · 2018-02-09T00:24:39Z

janusgraph-server/src/test/java/org/janusgraph/graphdb/tinkerpop/TestClientFactory.java

@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one


Same comment here as for the other file.

pluradj

added my review

pluradj · 2018-02-21T15:30:38Z

docs/configuredgraphfactory.adoc

+
+Graphs created using the ConfiguredGraphFactory are bound to the GremlinServer's Executor's Context by the "graph.graphname" property, and the graph's traversal reference is bound to the context by "<graphname>_traversal". This means, on subsequent connections to the server after the first time you create/open a graph, you can access the graph and traversal references by the "<graphname>" and "<graphname>_traversal" properties.
+
+IMPORTANT: If you are connected to a remote GremlinServer using the gremlin-console and a *sessioned* connection, then you will have to reconnect to the server to bind the variables. This is probably the case for any sessioned WebSocket connection.


s/GremlinServer/Gremlin Server
s/gremlin-console/Gremlin Console
s/probably the case/is also true

pluradj · 2018-02-21T15:34:01Z

docs/configuredgraphfactory.adoc

+[[graph-and-traversal-bindings]]
+=== Graph and Traversal Bindings
+
+Graphs created using the ConfiguredGraphFactory are bound to the GremlinServer's Executor's Context by the "graph.graphname" property, and the graph's traversal reference is bound to the context by "<graphname>_traversal". This means, on subsequent connections to the server after the first time you create/open a graph, you can access the graph and traversal references by the "<graphname>" and "<graphname>_traversal" properties.


s/GremlinServer's Executor's Context/executor context on the Gremlin Server

pluradj · 2018-02-21T15:34:33Z

docs/configuredgraphfactory.adoc

+
+IMPORTANT: If you are connected to a remote GremlinServer using the gremlin-console and a *sessioned* connection, then you will have to reconnect to the server to bind the variables. This is probably the case for any sessioned WebSocket connection.
+
+IMPORTANT: The JanusGraphManager rebinds every graph stored on the ConfigurationManagementGraph (or those for which you have created configurations) every 20 seconds. This means your graph and traversal bindings for graphs created using the ConfigredGraphFactory will be available on all JanusGraph nodes with a maximum of a 20 second lag. It also means that a binding will still be available on a node after a server restart.


is the 20 second interval configurable?

How does the rebinding operation affect incoming requests? Is there a failure scenario possible if a request comes in during rebinding?

I don't believe so. Basically, before the rebinding, the user cannot access graphs and traversals by their bindings, post rebinding completion, they can.

pluradj · 2018-02-21T15:36:05Z

janusgraph-core/src/main/java/org/janusgraph/graphdb/management/JanusGraphManager.java

+        final ScheduledExecutorService bindExecutor = Executors.newScheduledThreadPool(1);
+        // Dynamically created graphs created with the ConfiguredGraphFactory are
+        // bound across all nodes in the cluster and in the face of server restarts
+        bindExecutor.scheduleWithFixedDelay(new GremlinExecutorGraphBinder(this.gremlinExecutor), 0, 20L, TimeUnit.SECONDS);


would it make sense to make the 20 second interval be configurable?

It would be awkward to do this, as this is not tied to a specific graph and the only way to inject configuration settings into the JG server is by using graph configuration options.

We could make it a graph config option, but it would be awkward cause it would only be relevant upon the call to configureGremlinExecutor which would happen once at server start. We could make it only a config option read off of the ConfigurationManagementGraph's configuration settings, but I worry that will be confusing as well.

Thanks. I'm ok with leaving it as a constant. I don't think there is an immediate need to configure it.

pluradj · 2018-02-21T15:38:24Z

janusgraph-core/src/main/java/org/janusgraph/graphdb/management/JanusGraphManager.java

+                    this.gremlinExecutor.getScriptEngineManager().put(it, graph);
+                    this.gremlinExecutor.getScriptEngineManager().put(it + "_traversal", graph.traversal());
+                } catch (Exception e) {
+                    // cannot open graph, do nothing


log the error

pluradj · 2018-02-21T15:48:45Z

janusgraph-server/src/main/java/org/janusgraph/channelizers/JanusGraphChannelizer.java

+ */
+public interface JanusGraphChannelizer {
+
+    public void init(final ServerGremlinExecutor serverGremlinExecutor);


javadoc for this method

pluradj · 2018-02-21T15:49:36Z

...ver/src/test/java/org/janusgraph/graphdb/tinkerpop/AbstractGremlinServerIntegrationTest.java

+
+/*
+ * This file was pulled from the Apache TinkerPop project:
+ * https://github.com/apache/tinkerpop/blob/master/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/AbstractGremlinServerIntegrationTest.java


point at a specific version instead of master

pluradj · 2018-02-21T15:50:10Z

janusgraph-server/src/test/java/org/janusgraph/graphdb/tinkerpop/TestClientFactory.java

+
+/*
+ * This file was pulled from the Apache TinkerPop project:
+ * https://github.com/apache/tinkerpop/blob/master/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/TestClientFactory.java


point at a specific version instead of master

pluradj · 2018-02-21T15:55:36Z

janusgraph-dist/src/assembly/static/conf/gremlin-server/gremlin-server-configuration.yaml

@@ -1,7 +1,7 @@
 host: 0.0.0.0
 port: 8182
 scriptEvaluationTimeout: 30000
-channelizer: org.apache.tinkerpop.gremlin.server.channel.WebSocketChannelizer
+channelizer: org.janusgraph.channelizers.JanusGraphWebSocketChannelizer


This is going to cause a lot of confusion when configuring the Gremlin Server. Adding comments in all the Gremlin Server yaml files would be helpful to tell users when to use a TinkerPop WebSocketChannelizer instead of JanusGraphWebSocketChannelizer. Also add similar information in the documentation.

I think it would be more confusing if I put comments in the yaml files. Currently there are two YAMl files, one when using the CMG and one when not. The JG channelizers are in place in the CMG YAML because this is when you can make use of the channelizer, and not in the normal YAML because it can't work there. I think the existence of the separate files is enough differentiation between when one could/should/can't use the JG channelizers (of course along with the appropriate documentation on the matter)-- and I have already added to the CGF setting on when it is appropriate to use the JG channelizers and how to configure the YAMl to use so.

Does that make sense?

OK. Make sure to clarify in the docs then. Is there a migration hit for apps that were using CGF in JG 0.2.0?

Any app previously using the CGF can make use of this new binding functionality by making use of the new channelizers. That's about it in terms of steps forward for people previously using the CGF.

It's optional though, correct? It is not required to use the new channelizers.

pluradj · 2018-02-21T16:01:58Z

janusgraph-server/src/test/java/org/janusgraph/graphdb/tinkerpop/JanusGraphChannelizerTest.java

+                "org.janusgraph.core.ConfiguredGraphFactory.createTemplateConfiguration(new MapConfiguration(map));" +
+                "org.janusgraph.core.ConfiguredGraphFactory.create(\"newGraph\")");
+            //assert newGraph is indeed bound
+            assertEquals(0, client.submit("newGraph.vertices().size()").all().get().get(0).getInt());


add tests to verify that newGraph_traversal is bound and newGraph_traversal.getGraph() is the same as newGraph.

dpitera · 2018-02-22T20:05:39Z

@pluradj Repushed with requested changes.

pluradj · 2018-02-22T22:08:50Z

docs/configuredgraphfactory.adoc

+
+Graphs created using the ConfiguredGraphFactory are bound to the executor context on the Gremlin Server by the "graph.graphname" property, and the graph's traversal reference is bound to the context by "<graphname>_traversal". This means, on subsequent connections to the server after the first time you create/open a graph, you can access the graph and traversal references by the "<graphname>" and "<graphname>_traversal" properties.
+
+IMPORTANT: If you are connected to a remote Gremlin Server using the Gremlkin Console and a *sessioned* connection, then you will have to reconnect to the server to bind the variables. This is also true for any sessioned WebSocket connection.


s/Gremlkin/Gremlin

pluradj · 2018-02-22T22:13:36Z

...ver/src/test/java/org/janusgraph/graphdb/tinkerpop/AbstractGremlinServerIntegrationTest.java

+
+/*
+ * This file was pulled from the Apache TinkerPop project:
+ * https://github.com/apache/tinkerpop/blob/tp32/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/AbstractGremlinServerIntegrationTest.java


s/tp32/3.2.7
tp32 is a branch, 3.2.7 is a tagged version

pluradj · 2018-02-22T22:13:43Z

janusgraph-server/src/test/java/org/janusgraph/graphdb/tinkerpop/TestClientFactory.java

+
+/*
+ * This file was pulled from the Apache TinkerPop project:
+ * https://github.com/apache/tinkerpop/blob/tp32/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/TestClientFactory.java


s/tp32/3.2.7
tp32 is a branch, 3.2.7 is a tagged version

pluradj · 2018-02-22T22:22:24Z

@dpitera I didn't see any doc updates in configuredgraphfactory.adoc for the channelizers.

dpitera · 2018-02-23T21:54:39Z

@dpitera I didn't see any doc updates in configuredgraphfactory.adoc for the channelizers.

Oh u are right. Looking back, I remember that I just assumed everyone using 0.3.0 CFG would use the provided YAML distr and thus have this functionality, but I will update the section I added to include a notion of how this happens, i.e. by configuring the server to use the JG channelizers.

Furthermore, any graphs stored on the ConfigurationManagementGraph are bound every 20 seconds using a ScheduledExecutorService. This is useful for 2 reasons: 1. This allows graph bindings to be global across all JanusGraph nodes in a deployment. A user can therefore "create" a graph once and expect the binding to find itself on every node in the cluster in <20 seconds. This is particularly useful for users using services hidden behind third-party managed routers. 2. If a server is restart, the binding will be rebound on the server without having to reopen the graph again. As a part of this work, I have pulled in the AbstractGremlinServerIntegrationTest from TinkerPop as it is part of the src/test/** resources under the gremlin-server distribution, and these test resources are not packaged in the jar. As a quick short term solution, this allows us to test inner server components, like we have to do to test this changeset. Issues: JanusGraph#883 Signed-off-by: David Pitera <[email protected]>

dpitera · 2018-02-26T17:43:51Z

@pluradj Pushed with suggested updates

pluradj

thanks david

Bind all graphs to gremlin executor context

janusgraph-bot added the cla: yes This PR is compliant with the CLA label Jan 11, 2018

dpitera force-pushed the dynamic-bindings-new branch 3 times, most recently from cfb3eeb to f9511c7 Compare January 16, 2018 16:31

sjudeng requested changes Jan 28, 2018

View reviewed changes

janusgraph-bot added the cla: yes This PR is compliant with the CLA label Feb 3, 2018

mbrukman reviewed Feb 7, 2018

View reviewed changes

dpitera force-pushed the dynamic-bindings-new branch from f9511c7 to c9dcbff Compare February 7, 2018 17:04

dpitera force-pushed the dynamic-bindings-new branch from c9dcbff to 261c750 Compare February 7, 2018 20:49

mbrukman reviewed Feb 8, 2018

View reviewed changes

dpitera force-pushed the dynamic-bindings-new branch from 261c750 to 4295fe1 Compare February 8, 2018 18:15

sjudeng approved these changes Feb 9, 2018

View reviewed changes

sjudeng assigned pluradj and unassigned pluradj Feb 9, 2018

sjudeng requested a review from pluradj February 9, 2018 01:04

mbrukman reviewed Feb 9, 2018

View reviewed changes

dpitera force-pushed the dynamic-bindings-new branch 2 times, most recently from 6c51d80 to 579de33 Compare February 21, 2018 15:49

pluradj requested changes Feb 21, 2018

View reviewed changes

dpitera force-pushed the dynamic-bindings-new branch from 579de33 to 51df174 Compare February 22, 2018 20:04

pluradj reviewed Feb 22, 2018

View reviewed changes

dpitera mentioned this pull request Feb 23, 2018

Evict graph from cache upon configuration updates #897

Merged

10 tasks

pluradj mentioned this pull request Feb 25, 2018

update ConfiguredGraphFactory docs for new channelizers and graph eviction #938

Closed

dpitera force-pushed the dynamic-bindings-new branch from 51df174 to 0071e7c Compare February 26, 2018 17:43

janusgraph-bot added the cla: yes This PR is compliant with the CLA label Mar 13, 2018

pluradj approved these changes Mar 16, 2018

View reviewed changes

dpitera merged commit 7afe62d into JanusGraph:master Mar 16, 2018

pluradj added this to the Release v0.3.0 milestone Mar 29, 2018

bwatson-rti-org pushed a commit to bwatson-rti-org/janusgraph that referenced this pull request Mar 9, 2019

Merge pull request JanusGraph#884 from dpitera/dynamic-bindings-new

fd77506

Bind all graphs to gremlin executor context

micpod pushed a commit to micpod/janusgraph that referenced this pull request Nov 5, 2019

Merge pull request JanusGraph#884 from dpitera/dynamic-bindings-new

a562bd1

Bind all graphs to gremlin executor context


		IMPORTANT: The JanusGraphManager rebinds every graph stored on the ConfigurationManagementGraph (or those for which you have created configurations) every 20 seconds. This means your graph and traversal bindings for graphs created using the ConfigredGraphFactory will be available on all JanusGraph nodes with a maximum of a 20 second lag. It also means that a binding will still be available on a node after a server restart.

		[[bindingexample]]

		@@ -0,0 +1,130 @@
		/*
		* Licensed to the Apache Software Foundation (ASF) under one

		@@ -0,0 +1,62 @@
		/*
		* Licensed to the Apache Software Foundation (ASF) under one


		Graphs created using the ConfiguredGraphFactory are bound to the GremlinServer's Executor's Context by the "graph.graphname" property, and the graph's traversal reference is bound to the context by "<graphname>_traversal". This means, on subsequent connections to the server after the first time you create/open a graph, you can access the graph and traversal references by the "<graphname>" and "<graphname>_traversal" properties.

		IMPORTANT: If you are connected to a remote GremlinServer using the gremlin-console and a sessioned connection, then you will have to reconnect to the server to bind the variables. This is probably the case for any sessioned WebSocket connection.

Bind all graphs to gremlin executor context #884

Bind all graphs to gremlin executor context #884

Conversation

dpitera commented Jan 11, 2018 • edited Loading

For all changes:

For code changes:

For documentation related changes:

Note:

sjudeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpitera commented Feb 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpitera commented Feb 8, 2018

sjudeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pluradj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpitera commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pluradj commented Feb 22, 2018

dpitera commented Feb 23, 2018

dpitera commented Feb 26, 2018

pluradj left a comment

Choose a reason for hiding this comment

dpitera commented Jan 11, 2018 •

edited

Loading