-
Notifications
You must be signed in to change notification settings - Fork 841
TINKERPOP-3213 Added a new SessionedChildClient which reuses connection from parent … #3258
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 3.7-dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -773,6 +773,80 @@ public Client alias(final Map<String, String> aliases) { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * A {@code Client} implementation that operates in the context of a session. {@code ChildSessionClient} is tied to | ||||||
| * another client as a child, it borrows the connection from the parent client's pool for the transaction. Requests are | ||||||
| * sent to a single server, where each request is bound to the same thread and same connection with the same set of | ||||||
| * 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 { | ||||||
| private final String sessionId; | ||||||
| private final boolean manageTransactions; | ||||||
| private final boolean maintainStateAfterException; | ||||||
| private final Client parentClient; | ||||||
| private Connection borrowedConnection; | ||||||
| private boolean closed; | ||||||
|
|
||||||
| public SessionedChildClient(final Client parentClient, String sessionId) { | ||||||
| super(parentClient.cluster, parentClient.settings); | ||||||
| this.parentClient = parentClient; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.sessionId = sessionId; | ||||||
| this.closed = false; | ||||||
| this.manageTransactions = parentClient.settings.getSession().map(s -> s.manageTransactions).orElse(false); | ||||||
| this.maintainStateAfterException = parentClient.settings.getSession().map(s -> s.maintainStateAfterException).orElse(false); | ||||||
| } | ||||||
|
|
||||||
| public String getSessionId() { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||
| return sessionId; | ||||||
| } | ||||||
|
|
||||||
| @Override | ||||||
| public RequestMessage.Builder buildMessage(final RequestMessage.Builder builder) { | ||||||
| builder.processor("session"); | ||||||
| builder.addArg(Tokens.ARGS_SESSION, sessionId); | ||||||
| builder.addArg(Tokens.ARGS_MANAGE_TRANSACTION, manageTransactions); | ||||||
| builder.addArg(Tokens.ARGS_MAINTAIN_STATE_AFTER_EXCEPTION, maintainStateAfterException); | ||||||
| return builder; | ||||||
| } | ||||||
|
|
||||||
| @Override | ||||||
| protected void initializeImplementation() { | ||||||
| // do nothing, parentClient is already initialized | ||||||
| } | ||||||
|
|
||||||
| @Override | ||||||
| protected synchronized Connection chooseConnection(RequestMessage msg) throws TimeoutException, ConnectionException { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit:
Suggested change
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes that's the overhead we are trying to reduce. I have updated the PR description as well. |
||||||
| if (borrowedConnection == null) { | ||||||
| //Borrow from parentClient's pool instead of creating new connection | ||||||
| borrowedConnection = parentClient.chooseConnection(msg); | ||||||
| } | ||||||
| //Increment everytime, the connection is chosen, all these will be decremented when transaction is commited/rolledback | ||||||
| borrowedConnection.borrowed.incrementAndGet(); | ||||||
| return borrowedConnection; | ||||||
| } | ||||||
|
|
||||||
| @Override | ||||||
| public synchronized CompletableFuture<Void> closeAsync() { | ||||||
| if (borrowedConnection != null && !borrowedConnection.isDead()) { | ||||||
|
|
||||||
| //Decrement one last time which was incremented by parentClient when the connection is borrowed initially | ||||||
| borrowedConnection.borrowed.decrementAndGet(); | ||||||
| borrowedConnection.returnToPool(); | ||||||
|
|
||||||
| borrowedConnection = null; | ||||||
| } | ||||||
| closed = true; | ||||||
| return CompletableFuture.completedFuture(null); | ||||||
| } | ||||||
|
|
||||||
| @Override | ||||||
| public boolean isClosing() { | ||||||
| return parentClient.isClosing() || closed; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
| /** | ||||||
| * A {@code Client} implementation that operates in the context of a session. Requests are sent to a single | ||||||
| * server, where each request is bound to the same thread with the same set of bindings across requests. | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.