You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by "kenhuuu (via GitHub)" <gi...@apache.org> on 2025/12/02 20:15:06 UTC
[PR] TINKERPOP-3217 Add server option to close Session automatically. [tinkerpop]
kenhuuu opened a new pull request, #3284:
URL: https://github.com/apache/tinkerpop/pull/3284
The added destroySessionPostGraphOp setting enables re-using the same underlying connection for a different subsequent Session. This should increase performance for cases where many short-lived Transactions are sent to the server.
https://issues.apache.org/jira/browse/TINKERPOP-3217
<!--
Thanks for contributing! Reminders:
+ TARGET the earliest branch where you want the change
3.7-dev -> 3.7.6 (non-breaking only)
3.8-dev -> 3.8.1 (non-breaking only)
master -> 4.0.0
+ Committers will MERGE the PR forward to newer versions
+ ADD entry to the CHANGELOG.asciidoc for the targeted version
Do not reference a JIRA number there
+ ADD JIRA number to title and link in description
+ PRs requires 3 +1s from committers OR
1 +1 and 7 day wait to merge.
+ MORE details: https://s.apache.org/rtnal
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3217 Add server option to close Session automatically. [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on PR #3284:
URL: https://github.com/apache/tinkerpop/pull/3284#issuecomment-3629013988
VOTE +1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3217 Add server option to close Session automatically. [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3284:
URL: https://github.com/apache/tinkerpop/pull/3284#discussion_r2590465146
##########
gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinSessionReuseTxIntegrateTest.java:
##########
@@ -0,0 +1,411 @@
+/*
+ * 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.server;
+
+import org.apache.tinkerpop.gremlin.server.op.session.SessionOpProcessor;
+import org.apache.tinkerpop.gremlin.util.ExceptionHelper;
+import org.apache.tinkerpop.gremlin.driver.Client;
+import org.apache.tinkerpop.gremlin.driver.Cluster;
+import org.apache.tinkerpop.gremlin.driver.RequestOptions;
+import org.apache.tinkerpop.gremlin.driver.remote.DriverRemoteConnection;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.junit.Ignore;
+import org.junit.Test;
+
+import java.io.File;
+import java.util.HashMap;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+
+import static org.apache.tinkerpop.gremlin.process.traversal.AnonymousTraversalSource.traversal;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+/**
+ * Integration tests for gremlin-driver and bytecode sessions.
+ *
+ * NOTE: this is effectively a copy of GremlinSessionTxIntegrateTest but the expectation is that the gremlin-driver
Review Comment:
Is there a way to actually verify the connection is reused or the connection state such that it can be reused?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3217 Add server option to close Session automatically. [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #3284:
URL: https://github.com/apache/tinkerpop/pull/3284#discussion_r2591615350
##########
gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinSessionReuseTxIntegrateTest.java:
##########
@@ -0,0 +1,411 @@
+/*
+ * 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.server;
+
+import org.apache.tinkerpop.gremlin.server.op.session.SessionOpProcessor;
+import org.apache.tinkerpop.gremlin.util.ExceptionHelper;
+import org.apache.tinkerpop.gremlin.driver.Client;
+import org.apache.tinkerpop.gremlin.driver.Cluster;
+import org.apache.tinkerpop.gremlin.driver.RequestOptions;
+import org.apache.tinkerpop.gremlin.driver.remote.DriverRemoteConnection;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.junit.Ignore;
+import org.junit.Test;
+
+import java.io.File;
+import java.util.HashMap;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+
+import static org.apache.tinkerpop.gremlin.process.traversal.AnonymousTraversalSource.traversal;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+/**
+ * Integration tests for gremlin-driver and bytecode sessions.
+ *
+ * NOTE: this is effectively a copy of GremlinSessionTxIntegrateTest but the expectation is that the gremlin-driver
Review Comment:
There are server side metrics and/or logs that can be asserted against when a session is created/destroyed. This is a test that should be added but needs to wait on https://github.com/apache/tinkerpop/pull/3258
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3217 Add server option to close Session automatically. [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #3284:
URL: https://github.com/apache/tinkerpop/pull/3284#discussion_r2591610372
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Settings.java:
##########
@@ -189,6 +189,14 @@ public Settings() {
*/
public boolean strictTransactionManagement = false;
+ /**
+ * If set to {@code true} the Gremlin Server will destroy the session when a GraphOp (commit or rollback) is
+ * successfully completed on that session.
+ *
+ * NOTE: Defaults to false in 3.7.6/3.8.1 to prevent breaking change.
Review Comment:
Do you mean you'd prefer it to say 3.7.x/3.8.x? I changed it to that instead for clarity. Let me know if you meant something else.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3217 Add server option to close Session automatically. [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #3284:
URL: https://github.com/apache/tinkerpop/pull/3284#discussion_r2591605719
##########
gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinSessionReuseTxIntegrateTest.java:
##########
@@ -0,0 +1,411 @@
+/*
+ * 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.server;
+
+import org.apache.tinkerpop.gremlin.server.op.session.SessionOpProcessor;
+import org.apache.tinkerpop.gremlin.util.ExceptionHelper;
+import org.apache.tinkerpop.gremlin.driver.Client;
+import org.apache.tinkerpop.gremlin.driver.Cluster;
+import org.apache.tinkerpop.gremlin.driver.RequestOptions;
+import org.apache.tinkerpop.gremlin.driver.remote.DriverRemoteConnection;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.junit.Ignore;
+import org.junit.Test;
+
+import java.io.File;
+import java.util.HashMap;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+
+import static org.apache.tinkerpop.gremlin.process.traversal.AnonymousTraversalSource.traversal;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+/**
+ * Integration tests for gremlin-driver and bytecode sessions.
+ *
+ * NOTE: this is effectively a copy of GremlinSessionTxIntegrateTest but the expectation is that the gremlin-driver
+ * will be configured in a way that allows connection re-use as the server allows it via the "destroySessionPostGraphOp"
+ * setting.
+ */
+public class GremlinSessionReuseTxIntegrateTest extends AbstractGremlinServerIntegrationTest {
+
+ /**
+ * Configure specific Gremlin Server settings for specific tests.
+ */
+ @Override
+ public Settings overrideSettings(final Settings settings) {
+ final String nameOfTest = name.getMethodName();
+
+ settings.graphs.put("graph", "conf/tinkertransactiongraph-empty.properties");
+
+ // Set this as it is what allows connection re-use on the server side.
+ settings.destroySessionPostGraphOp = true;
+
+ switch (nameOfTest) {
+ case "shouldExecuteBytecodeInSession":
+ break;
+ case "shouldTimeoutTxBytecode":
+ settings.processors.clear();
+
+ // OpProcessor setting
+ final Settings.ProcessorSettings processorSettings = new Settings.ProcessorSettings();
+ processorSettings.className = SessionOpProcessor.class.getCanonicalName();
+ processorSettings.config = new HashMap<>();
+ processorSettings.config.put(SessionOpProcessor.CONFIG_SESSION_TIMEOUT, 3000L);
+ settings.processors.add(processorSettings);
+
+ // Unified setting
+ settings.sessionLifetimeTimeout = 3000L;
+ break;
+ }
+
+ return settings;
+ }
+
Review Comment:
Yes, but this can't be tested right now because the driver automatically closes the connection after commit/rollback. This test needs to wait on https://github.com/apache/tinkerpop/pull/3258 .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3217 Add server option to close Session automatically. [tinkerpop]
Posted by "andrii0lomakin (via GitHub)" <gi...@apache.org>.
andrii0lomakin commented on PR #3284:
URL: https://github.com/apache/tinkerpop/pull/3284#issuecomment-3663994697
@kenhuuu we use our own fork, so we can use it already now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3217 Add server option to close Session automatically. [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #3284:
URL: https://github.com/apache/tinkerpop/pull/3284#discussion_r2591607917
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/session/SessionOpProcessor.java:
##########
@@ -571,6 +582,13 @@ protected void handleGraphOperation(final Bytecode bytecode, final Graph graph,
.statusMessage(t.getMessage())
.statusAttributeException(t).create());
}
+
+ if (destroySessionPostGraphOp) {
+ // Destroy the session after a successful rollback due to error. Placed here rather than
+ // in a finally block since we don't want to end the session if no commit/rollback succeeded.
+ session.manualKill(true);
Review Comment:
This shouldn't happen, but even if it does, theres explicit handling in that method for that situation so it's safe to call it more than once.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3217 Add server option to close Session automatically. [tinkerpop]
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on code in PR #3284:
URL: https://github.com/apache/tinkerpop/pull/3284#discussion_r2582934583
##########
gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinSessionReuseTxIntegrateTest.java:
##########
@@ -0,0 +1,411 @@
+/*
+ * 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.server;
+
+import org.apache.tinkerpop.gremlin.server.op.session.SessionOpProcessor;
+import org.apache.tinkerpop.gremlin.util.ExceptionHelper;
+import org.apache.tinkerpop.gremlin.driver.Client;
+import org.apache.tinkerpop.gremlin.driver.Cluster;
+import org.apache.tinkerpop.gremlin.driver.RequestOptions;
+import org.apache.tinkerpop.gremlin.driver.remote.DriverRemoteConnection;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.junit.Ignore;
+import org.junit.Test;
+
+import java.io.File;
+import java.util.HashMap;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+
+import static org.apache.tinkerpop.gremlin.process.traversal.AnonymousTraversalSource.traversal;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+/**
+ * Integration tests for gremlin-driver and bytecode sessions.
+ *
+ * NOTE: this is effectively a copy of GremlinSessionTxIntegrateTest but the expectation is that the gremlin-driver
+ * will be configured in a way that allows connection re-use as the server allows it via the "destroySessionPostGraphOp"
+ * setting.
+ */
+public class GremlinSessionReuseTxIntegrateTest extends AbstractGremlinServerIntegrationTest {
+
+ /**
+ * Configure specific Gremlin Server settings for specific tests.
+ */
+ @Override
+ public Settings overrideSettings(final Settings settings) {
+ final String nameOfTest = name.getMethodName();
+
+ settings.graphs.put("graph", "conf/tinkertransactiongraph-empty.properties");
+
+ // Set this as it is what allows connection re-use on the server side.
+ settings.destroySessionPostGraphOp = true;
+
+ switch (nameOfTest) {
+ case "shouldExecuteBytecodeInSession":
+ break;
+ case "shouldTimeoutTxBytecode":
+ settings.processors.clear();
+
+ // OpProcessor setting
+ final Settings.ProcessorSettings processorSettings = new Settings.ProcessorSettings();
+ processorSettings.className = SessionOpProcessor.class.getCanonicalName();
+ processorSettings.config = new HashMap<>();
+ processorSettings.config.put(SessionOpProcessor.CONFIG_SESSION_TIMEOUT, 3000L);
+ settings.processors.add(processorSettings);
+
+ // Unified setting
+ settings.sessionLifetimeTimeout = 3000L;
+ break;
+ }
+
+ return settings;
+ }
+
Review Comment:
Should there be a test where session is auto destroyed after a rollback due an error?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3217 Add server option to close Session automatically. [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu merged PR #3284:
URL: https://github.com/apache/tinkerpop/pull/3284
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3217 Add server option to close Session automatically. [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3284:
URL: https://github.com/apache/tinkerpop/pull/3284#discussion_r2590412184
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/session/SessionOpProcessor.java:
##########
@@ -571,6 +582,13 @@ protected void handleGraphOperation(final Bytecode bytecode, final Graph graph,
.statusMessage(t.getMessage())
.statusAttributeException(t).create());
}
+
+ if (destroySessionPostGraphOp) {
+ // Destroy the session after a successful rollback due to error. Placed here rather than
+ // in a finally block since we don't want to end the session if no commit/rollback succeeded.
+ session.manualKill(true);
Review Comment:
Since this is in a catch block that is catching all error types, is it possible for `session.manualKill` to be called twice if the previous call in the try block threw an error?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3217 Add server option to close Session automatically. [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on code in PR #3284:
URL: https://github.com/apache/tinkerpop/pull/3284#discussion_r2591057607
##########
gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinSessionReuseTxIntegrateTest.java:
##########
@@ -0,0 +1,411 @@
+/*
+ * 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.server;
+
+import org.apache.tinkerpop.gremlin.server.op.session.SessionOpProcessor;
+import org.apache.tinkerpop.gremlin.util.ExceptionHelper;
+import org.apache.tinkerpop.gremlin.driver.Client;
+import org.apache.tinkerpop.gremlin.driver.Cluster;
+import org.apache.tinkerpop.gremlin.driver.RequestOptions;
+import org.apache.tinkerpop.gremlin.driver.remote.DriverRemoteConnection;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.junit.Ignore;
+import org.junit.Test;
+
+import java.io.File;
+import java.util.HashMap;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+
+import static org.apache.tinkerpop.gremlin.process.traversal.AnonymousTraversalSource.traversal;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+/**
+ * Integration tests for gremlin-driver and bytecode sessions.
+ *
+ * NOTE: this is effectively a copy of GremlinSessionTxIntegrateTest but the expectation is that the gremlin-driver
+ * will be configured in a way that allows connection re-use as the server allows it via the "destroySessionPostGraphOp"
+ * setting.
+ */
+public class GremlinSessionReuseTxIntegrateTest extends AbstractGremlinServerIntegrationTest {
+
+ /**
+ * Configure specific Gremlin Server settings for specific tests.
+ */
+ @Override
+ public Settings overrideSettings(final Settings settings) {
+ final String nameOfTest = name.getMethodName();
+
+ settings.graphs.put("graph", "conf/tinkertransactiongraph-empty.properties");
+
+ // Set this as it is what allows connection re-use on the server side.
+ settings.destroySessionPostGraphOp = true;
+
+ switch (nameOfTest) {
+ case "shouldExecuteBytecodeInSession":
+ break;
+ case "shouldTimeoutTxBytecode":
+ settings.processors.clear();
+
+ // OpProcessor setting
+ final Settings.ProcessorSettings processorSettings = new Settings.ProcessorSettings();
+ processorSettings.className = SessionOpProcessor.class.getCanonicalName();
+ processorSettings.config = new HashMap<>();
+ processorSettings.config.put(SessionOpProcessor.CONFIG_SESSION_TIMEOUT, 3000L);
+ settings.processors.add(processorSettings);
+
+ // Unified setting
+ settings.sessionLifetimeTimeout = 3000L;
+ break;
+ }
+
+ return settings;
+ }
+
+ @Test
+ @Ignore("TINKERPOP-2832")
+ public void shouldTimeoutTxBytecode() throws Exception {
+
+ final Cluster cluster = TestClientFactory.build().create();
+ final GraphTraversalSource g = traversal().withRemote(DriverRemoteConnection.using(cluster));
+
+ GraphTraversalSource gtx = g.tx().begin();
+ gtx.addV("person").addE("link").iterate();
+ gtx.tx().commit();
+
+ assertEquals(1L, g.V().count().next().longValue());
+ assertEquals(1L, g.E().count().next().longValue());
+
+ try {
+ gtx = g.tx().begin();
+
+ assertEquals(1L, gtx.V().count().next().longValue());
+ assertEquals(1L, gtx.E().count().next().longValue());
+
+ // wait long enough for the session to die
+ Thread.sleep(4000);
+
+ // the following should fail with a dead session
+ gtx.V().count().iterate();
+ fail("Session is dead - a new one should not reopen to server this request");
Review Comment:
```suggestion
fail("Session is dead - a new one should not reopen to serve this request");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3217 Add server option to close Session automatically. [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on PR #3284:
URL: https://github.com/apache/tinkerpop/pull/3284#issuecomment-3662487582
> Thank you, @kenhuuu, nice feature. Will port to our version too.
FYI, @andrii0lomakin there is no released Java driver/GLV that can take advantage of this. This will change in the next version of TinkerPop.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3217 Add server option to close Session automatically. [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3284:
URL: https://github.com/apache/tinkerpop/pull/3284#discussion_r2590451472
##########
gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinSessionReuseTxIntegrateTest.java:
##########
@@ -0,0 +1,411 @@
+/*
+ * 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.server;
+
+import org.apache.tinkerpop.gremlin.server.op.session.SessionOpProcessor;
+import org.apache.tinkerpop.gremlin.util.ExceptionHelper;
+import org.apache.tinkerpop.gremlin.driver.Client;
+import org.apache.tinkerpop.gremlin.driver.Cluster;
+import org.apache.tinkerpop.gremlin.driver.RequestOptions;
+import org.apache.tinkerpop.gremlin.driver.remote.DriverRemoteConnection;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.junit.Ignore;
+import org.junit.Test;
+
+import java.io.File;
Review Comment:
Unused import
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3217 Add server option to close Session automatically. [tinkerpop]
Posted by "andrii0lomakin (via GitHub)" <gi...@apache.org>.
andrii0lomakin commented on PR #3284:
URL: https://github.com/apache/tinkerpop/pull/3284#issuecomment-3640346137
Thank you, @kenhuuu, nice feature. Will port to our version too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3217 Add server option to close Session automatically. [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on PR #3284:
URL: https://github.com/apache/tinkerpop/pull/3284#issuecomment-3629050235
VOTE +1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3217 Add server option to close Session automatically. [tinkerpop]
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on code in PR #3284:
URL: https://github.com/apache/tinkerpop/pull/3284#discussion_r2582911740
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/session/SessionOpProcessor.java:
##########
@@ -571,6 +582,13 @@ protected void handleGraphOperation(final Bytecode bytecode, final Graph graph,
.statusMessage(t.getMessage())
.statusAttributeException(t).create());
}
+
+ if (destroySessionPostGraphOp) {
+ // Destroy the session after a successful rollback due to error. Placed here rather than
+ // in a finally block since we don't want to end the session if no commit/rollback succeeded.
+ session.manualKill(true);
Review Comment:
Can there ever be a case where there's an error and rollback isn't successful, but the session is killed here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3217 Add server option to close Session automatically. [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on code in PR #3284:
URL: https://github.com/apache/tinkerpop/pull/3284#discussion_r2591045899
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Settings.java:
##########
@@ -189,6 +189,14 @@ public Settings() {
*/
public boolean strictTransactionManagement = false;
+ /**
+ * If set to {@code true} the Gremlin Server will destroy the session when a GraphOp (commit or rollback) is
+ * successfully completed on that session.
+ *
+ * NOTE: Defaults to false in 3.7.6/3.8.1 to prevent breaking change.
Review Comment:
Nit: I generally prefer not to reference specific releases in javadocs as it quickly becomes outdated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3217 Add server option to close Session automatically. [tinkerpop]
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on code in PR #3284:
URL: https://github.com/apache/tinkerpop/pull/3284#discussion_r2582919673
##########
gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinSessionReuseTxIntegrateTest.java:
##########
@@ -0,0 +1,411 @@
+/*
+ * 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.server;
+
+import org.apache.tinkerpop.gremlin.server.op.session.SessionOpProcessor;
+import org.apache.tinkerpop.gremlin.util.ExceptionHelper;
+import org.apache.tinkerpop.gremlin.driver.Client;
+import org.apache.tinkerpop.gremlin.driver.Cluster;
+import org.apache.tinkerpop.gremlin.driver.RequestOptions;
+import org.apache.tinkerpop.gremlin.driver.remote.DriverRemoteConnection;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.junit.Ignore;
+import org.junit.Test;
+
+import java.io.File;
+import java.util.HashMap;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+
+import static org.apache.tinkerpop.gremlin.process.traversal.AnonymousTraversalSource.traversal;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+/**
+ * Integration tests for gremlin-driver and bytecode sessions.
+ *
+ * NOTE: this is effectively a copy of GremlinSessionTxIntegrateTest but the expectation is that the gremlin-driver
+ * will be configured in a way that allows connection re-use as the server allows it via the "destroySessionPostGraphOp"
+ * setting.
+ */
+public class GremlinSessionReuseTxIntegrateTest extends AbstractGremlinServerIntegrationTest {
+
+ /**
+ * Configure specific Gremlin Server settings for specific tests.
+ */
+ @Override
+ public Settings overrideSettings(final Settings settings) {
+ final String nameOfTest = name.getMethodName();
+
+ settings.graphs.put("graph", "conf/tinkertransactiongraph-empty.properties");
+
+ // Set this as it is what allows connection re-use on the server side.
+ settings.destroySessionPostGraphOp = true;
+
+ switch (nameOfTest) {
+ case "shouldExecuteBytecodeInSession":
+ break;
+ case "shouldTimeoutTxBytecode":
+ settings.processors.clear();
+
+ // OpProcessor setting
+ final Settings.ProcessorSettings processorSettings = new Settings.ProcessorSettings();
+ processorSettings.className = SessionOpProcessor.class.getCanonicalName();
+ processorSettings.config = new HashMap<>();
+ processorSettings.config.put(SessionOpProcessor.CONFIG_SESSION_TIMEOUT, 3000L);
Review Comment:
nit: make timeout a constant
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3217 Add server option to close Session automatically. [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on code in PR #3284:
URL: https://github.com/apache/tinkerpop/pull/3284#discussion_r2591055628
##########
gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinSessionReuseTxIntegrateTest.java:
##########
@@ -0,0 +1,411 @@
+/*
+ * 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.server;
+
+import org.apache.tinkerpop.gremlin.server.op.session.SessionOpProcessor;
+import org.apache.tinkerpop.gremlin.util.ExceptionHelper;
+import org.apache.tinkerpop.gremlin.driver.Client;
+import org.apache.tinkerpop.gremlin.driver.Cluster;
+import org.apache.tinkerpop.gremlin.driver.RequestOptions;
+import org.apache.tinkerpop.gremlin.driver.remote.DriverRemoteConnection;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.junit.Ignore;
+import org.junit.Test;
+
+import java.io.File;
+import java.util.HashMap;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+
+import static org.apache.tinkerpop.gremlin.process.traversal.AnonymousTraversalSource.traversal;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+/**
+ * Integration tests for gremlin-driver and bytecode sessions.
+ *
+ * NOTE: this is effectively a copy of GremlinSessionTxIntegrateTest but the expectation is that the gremlin-driver
+ * will be configured in a way that allows connection re-use as the server allows it via the "destroySessionPostGraphOp"
+ * setting.
+ */
+public class GremlinSessionReuseTxIntegrateTest extends AbstractGremlinServerIntegrationTest {
+
+ /**
+ * Configure specific Gremlin Server settings for specific tests.
+ */
+ @Override
+ public Settings overrideSettings(final Settings settings) {
+ final String nameOfTest = name.getMethodName();
+
+ settings.graphs.put("graph", "conf/tinkertransactiongraph-empty.properties");
+
+ // Set this as it is what allows connection re-use on the server side.
+ settings.destroySessionPostGraphOp = true;
+
+ switch (nameOfTest) {
+ case "shouldExecuteBytecodeInSession":
Review Comment:
Nit: don't need empty case for this test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3217 Add server option to close Session automatically. [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #3284:
URL: https://github.com/apache/tinkerpop/pull/3284#discussion_r2591608745
##########
gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinSessionReuseTxIntegrateTest.java:
##########
@@ -0,0 +1,411 @@
+/*
+ * 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.server;
+
+import org.apache.tinkerpop.gremlin.server.op.session.SessionOpProcessor;
+import org.apache.tinkerpop.gremlin.util.ExceptionHelper;
+import org.apache.tinkerpop.gremlin.driver.Client;
+import org.apache.tinkerpop.gremlin.driver.Cluster;
+import org.apache.tinkerpop.gremlin.driver.RequestOptions;
+import org.apache.tinkerpop.gremlin.driver.remote.DriverRemoteConnection;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
+import org.apache.tinkerpop.gremlin.structure.Transaction;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.junit.Ignore;
+import org.junit.Test;
+
+import java.io.File;
Review Comment:
Thanks. Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3217 Add server option to close Session automatically. [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #3284:
URL: https://github.com/apache/tinkerpop/pull/3284#discussion_r2591601303
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/session/SessionOpProcessor.java:
##########
@@ -571,6 +582,13 @@ protected void handleGraphOperation(final Bytecode bytecode, final Graph graph,
.statusMessage(t.getMessage())
.statusAttributeException(t).create());
}
+
+ if (destroySessionPostGraphOp) {
+ // Destroy the session after a successful rollback due to error. Placed here rather than
+ // in a finally block since we don't want to end the session if no commit/rollback succeeded.
+ session.manualKill(true);
Review Comment:
Thanks for pointing this out. I thought this case would have been covered by `onError` but it actually only runs if its transaction-managed. Addressed this by adding an extra check for `shouldManageTransactionsForRequest`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org