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
SpannerIO: support max commit delay #31008
base: master
Are you sure you want to change the base?
Conversation
R: @damccorm |
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
assign set of reviewers |
@nielm hey, sorry for tagging you explicitly here but I think I have accidentally added a wrong reviewer to this PR. Could you please help me assign the right people here? |
Effectively we already have this at the RPC level /** Specifies the commit deadline. This is overridden if the CommitRetrySettings is specified. */
public SpannerConfig withCommitDeadline(Duration commitDeadline) {
return withCommitDeadline(ValueProvider.StaticValueProvider.of(commitDeadline));
}
/** Specifies the commit retry settings. Setting this overrides the commit deadline. */
public SpannerConfig withCommitRetrySettings(RetrySettings commitRetrySettings) {
return toBuilder().setCommitRetrySettings(commitRetrySettings).build();
} Both of these allow the deadline for a commit to complete to be set (The default is 15s). Setting the commit deadline to significantly longer increases the risk of Spanner going into overload and pushing back against writes, which will reduce the throughput of the pipeline. |
@nielm oh, I believe that is a completely different feature, this PR is about throughput optimized writes: documentation link. It was only released on March 26th, a couple of weeks ago: release notes link |
@nielm did you have a chance to look at this again? |
The setting of the commit deadline timeout predates the per-transactiom commit delay feature, but it has the same effect - limiting the duration that the spanner server can spend processing a commit. The difference is that the commit delay can be applied to different transactions, while the commit deadline is global for a spanner client. However in beam it is not possible to set the commit delay per transaction, so it is also global. There is no benefit to adding the commit delay to beam, when the commit deadline already exists. And the commit deadline will act as a maximum commit delay. |
@nielm I am sorry but this is a completely different feature. Please read the documentation link I am referring to. It was released a couple of weeks ago and we are already using it in production (we had to fork Setting commit deadline does not affect performance of the writes at all, it just tells the client library how much time it can wait and retry the transaction, whereas setting a maximum commit delay may improve performance significantly if your app doesn't care much about the exact commit timing. Documentation quote:
|
@johnjcasey may I ask you to help? I think we got a little confused here. |
My apologies - I am indeed confusing it with the commit timeout delay... Two comments on the PR
|
d0efd2e
to
74f014e
Compare
Ah, sure. No idea why I updated
I think by default we should just leave it out (as I implemented), because there is already a documented default behavior for this case:
|
There is a test failure, but I think it is unrelated to this PR:
|
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.
LGTM
R: @chamikaramj for IO |
Addresses #31007
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123
), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>
instead.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.