-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix(spanner): update PDML to take sessions from pool #2736
Merged
hengfengli
merged 4 commits into
googleapis:master
from
hengfengli:take-session-from-pool
Aug 24, 2020
Merged
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
0a6efd7
fix(spanner): update PDML and BatchROTxn to take sessions from pool
hengfengli 614865e
Update Cleanup and docs.
hengfengli 2412aaf
Revert changes for Batch API.
hengfengli bf602c5
Merge branch 'master' into take-session-from-pool
hengfengli File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is this session ever returned to the pool? Or is there a specific reason why that is not necessary in this case?
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.
BatchReadOnlyTransaction
returns a transaction. If we recycle the session inside this function, then the transaction would not work after returning, right?I thought
BatchReadOnlyTransaction.Close()
orBatchReadOnlyTransaction.Cleanup()
should recycle the session. Do you think I should add it to Close() or Cleanup()? It looks like .Close() is the right place.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.
I am a bit confused about the purpose of
Close()
andCleanup()
.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.
BatchReadOnlyTransaction
s are intended to be used across multiple clients. One client should create the transaction, and all other clients should reference this same transaction. Example:BatchReadOnlyTransaction
by callingClient.BatchReadOnlyTransaction(ctx context.Context, tb TimestampBound)
. This step used to create a new session, but should now take one from the session pool of Client 1.Client.BatchReadOnlyTransactionFromID(tid BatchReadOnlyTransactionID)
. This means that Client 2 will technically be referencing a session from the session pool of Client 1.BatchReadOnlyTransaction.Cleanup
, and that method should return the session to the pool.BatchReadOnlyTransaction.Close
. That does not really do anything other than marking the transaction as no longer in use by that client.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.
We should also update the comment on
Client.BatchReadOnlyTransaction
as it specifically states that the method creates a new session.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.
Thanks a lot for the detailed explanation! 💯 I also updated the doc in
Cleanup()
, which was confusing as well.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.
I'm starting to think that we might need to split the two changes in this PR, as the change for
BatchReadOnlyTransaction
has some more impact than we first thought. Prior to this change, the way to useBatchReadOnlyTransaction
was the following:BatchReadOnlyTransaction
on a client and pass the transaction id to any other client that should also use that transaction to read data.BatchReadOnlyTransaction.Close
.BatchReadOnlyTransaction.Cleanup
. Failing to do so will not really have any consequences.With the changes in this PR the behavior changes to:
BatchReadOnlyTransaction
on a client and pass the transaction id to any other client that should also use that transaction to read data.BatchReadOnlyTransaction.Close
, unless it is the client that created theBatchReadOnlyTransaction
.BatchReadOnlyTransaction.Cleanup
. Failing to do so will cause a session to leak from the pool of the client that created the transaction.I think the above might also be a good reason to reconsider whether we should make this change at all, and if we do, it should be a separate change so it can be clearly marked in the release notes as a (at least somewhat) breaking change.
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.
For 2, any client can call
BatchReadOnlyTransaction.Close
, which code looks like:google-cloud-go/spanner/batch.go
Lines 209 to 214 in 614865e
Why do you say ", unless it is the client that created the
BatchReadOnlyTransaction
"?For 3, "only the client that created the transaction must call
BatchReadOnlyTransaction.Cleanup
". I think all clients can callCleanup
, which is very similar to the previous code that I just replaced the DeleteSession request tosh.recycle()
.Yes, if it fails to call
Cleanup
, which would cause a session leak from the pool of the client that created the transaction.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.
I wonder how Java handles this issue then?
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.
You're right, all clients could call both
Cleanup
andClose
without any problems. The only real change here is that the client that created the transaction should callCleanup
, while that is optional in the current implementation.(The Java client has not been changed to use a session from the pool yet)
The Java client does not have separate
Close
andCleanup
methods, instead it only has aClose
method. TheClose
method works very much in the same way as theCleanup
method in Go and always deletes the session that is used, so it should only be called once all clients have finished reading. So for the Java client we will have a similar problem, as the current API allowsClose
to be called from any client, while if we choose to use a session from the pool it will change to a situation whereClose
should at least be also be called on the client that created the transaction.