-
Notifications
You must be signed in to change notification settings - Fork 4.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
Rollback Bigtable throttling counter #32442
Conversation
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
b1a74c6
to
d751704
Compare
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
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 dont think you can remove methods that break binary compatibility. Instead I think you have to make them no-ops, change javadocs to say they are no-ops and log a warning
* | ||
* <p>Will also set {@link #withThrottlingReportTargetMs} to the same value. | ||
*/ | ||
public Write withThrottlingTargetMs(int throttlingTargetMs) { |
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.
can you actually remove a public method? I suspect that you need to make it a no-op that logs a warning
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 think I can and we should.
Only user actively set this method in their 2.59.0 pipeline will be affected. Users do not explicitly set this method, and upgrade from <=2.58.0 are unaffected. For those indeed use this method in 2.59.0 they have good reason to do so. We are removing a tested and functioning feature on request, make it no-op or warning then user won't aware this feature is gone in the next release.
In the past Beam has an "Experimental" annotation but voted to remove them (#26490). Now "new code is changeable/evolving by default" (see discussion link in that PR) especially true for a new API in single version not enabled by default.
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.
If the conceptual "binary compatibility" is a concern, I've add back the method, but throw instead of no-op or ignore there
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.
yes, I was concerned about breaking binary compatibility. I
I dont see it throwing an exception though, nor do I think it should throw an exception. Please make it a no-op and log a warning instead.
Also we didnt expose ThrottlingTargetMs nor latency based throttling for a reason, it has some really sharp edge cases that are very hard to debug. Please make this a no-op as it was prior to your change
* configurations (e.g. {@link #withFlowControl}, {@link #withThrottlingTargetMs} will adjust | ||
* the default value accordingly. Set to 0 to disable throttling time reporting. | ||
*/ | ||
public Write withThrottlingReportTargetMs(int throttlingReportTargetMs) { |
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.
same as above
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.
Please dont throw an exception, instead log a warning. We dont want to be in position where are user started using this flag in 2.59 and then their jobs start failing when they upgrade to 2.60. In other words throwing an UnsupportedOperationException is effectively the same as breaking binary compatibility
6c2ce5b
to
006b5af
Compare
* | ||
* <p>Will also set {@link #withThrottlingReportTargetMs} to the same value. | ||
*/ | ||
/** Returns a new {@link BigtableIO.Write} with client side latency based throttling enabled. */ |
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.
withThrottlingTargetMs
can be preserved. It was a config existed in BigtableWriteOptions but missing the correspondent in transform configuration. It was a inconsistency in BigtableIO configuration setting and not related to throttling counter change.
* DEADLINE_EXCEEDED is a common error code and was reported as unknown * BigtableWriteException originally printed the whole raw record, cloudlog get truncated and does not see stacktrace after it
006b5af
to
42e28b7
Compare
* | ||
* <p>Will also set {@link #withThrottlingReportTargetMs} to the same value. | ||
*/ | ||
public Write withThrottlingTargetMs(int throttlingTargetMs) { |
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.
yes, I was concerned about breaking binary compatibility. I
I dont see it throwing an exception though, nor do I think it should throw an exception. Please make it a no-op and log a warning instead.
Also we didnt expose ThrottlingTargetMs nor latency based throttling for a reason, it has some really sharp edge cases that are very hard to debug. Please make this a no-op as it was prior to your change
* configurations (e.g. {@link #withFlowControl}, {@link #withThrottlingTargetMs} will adjust | ||
* the default value accordingly. Set to 0 to disable throttling time reporting. | ||
*/ | ||
public Write withThrottlingReportTargetMs(int throttlingReportTargetMs) { |
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.
Please dont throw an exception, instead log a warning. We dont want to be in position where are user started using this flag in 2.59 and then their jobs start failing when they upgrade to 2.60. In other words throwing an UnsupportedOperationException is effectively the same as breaking binary compatibility
Thank you! |
* Revert BigtableIO change in apache#31924 * Exclude fixes not related to throttling counter change * issue warning for removed configs
Requested by Bigtable owner
Please add a meaningful description for your change here
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.