TINKERPOP-3213 Added a new SessionedChildClient which reuses connection from parent … #3258
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 3.7-dev #3258 +/- ##
=============================================
+ Coverage 76.14% 76.44% +0.29%
- Complexity 13152 13300 +148
=============================================
Files 1084 1092 +8
Lines 65160 67069 +1909
Branches 7285 7381 +96
=============================================
+ Hits 49616 51270 +1654
- Misses 12839 13051 +212
- Partials 2705 2748 +43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…Client instead of creating new one
There was a problem hiding this comment.
I don't have any major concerns with the changes here. It appears that the existing tests in GremlinServerSessionIntegrateTest are giving reasonable coverage to the new SessionedChildClient. Would you mind elaborating a bit more in the PR description as to the exact problem you're aiming to solve? Is it simply to reduce connection creation overhead in use cases which involve many short lived sessions?
| } | ||
|
|
||
| @Override | ||
| protected synchronized Connection chooseConnection(RequestMessage msg) throws TimeoutException, ConnectionException { |
There was a problem hiding this comment.
Nit:
| protected synchronized Connection chooseConnection(RequestMessage msg) throws TimeoutException, ConnectionException { | |
| protected synchronized Connection chooseConnection(final RequestMessage msg) throws TimeoutException, ConnectionException { |
There was a problem hiding this comment.
Yes that's the overhead we are trying to reduce. I have updated the PR description as well.
kenhuuu
changed the title
| * bindings across requests. | ||
| * Transaction are not automatically committed. It is up the client to issue commit/rollback commands. | ||
| */ | ||
| public final static class SessionedChildClient extends Client { |
There was a problem hiding this comment.
It would have been best if there was a solution that kept this class package-private, as it now differs from the other Clients that are only supposed to be exposed via Cluster. It's fine for this PR, but I'd probably recommend adding a Jira to look into this again in the future after this is merged.
|
|
||
| public SessionedChildClient(final Client parentClient, String sessionId) { | ||
| super(parentClient.cluster, parentClient.settings); | ||
| this.parentClient = parentClient; |
There was a problem hiding this comment.
Does it make sense for any type of Client to be the parent? SessionedClient only allows for one connection so maybe there should be a check here that disallows that particular client?
| this.maintainStateAfterException = parentClient.settings.getSession().map(s -> s.maintainStateAfterException).orElse(false); | ||
| } | ||
|
|
||
| public String getSessionId() { |
There was a problem hiding this comment.
This method is currently unused which may be a potential problem. There is specialized handling for sessions at https://github.com/apache/tinkerpop/blob/3.7-dev/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Connection.java#L313 as well as https://github.com/apache/tinkerpop/blob/3.7-dev/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/DriverRemoteConnection.java#L235
4 participants
This PR aims to reduce the overhead of creating a new connection and closing it for every transaction using
SessionedClientinDriverRemoteConnection. Instead the existing Client object inDriverRemoteConnectionis tied to the newSessionedChildClientas a parent, and thisSessionedChildClientborrows the connection from the parent Client's pool for transactions instead of creating a newConnection.https://issues.apache.org/jira/browse/TINKERPOP-3213