igorbernstein2 commented on code in PR #32442:
URL: https://github.com/apache/beam/pull/32442#discussion_r1765320721
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -1116,51 +1110,12 @@ public Write withMaxOutstandingBytes(long bytes) {
* always enabled on batch writes and limits the number of outstanding
requests to the Bigtable
* server.
*
- * <p>When enabled, will also set default {@link
#withThrottlingReportTargetMs} to 1 minute.
- * This enables runner react with increased latency in flush call due to
flow control.
- *
* <p>Does not modify this object.
*/
public Write withFlowControl(boolean enableFlowControl) {
- BigtableWriteOptions options = getBigtableWriteOptions();
- BigtableWriteOptions.Builder builder =
options.toBuilder().setFlowControl(enableFlowControl);
- if (enableFlowControl) {
- builder = builder.setThrottlingReportTargetMs(60_000);
- }
- return toBuilder().setBigtableWriteOptions(builder.build()).build();
- }
-
- /**
- * Returns a new {@link BigtableIO.Write} with client side latency based
throttling enabled.
- *
- * <p>Will also set {@link #withThrottlingReportTargetMs} to the same
value.
- */
- public Write withThrottlingTargetMs(int throttlingTargetMs) {
Review Comment:
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
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -1116,51 +1110,12 @@ public Write withMaxOutstandingBytes(long bytes) {
* always enabled on batch writes and limits the number of outstanding
requests to the Bigtable
* server.
*
- * <p>When enabled, will also set default {@link
#withThrottlingReportTargetMs} to 1 minute.
- * This enables runner react with increased latency in flush call due to
flow control.
- *
* <p>Does not modify this object.
*/
public Write withFlowControl(boolean enableFlowControl) {
- BigtableWriteOptions options = getBigtableWriteOptions();
- BigtableWriteOptions.Builder builder =
options.toBuilder().setFlowControl(enableFlowControl);
- if (enableFlowControl) {
- builder = builder.setThrottlingReportTargetMs(60_000);
- }
- return toBuilder().setBigtableWriteOptions(builder.build()).build();
- }
-
- /**
- * Returns a new {@link BigtableIO.Write} with client side latency based
throttling enabled.
- *
- * <p>Will also set {@link #withThrottlingReportTargetMs} to the same
value.
- */
- public Write withThrottlingTargetMs(int throttlingTargetMs) {
- BigtableWriteOptions options = getBigtableWriteOptions();
- return toBuilder()
- .setBigtableWriteOptions(
- options
- .toBuilder()
- .setThrottlingTargetMs(throttlingTargetMs)
- .setThrottlingReportTargetMs(throttlingTargetMs)
- .build())
- .build();
- }
-
- /**
- * Returns a new {@link BigtableIO.Write} with throttling time reporting
enabled. When write
- * request latency exceeded the set value, the amount greater than the
target will be considered
- * as throttling time and report back to runner.
- *
- * <p>If not set, defaults to 3 min for completed batch request. Client
side flowing control
- * 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) {
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
