You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/06/02 22:24:59 UTC
[GitHub] [beam] diegomez17 opened a new pull request, #17823: [BEAM-14554] Create setting to report Bigtable client throttling time to Dataflow
diegomez17 opened a new pull request, #17823:
URL: https://github.com/apache/beam/pull/17823
Create a setting inside of BigtableConfig to report external system throttle time to Dataflow. This setting will inform Dataflow that the client is throttling and Dataflow will either maintain or decrease workers.
This should help with overloading issues on the Bigtable side.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] diegomez17 commented on a diff in pull request #17823: [BEAM-14554] Create setting to report Bigtable client throttling time to Dataflow
Posted by GitBox <gi...@apache.org>.
diegomez17 commented on code in PR #17823:
URL: https://github.com/apache/beam/pull/17823#discussion_r917069156
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -1431,4 +1484,30 @@ static void validateTableExists(BigtableConfig config, PipelineOptions options)
}
}
}
+
+ interface ResourceStatsSupplier {
+ ResourceLimiterStats getStats();
+ }
+
+ private static class ResourceStatsSupplierImpl implements ResourceStatsSupplier, Serializable {
+
+ private final BigtableConfig bigtableConfig;
+
+ ResourceStatsSupplierImpl(BigtableConfig bigtableConfig) {
+ this.bigtableConfig = bigtableConfig;
+ }
+
+ @Override
+ public ResourceLimiterStats getStats() {
Review Comment:
Due to extending the interface that is inherently public, the method has to be too.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] diegomez17 commented on a diff in pull request #17823: [BEAM-14554] Create setting to report Bigtable client throttling time to Dataflow
Posted by GitBox <gi...@apache.org>.
diegomez17 commented on code in PR #17823:
URL: https://github.com/apache/beam/pull/17823#discussion_r916889261
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableConfig.java:
##########
@@ -97,6 +102,12 @@ abstract Builder setBigtableOptionsConfigurator(
abstract Builder setEmulatorHost(String emulatorHost);
+ /*
+ This feature is experimental and may be changed and relocated in the future
+ */
Review Comment:
Please look at comment now, don't know if I should be linking a method or class.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #17823: [BEAM-14554] Create setting to report Bigtable client throttling time to Dataflow
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #17823:
URL: https://github.com/apache/beam/pull/17823#issuecomment-1585650376
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] igorbernstein2 commented on a diff in pull request #17823: [BEAM-14554] Create setting to report Bigtable client throttling time to Dataflow
Posted by GitBox <gi...@apache.org>.
igorbernstein2 commented on code in PR #17823:
URL: https://github.com/apache/beam/pull/17823#discussion_r899279150
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -844,6 +874,22 @@ public void processElement(ProcessContext c, BoundedWindow window) throws Except
failures.add(new BigtableWriteException(c.element(), exception));
}
});
+ if (config.getDataflowThrottleReporting()) {
+ long delta = 0;
+ ResourceLimiterStats stats =
+ ResourceLimiterStats.getInstance(
+ new BigtableInstanceName(
+ config.getProjectId().get(), config.getInstanceId().get()));
Review Comment:
Please cache this in the constructor
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -739,6 +754,16 @@ public Write withEmulator(String emulatorHost) {
return toBuilder().setBigtableConfig(config.withEmulator(emulatorHost)).build();
}
+ /* This is an experimental feature that may get changed in the future
+ *
+ * Returns a new {@link BigtableIO.Write} that will report amount of time throttling to Dataflow
+ */
+ @Experimental
+ public Write withDataflowThrottleReporting() {
Review Comment:
I would add a bool param here. It will allow you to transition this to be opt out later
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] igorbernstein2 commented on a diff in pull request #17823: [BEAM-14554] Create setting to report Bigtable client throttling time to Dataflow
Posted by GitBox <gi...@apache.org>.
igorbernstein2 commented on code in PR #17823:
URL: https://github.com/apache/beam/pull/17823#discussion_r911422373
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableConfig.java:
##########
@@ -97,6 +102,12 @@ abstract Builder setBigtableOptionsConfigurator(
abstract Builder setEmulatorHost(String emulatorHost);
+ /*
+ This feature is experimental and may be changed and relocated in the future
+ */
Review Comment:
You don't need Experimental here since this is a package protected class. But it could use a bit more documentation explaining what it does. At the very least a reference to the docs in BigtableIO
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -813,12 +839,34 @@ public String toString() {
}
}
- private static class BigtableWriterFn
+ @VisibleForTesting
+ static class BigtableWriterFn
extends DoFn<KV<ByteString, Iterable<Mutation>>, BigtableWriteResult> {
- BigtableWriterFn(BigtableConfig bigtableConfig) {
+ protected final Counter cumulativeThrottlingMilliseconds =
+ Metrics.counter(BigtableWriterFn.class, "throttling-msecs");
+ private static Object metricLock = new Object();
+ private static long lastAggregatedThrottleTime = 0;
+ private ResourceStatsSupplier statsSupplier;
+
+ BigtableWriterFn(BigtableConfig bigtableConfig, ResourceStatsSupplier statsSupplier) {
this.config = bigtableConfig;
this.failures = new ConcurrentLinkedQueue<>();
+ this.statsSupplier = statsSupplier;
+ }
+
+ public static BigtableWriterFn create(BigtableConfig bigtableConfig) {
+ if (bigtableConfig.getProjectId() == null || bigtableConfig.getInstanceId() == null) {
+ return new BigtableWriterFn(
+ bigtableConfig,
+ new ResourceStatsSupplierImpl(
+ bigtableConfig.getBigtableOptions().getProjectId(),
+ bigtableConfig.getBigtableOptions().getInstanceId()));
Review Comment:
I think this breaks templates.
There are 2 phases in a dataflow job: graph construction and execution. During construction, all of the DoFns are instantiated, but none of the ValueProviders are populated. Then during execution all of the ValueProviders are populated and DoFns are executed.
Here you are trying to resolve ValueProviders during construction time, which will break the template mechanism.
you can do one of 2 things:
1. pass value provider for the instance name (the valueprovider will take BigtableConfig as the input)
2. Pass the BigtableConfig and resolve the instance name later
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIOTest.java:
##########
@@ -1935,3 +1965,21 @@ abstract static class Builder {
}
}
}
+
+class FakeResourceStatsSupplier implements BigtableIO.ResourceStatsSupplier, Serializable {
Review Comment:
please keep these as private static classes inside the test case class
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIOTest.java:
##########
@@ -1935,3 +1965,21 @@ abstract static class Builder {
}
}
}
+
+class FakeResourceStatsSupplier implements BigtableIO.ResourceStatsSupplier, Serializable {
Review Comment:
Also to make it easier to track down where magic numbers are coming from I would add a ctor that takes the fake cumulativeThrottlingTimeNanos and passes it down into FakeResourceStats. This way at the call site you can see where the numbers are coming from
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -813,12 +839,34 @@ public String toString() {
}
}
- private static class BigtableWriterFn
+ @VisibleForTesting
+ static class BigtableWriterFn
extends DoFn<KV<ByteString, Iterable<Mutation>>, BigtableWriteResult> {
- BigtableWriterFn(BigtableConfig bigtableConfig) {
+ protected final Counter cumulativeThrottlingMilliseconds =
+ Metrics.counter(BigtableWriterFn.class, "throttling-msecs");
+ private static Object metricLock = new Object();
+ private static long lastAggregatedThrottleTime = 0;
+ private ResourceStatsSupplier statsSupplier;
Review Comment:
final?
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -1431,4 +1492,24 @@ static void validateTableExists(BigtableConfig config, PipelineOptions options)
}
}
}
+
+ interface ResourceStatsSupplier {
+ ResourceLimiterStats getStats();
+ }
+
+ static class ResourceStatsSupplierImpl implements ResourceStatsSupplier, Serializable {
Review Comment:
should this be private?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] diegomez17 commented on a diff in pull request #17823: [BEAM-14554] Create setting to report Bigtable client throttling time to Dataflow
Posted by GitBox <gi...@apache.org>.
diegomez17 commented on code in PR #17823:
URL: https://github.com/apache/beam/pull/17823#discussion_r895925032
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -844,6 +879,22 @@ public void processElement(ProcessContext c, BoundedWindow window) throws Except
failures.add(new BigtableWriteException(c.element(), exception));
}
});
+ if (config.getBulkMutationThrottling()) {
+ long delta = 0;
Review Comment:
Moved it and changed naming
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] mutianf commented on a diff in pull request #17823: [BEAM-14554] Create setting to report Bigtable client throttling time to Dataflow
Posted by GitBox <gi...@apache.org>.
mutianf commented on code in PR #17823:
URL: https://github.com/apache/beam/pull/17823#discussion_r894701017
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -844,6 +879,21 @@ public void processElement(ProcessContext c, BoundedWindow window) throws Except
failures.add(new BigtableWriteException(c.element(), exception));
}
});
+ if (config.getBulkMutationThrottling()) {
+ long newAggregatedThrottleTime =
+ TimeUnit.NANOSECONDS.toMillis(
+ ResourceLimiterStats.getInstance(
+ new BigtableInstanceName(
+ config.getProjectId().get(), config.getInstanceId().get()))
+ .getCumulativeThrottlingTimeNanos());
+ long lastAggregatedThrottleTimeBeforeUpdate = lastAggregatedThrottleTime.get();
Review Comment:
update this seciton to use a lock
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] mutianf commented on a diff in pull request #17823: [BEAM-14554] Create setting to report Bigtable client throttling time to Dataflow
Posted by GitBox <gi...@apache.org>.
mutianf commented on code in PR #17823:
URL: https://github.com/apache/beam/pull/17823#discussion_r917007399
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -1431,4 +1484,30 @@ static void validateTableExists(BigtableConfig config, PipelineOptions options)
}
}
}
+
+ interface ResourceStatsSupplier {
+ ResourceLimiterStats getStats();
+ }
+
+ private static class ResourceStatsSupplierImpl implements ResourceStatsSupplier, Serializable {
+
+ private final BigtableConfig bigtableConfig;
+
+ ResourceStatsSupplierImpl(BigtableConfig bigtableConfig) {
+ this.bigtableConfig = bigtableConfig;
+ }
+
+ @Override
+ public ResourceLimiterStats getStats() {
Review Comment:
does this need to be public?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #17823: [BEAM-14554] Create setting to report Bigtable client throttling time to Dataflow
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #17823:
URL: https://github.com/apache/beam/pull/17823#issuecomment-1596130948
This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] mutianf commented on a diff in pull request #17823: [BEAM-14554] Create setting to report Bigtable client throttling time to Dataflow
Posted by GitBox <gi...@apache.org>.
mutianf commented on code in PR #17823:
URL: https://github.com/apache/beam/pull/17823#discussion_r890484611
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableConfig.java:
##########
@@ -97,6 +100,8 @@ abstract Builder setBigtableOptionsConfigurator(
abstract Builder setEmulatorHost(String emulatorHost);
+ abstract Builder setBulkMutationDataflowThrottling(boolean isEnabled);
Review Comment:
I think the name is a bit too long, maybe change it to `setBulkMutationThrottling`.
Also add `@Experimental` flag, and add a comment about this is the experimental feature
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableConfig.java:
##########
@@ -144,6 +149,11 @@ BigtableConfig withEmulator(String emulatorHost) {
return toBuilder().setEmulatorHost(emulatorHost).build();
}
+ @VisibleForTesting
+ BigtableConfig withBulkMutationDataflowThrottling(boolean isEnabled) {
Review Comment:
withBulkMutationThrottling and `@Experimental`
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableConfig.java:
##########
@@ -69,6 +69,9 @@ abstract class BigtableConfig implements Serializable {
/** Bigtable emulator. Used only for testing. */
abstract @Nullable String getEmulatorHost();
+ /** Reporting throttle time to Dataflow flag. */
+ abstract boolean getBulkMutationDataflowThrottling();
Review Comment:
I think the name is a bit too long, maybe change it to `getBulkMutationThrottling` and add an `@Experimental` flag and a comment saying it's experimental feature.
And also comment should be something like: whether throttling time is reported to dataflow jobs
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIOTest.java:
##########
@@ -324,7 +333,11 @@ public void testReadWithReaderAdvanceFailed() throws IOException {
FailureBigtableService failureService =
new FailureBigtableService(FailureOptions.builder().setFailAtAdvance(true).build());
BigtableConfig failureConfig =
- BigtableConfig.builder().setValidate(true).setBigtableService(failureService).build();
+ BigtableConfig.builder()
+ .setValidate(true)
+ .setBulkMutationDataflowThrottling(true)
Review Comment:
Should this be false?
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIOTest.java:
##########
@@ -796,7 +809,11 @@ public void testReadingWithSplitFailed() throws Exception {
FailureBigtableService failureService =
new FailureBigtableService(FailureOptions.builder().setFailAtSplit(true).build());
BigtableConfig failureConfig =
- BigtableConfig.builder().setValidate(true).setBigtableService(failureService).build();
+ BigtableConfig.builder()
+ .setValidate(true)
+ .setBulkMutationDataflowThrottling(true)
Review Comment:
False here too
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -563,13 +573,23 @@ public final String toString() {
return getBigtableConfig().getBigtableOptions();
}
+ /**
+ * Returns whether client's throttle time is being passed to Dataflow for bulk mutations
+ *
+ * <p>This change is experimental and may be changed and relocated in the future
+ */
+ public boolean isBulkMutationDataflowThrottlingEnabled() {
Review Comment:
`@Experimental`
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIOTest.java:
##########
@@ -302,7 +307,11 @@ public void testReadWithReaderStartFailed() throws IOException {
FailureBigtableService failureService =
new FailureBigtableService(FailureOptions.builder().setFailAtStart(true).build());
BigtableConfig failureConfig =
- BigtableConfig.builder().setValidate(true).setBigtableService(failureService).build();
+ BigtableConfig.builder()
+ .setValidate(true)
+ .setBulkMutationDataflowThrottling(true)
Review Comment:
Should this be false?
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -739,6 +759,15 @@ public Write withEmulator(String emulatorHost) {
return toBuilder().setBigtableConfig(config.withEmulator(emulatorHost)).build();
}
+ /* This is an experimental feature that may get changed in the future
+ *
+ * Returns a new {@link BigtableIO.Write} that will report amount of time throttling to Dataflow
+ */
+ public Write withBulkMutationDataflowThrottling() {
Review Comment:
`@Experimental`
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] mutianf commented on a diff in pull request #17823: [BEAM-14554] Create setting to report Bigtable client throttling time to Dataflow
Posted by GitBox <gi...@apache.org>.
mutianf commented on code in PR #17823:
URL: https://github.com/apache/beam/pull/17823#discussion_r894785971
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -844,6 +879,22 @@ public void processElement(ProcessContext c, BoundedWindow window) throws Except
failures.add(new BigtableWriteException(c.element(), exception));
}
});
+ if (config.getBulkMutationThrottling()) {
+ long delta = 0;
Review Comment:
you can probably get the instance outside of the lock:
ResourceLimiterStats stats = ResourcesLimiterStats.getInstance(..);
and inside the lock:
long newAgg = stats.getCumulativeThrottlingTimeNanos();
delta = ...
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #17823: [BEAM-14554] Create setting to report Bigtable client throttling time to Dataflow
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #17823:
URL: https://github.com/apache/beam/pull/17823#issuecomment-1145403173
Can one of the admins verify this patch?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #17823: [BEAM-14554] Create setting to report Bigtable client throttling time to Dataflow
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #17823:
URL: https://github.com/apache/beam/pull/17823#issuecomment-1145403177
Can one of the admins verify this patch?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] mutianf commented on a diff in pull request #17823: [BEAM-14554] Create setting to report Bigtable client throttling time to Dataflow
Posted by GitBox <gi...@apache.org>.
mutianf commented on code in PR #17823:
URL: https://github.com/apache/beam/pull/17823#discussion_r896955222
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -241,7 +246,11 @@ public abstract static class Read extends PTransform<PBegin, PCollection<Row>> {
static Read create() {
BigtableConfig config =
- BigtableConfig.builder().setTableId(StaticValueProvider.of("")).setValidate(true).build();
+ BigtableConfig.builder()
+ .setTableId(StaticValueProvider.of(""))
+ .setValidate(true)
+ .setDataflowThrottleReporting(false)
Review Comment:
I think you don't need to set it here again since you're already setting it in BigtableConfig line 80
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -563,13 +572,24 @@ public final String toString() {
return getBigtableConfig().getBigtableOptions();
}
+ /**
+ * Returns whether client's throttle time is being passed to Dataflow for bulk mutations
+ *
+ * <p>This change is experimental and may be changed and relocated in the future
+ */
+ @Experimental
+ public boolean isDataflowThrottleReportingEnabled() {
+ return getBigtableConfig().getDataflowThrottleReporting();
+ }
+
abstract Builder toBuilder();
static Write create() {
BigtableConfig config =
BigtableConfig.builder()
.setTableId(StaticValueProvider.of(""))
.setValidate(true)
+ .setDataflowThrottleReporting(false)
Review Comment:
Same here
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] closed pull request #17823: [BEAM-14554] Create setting to report Bigtable client throttling time to Dataflow
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #17823: [BEAM-14554] Create setting to report Bigtable client throttling time to Dataflow
URL: https://github.com/apache/beam/pull/17823
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] mutianf commented on a diff in pull request #17823: [BEAM-14554] Create setting to report Bigtable client throttling time to Dataflow
Posted by GitBox <gi...@apache.org>.
mutianf commented on code in PR #17823:
URL: https://github.com/apache/beam/pull/17823#discussion_r914967244
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigtable/FakeResourceStats.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.io.gcp.bigtable;
+
+import com.google.cloud.bigtable.grpc.async.ResourceLimiterStats;
+import java.io.Serializable;
+
+public class FakeResourceStats extends ResourceLimiterStats implements Serializable {
Review Comment:
Will it work if you move FakeResourceStats and FakeResourceStatsSupplier to BigtableIOTest?
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -1431,4 +1492,25 @@ static void validateTableExists(BigtableConfig config, PipelineOptions options)
}
}
}
+
+ interface ResourceStatsSupplier extends Serializable {
+ ResourceLimiterStats getStats();
+ }
+
+ static class ResourceStatsSupplierImpl implements ResourceStatsSupplier, Serializable {
+
+ private final String project;
+ private final String instance;
+
+ ResourceStatsSupplierImpl(String project, String instance) {
+
Review Comment:
extra line
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIOTest.java:
##########
@@ -1563,6 +1565,33 @@ public void testWriteWithBigTableOptionsSetsBulkOptionsAndRetryOptions() {
assertThat(options.getRetryOptions(), Matchers.equalTo(retryOptionsBuilder.build()));
}
+ @Test
+ public void testWriteWithEnabledDataflowThrottleReporting() {
+ BigtableIO.Write write =
+ defaultWrite.withTableId("TEST-TABLE").withDataflowThrottleReporting(true);
+
+ assertTrue(write.isDataflowThrottleReportingEnabled());
+ }
+
+ @Test
+ public void testWritePipelineWithDataflowThrottleReporting() {
+ final String table = "TEST-TABLE";
+ service.createTable(table);
+
+ BigtableConfig mockConfig =
Review Comment:
I think you can rename this to fakeConfig so it's consistent with your other naming
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -1431,4 +1492,25 @@ static void validateTableExists(BigtableConfig config, PipelineOptions options)
}
}
}
+
+ interface ResourceStatsSupplier extends Serializable {
Review Comment:
would it work if this doesn't extend Serializable?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] igorbernstein2 commented on a diff in pull request #17823: [BEAM-14554] Create setting to report Bigtable client throttling time to Dataflow
Posted by GitBox <gi...@apache.org>.
igorbernstein2 commented on code in PR #17823:
URL: https://github.com/apache/beam/pull/17823#discussion_r918030980
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableConfig.java:
##########
@@ -97,6 +102,14 @@ abstract Builder setBigtableOptionsConfigurator(
abstract Builder setEmulatorHost(String emulatorHost);
+ /**
+ * This feature is used to report back the client's throttling time to a Dataflow job. This will
+ * reduce the number of workers during a Dataflow bulk mutation job.
+ *
+ * <p>{@link BigtableIO.Write}
+ */
+ abstract Builder setDataflowThrottleReporting(boolean isEnabled);
Review Comment:
Please be consistent: either mark all of the new package private members as Experimental or none of them
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -1431,4 +1484,30 @@ static void validateTableExists(BigtableConfig config, PipelineOptions options)
}
}
}
+
+ interface ResourceStatsSupplier {
+ ResourceLimiterStats getStats();
+ }
+
+ private static class ResourceStatsSupplierImpl implements ResourceStatsSupplier, Serializable {
+
+ private final BigtableConfig bigtableConfig;
+
+ ResourceStatsSupplierImpl(BigtableConfig bigtableConfig) {
+ this.bigtableConfig = bigtableConfig;
+ }
+
+ @Override
+ public ResourceLimiterStats getStats() {
+ if (bigtableConfig.getProjectId() == null || bigtableConfig.getInstanceId() == null) {
+ return ResourceLimiterStats.getInstance(
+ new BigtableInstanceName(
+ bigtableConfig.getBigtableOptions().getProjectId(),
+ bigtableConfig.getBigtableOptions().getInstanceId()));
+ }
+ return ResourceLimiterStats.getInstance(
+ new BigtableInstanceName(
+ bigtableConfig.getProjectId().get(), bigtableConfig.getInstanceId().get()));
+ }
+ }
Review Comment:
I think it would more clear to factor out the instantiation:
String projectId = bigtableConfig.getProjectId();
if (projectId == null) {
projectId = bigtableConfig.getBigtableOptions().getProjectId();
}
...
InstanceName name = new BigtableInstanceName(projectId...);
return ResourceLimiterStats.getInstance(name);
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] diegomez17 commented on a diff in pull request #17823: [BEAM-14554] Create setting to report Bigtable client throttling time to Dataflow
Posted by GitBox <gi...@apache.org>.
diegomez17 commented on code in PR #17823:
URL: https://github.com/apache/beam/pull/17823#discussion_r918323769
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -1431,4 +1484,30 @@ static void validateTableExists(BigtableConfig config, PipelineOptions options)
}
}
}
+
+ interface ResourceStatsSupplier {
+ ResourceLimiterStats getStats();
+ }
+
+ private static class ResourceStatsSupplierImpl implements ResourceStatsSupplier, Serializable {
+
+ private final BigtableConfig bigtableConfig;
+
+ ResourceStatsSupplierImpl(BigtableConfig bigtableConfig) {
+ this.bigtableConfig = bigtableConfig;
+ }
+
+ @Override
+ public ResourceLimiterStats getStats() {
+ if (bigtableConfig.getProjectId() == null || bigtableConfig.getInstanceId() == null) {
+ return ResourceLimiterStats.getInstance(
+ new BigtableInstanceName(
+ bigtableConfig.getBigtableOptions().getProjectId(),
+ bigtableConfig.getBigtableOptions().getInstanceId()));
+ }
+ return ResourceLimiterStats.getInstance(
+ new BigtableInstanceName(
+ bigtableConfig.getProjectId().get(), bigtableConfig.getInstanceId().get()));
+ }
+ }
Review Comment:
Included instanceId just in case, done.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org