-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat(spanner): support commit stats #3056
Conversation
@skuruppu @olavloite Could you please give me some advice on this change? Which one you think is better? Or, if you have any other suggestions? I appreciate. |
This is a quite difficult choice to make. My initial thoughts are:
Based on the above, and partly on the assumption that many users are currently ignoring the commit timestamp that is returned by
The signature of the two methods would then be:
@hengfengli @skuruppu |
I completely agree that if we were designing from scratch, we should just accept the request proto as input and set the return value to the response proto. I think we should consider this when supporting new methods just to avoid having to make this type of decision in the future. But since we have to make this decision, I'm quite worried about making a breaking change to these commonly used methods. I don't have any data but if people are reading the commit timestamp, then they are likely to be using it for some important reason so I would hesitate to break whatever that use case may be. So @thiagotnunes and I were actually discussing about making the Java change similar to what @hengfengli has proposed here where we introduce a I don't know if we need to go even further and create a struct for the return type with I know it's not the ideal solution we want but I think what @hengfengli has produced is the least invasive option. Especially given that users are only meant to use this new option to debug queries. I wouldn't want a debugging change to affect existing code running in production apps. |
@olavloite Thanks for the ideas. I was inspired by adding For the method |
I have created a more generic API change to support transaction options in #3058. Please review the code over there, because this PR will be based on it. |
344fb5d
to
0917238
Compare
@olavloite @skuruppu This PR is ready for review. Please take a look. |
if got, want := resp.CommitStats.OverloadDelay.Nanos, int32(1); got != want { | ||
t.Fatalf("Mismatch overload delay - got: %d, want: %d", got, want) | ||
} |
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.
@olavloite since @hengfengli is away, would you mind removing references to overload_delay
in this PR? I think you should be able to push commits to the same branch. But if not, feel free to fork the PR.
Replaced by #3444 |
Adds support for `CommitStats` to the Spanner client. Based on #3056.
Adds support for `CommitStats` to the Spanner client. Based on googleapis/google-cloud-go#3056.
Current approach
To not make a breaking change, I came up with the following changes (in this draft PR):
TransactionOptions
CommitOptions
as a fieldClient.ReadWriteTransactionWithOptions
andNewReadWriteStmtBasedTransactionWithOptions
andTransactionOptions
will be passed into these two functionsCommitOptions
andCommitResponse
ReadWriteTransactionWithOptions
andCommitReturnResponse
will returnCommitResponse
instead oftime.Time
Alternative approach
Another option is to update the struct
ReadWriteTransaction
:Then, update
Client.ReadWriteTransaction
andReadWriteStmtBasedTransaction.Commit
to return (CommitResponse, error).Basically, the
ReturnCommitStats
option can be set on a transaction struct inside the passing function:Of course, this would be a breaking change for users.