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 2020/07/29 07:14:46 UTC
[GitHub] [beam] robinyqiu opened a new pull request #12403: [BEAM-10597] Propagate BigQuery streaming insert throttled time to Dataflow worker in Java SDK
robinyqiu opened a new pull request #12403:
URL: https://github.com/apache/beam/pull/12403
r: @chamikaramj @ihji
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
--- | --- | --- | --- | --- | --- | ---
Go | [](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
Java | [](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
XLang | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robinyqiu commented on a change in pull request #12403: [BEAM-10597] Propagate BigQuery streaming insert throttled time to Dataflow worker
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on a change in pull request #12403:
URL: https://github.com/apache/beam/pull/12403#discussion_r471830220
##########
File path: runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/BatchModeExecutionContext.java
##########
@@ -543,6 +547,16 @@ public Long extractThrottleTime() {
totalThrottleTime += httpClientApiThrottlingTime.getCumulative();
}
+ CounterCell bigqueryStreamingInsertThrottleTime =
+ container.tryGetCounter(
+ MetricName.named(
+ BIGQUERY_STREAMING_INSERT_THROTTLE_TIME_NAMESPACE,
+ BIGQUERY_STREAMING_INSERT_THROTTLE_TIME_NAME));
Review comment:
Hi @chamikaramj @ihji ! I have made the change such that BQ, GCS, and Datastore all report throttled time in milliseconds at the beginning, and they now share the common counter name for consistency. The millisecond to second conversion is done only when later a throttled time in second is expected by worker side code. PTAL
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robinyqiu commented on pull request #12403: [BEAM-10597] Propagate BigQuery streaming insert throttled time to Dataflow worker
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on pull request #12403:
URL: https://github.com/apache/beam/pull/12403#issuecomment-677970286
Run Java PreCommit
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] chamikaramj commented on a change in pull request #12403: [BEAM-10597] Propagate BigQuery streaming insert throttled time to Dataflow worker
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on a change in pull request #12403:
URL: https://github.com/apache/beam/pull/12403#discussion_r471864446
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -867,6 +872,7 @@ public void deleteDataset(String projectId, String datasetId)
}
try {
sleeper.sleep(nextBackoffMillis);
+ throttlingMilliSeconds.inc(nextBackoffMillis);
Review comment:
Throttling results in rate limit errors, right ? If so that would be captured by backoff1 I think. Prob. Heejong can confirm.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] chamikaramj commented on pull request #12403: [BEAM-10597] Propagate BigQuery streaming insert throttled time to Dataflow worker
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on pull request #12403:
URL: https://github.com/apache/beam/pull/12403#issuecomment-677791594
Retest this please
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robinyqiu commented on a change in pull request #12403: [BEAM-10597] Propagate BigQuery streaming insert throttled time to Dataflow worker
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on a change in pull request #12403:
URL: https://github.com/apache/beam/pull/12403#discussion_r472556317
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -867,6 +872,7 @@ public void deleteDataset(String projectId, String datasetId)
}
try {
sleeper.sleep(nextBackoffMillis);
+ throttlingMilliSeconds.inc(nextBackoffMillis);
Review comment:
> Is there a way to get throttled time from all parallel threads and just use the maximum?
Yes I think this is the right thing to do. Made the change already. PTAL. WDYT, @ihji?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ihji commented on a change in pull request #12403: [BEAM-10597] Propagate BigQuery streaming insert throttled time to Dataflow worker
Posted by GitBox <gi...@apache.org>.
ihji commented on a change in pull request #12403:
URL: https://github.com/apache/beam/pull/12403#discussion_r464894202
##########
File path: runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/BatchModeExecutionContext.java
##########
@@ -543,6 +547,16 @@ public Long extractThrottleTime() {
totalThrottleTime += httpClientApiThrottlingTime.getCumulative();
}
+ CounterCell bigqueryStreamingInsertThrottleTime =
+ container.tryGetCounter(
+ MetricName.named(
+ BIGQUERY_STREAMING_INSERT_THROTTLE_TIME_NAMESPACE,
+ BIGQUERY_STREAMING_INSERT_THROTTLE_TIME_NAME));
Review comment:
+1
It would be great if we could use the same constant for all three use cases.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] chamikaramj commented on a change in pull request #12403: [BEAM-10597] Propagate BigQuery streaming insert throttled time to Dataflow worker
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on a change in pull request #12403:
URL: https://github.com/apache/beam/pull/12403#discussion_r472521741
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -867,6 +872,7 @@ public void deleteDataset(String projectId, String datasetId)
}
try {
sleeper.sleep(nextBackoffMillis);
+ throttlingMilliSeconds.inc(nextBackoffMillis);
Review comment:
Yeah, I think that's a valid concern. We probably need to figure out the time requests are throttled without including backoff due to other errors. Is there a way to get throttled time from all parallel threads and just use the maximum ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robinyqiu merged pull request #12403: [BEAM-10597] Propagate BigQuery streaming insert throttled time to Dataflow worker
Posted by GitBox <gi...@apache.org>.
robinyqiu merged pull request #12403:
URL: https://github.com/apache/beam/pull/12403
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robinyqiu commented on pull request #12403: [BEAM-10597] Propagate BigQuery streaming insert throttled time to Dataflow worker
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on pull request #12403:
URL: https://github.com/apache/beam/pull/12403#issuecomment-677980363
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aaltay commented on pull request #12403: [BEAM-10597] Propagate BigQuery streaming insert throttled time to Dataflow worker
Posted by GitBox <gi...@apache.org>.
aaltay commented on pull request #12403:
URL: https://github.com/apache/beam/pull/12403#issuecomment-673679867
@robinyqiu - What is the next step on this PR?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robinyqiu commented on pull request #12403: [BEAM-10597] Propagate BigQuery streaming insert throttled time to Dataflow worker
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on pull request #12403:
URL: https://github.com/apache/beam/pull/12403#issuecomment-666696191
Run Python PreCommit
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robinyqiu commented on pull request #12403: [BEAM-10597] Propagate BigQuery streaming insert throttled time to Dataflow worker
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on pull request #12403:
URL: https://github.com/apache/beam/pull/12403#issuecomment-673696046
> @robinyqiu - What is the next step on this PR?
I thought I replied to the comments but I didn't actually send it out. Thanks for the ping.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robinyqiu commented on a change in pull request #12403: [BEAM-10597] Propagate BigQuery streaming insert throttled time to Dataflow worker
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on a change in pull request #12403:
URL: https://github.com/apache/beam/pull/12403#discussion_r472501864
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -867,6 +872,7 @@ public void deleteDataset(String projectId, String datasetId)
}
try {
sleeper.sleep(nextBackoffMillis);
+ throttlingMilliSeconds.inc(nextBackoffMillis);
Review comment:
My major concern on accumulating `backoff1` is that we may over calculate, because we will be adding time being throttled on all threads.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robinyqiu commented on a change in pull request #12403: [BEAM-10597] Propagate BigQuery streaming insert throttled time to Dataflow worker
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on a change in pull request #12403:
URL: https://github.com/apache/beam/pull/12403#discussion_r470228614
##########
File path: runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/StreamingDataflowWorker.java
##########
@@ -520,8 +526,11 @@ public int getSize() {
private void translateKnownStepCounters(CounterUpdate stepCounterUpdate) {
CounterStructuredName structuredName =
stepCounterUpdate.getStructuredNameAndMetadata().getName();
- if (THROTTLING_MSECS_METRIC_NAME.getNamespace().equals(structuredName.getOriginNamespace())
- && THROTTLING_MSECS_METRIC_NAME.getName().equals(structuredName.getName())) {
+ if ((THROTTLING_MSECS_METRIC_NAME.getNamespace().equals(structuredName.getOriginNamespace())
+ && THROTTLING_MSECS_METRIC_NAME.getName().equals(structuredName.getName()))
+ || (BIGQUERY_STREAMING_INSERT_THROTTLE_TIME_NAMESPACE.equals(
Review comment:
Yes. GCS and Datastore counters are only consumed by batch worker (the `THROTTLING_MSECS_METRIC_NAME` counter here is a separate counter; I am not sure what this is. Maybe all throttling metrics should go to this counter? @ihji I saw you have a JIRA about it, not sure if this what you want to do).
Here in the streaming case, precision is on millisecond (whereas GCS and DataStore only store seconds)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robinyqiu commented on a change in pull request #12403: [BEAM-10597] Propagate BigQuery streaming insert throttled time to Dataflow worker
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on a change in pull request #12403:
URL: https://github.com/apache/beam/pull/12403#discussion_r470228614
##########
File path: runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/StreamingDataflowWorker.java
##########
@@ -520,8 +526,11 @@ public int getSize() {
private void translateKnownStepCounters(CounterUpdate stepCounterUpdate) {
CounterStructuredName structuredName =
stepCounterUpdate.getStructuredNameAndMetadata().getName();
- if (THROTTLING_MSECS_METRIC_NAME.getNamespace().equals(structuredName.getOriginNamespace())
- && THROTTLING_MSECS_METRIC_NAME.getName().equals(structuredName.getName())) {
+ if ((THROTTLING_MSECS_METRIC_NAME.getNamespace().equals(structuredName.getOriginNamespace())
+ && THROTTLING_MSECS_METRIC_NAME.getName().equals(structuredName.getName()))
+ || (BIGQUERY_STREAMING_INSERT_THROTTLE_TIME_NAMESPACE.equals(
Review comment:
Yes. GCS and Datastore counters are only consumed by batch worker (the `THROTTLING_MSECS_METRIC_NAME` counter here is a separate counter; I am not sure what this is. Maybe all throttling metrics should go to this counter? @ihji ).
Here in the streaming case, precision is on millisecond (whereas GCS and DataStore only store seconds)
##########
File path: runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/BatchModeExecutionContext.java
##########
@@ -543,6 +547,16 @@ public Long extractThrottleTime() {
totalThrottleTime += httpClientApiThrottlingTime.getCumulative();
}
+ CounterCell bigqueryStreamingInsertThrottleTime =
+ container.tryGetCounter(
+ MetricName.named(
+ BIGQUERY_STREAMING_INSERT_THROTTLE_TIME_NAMESPACE,
+ BIGQUERY_STREAMING_INSERT_THROTTLE_TIME_NAME));
Review comment:
Here we can use seconds, but on the streaming side msec is needed. That's the reason why I kept msec.
For consistency, we can change all counters to use msec originally, and do msec to sec conversion here. WDYT?
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -867,6 +872,7 @@ public void deleteDataset(String projectId, String datasetId)
}
try {
sleeper.sleep(nextBackoffMillis);
+ throttlingMilliSeconds.inc(nextBackoffMillis);
Review comment:
The retried failures here are transient failures, which I believe include throttling. I have thought about incrementing backoff1 but that is executed in a future (a parallel thread). If we accumulate counters over all threads then I think we will over calculate the number. So I add the counter here in the main thread.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ihji commented on a change in pull request #12403: [BEAM-10597] Propagate BigQuery streaming insert throttled time to Dataflow worker
Posted by GitBox <gi...@apache.org>.
ihji commented on a change in pull request #12403:
URL: https://github.com/apache/beam/pull/12403#discussion_r472507737
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -867,6 +872,7 @@ public void deleteDataset(String projectId, String datasetId)
}
try {
sleeper.sleep(nextBackoffMillis);
+ throttlingMilliSeconds.inc(nextBackoffMillis);
Review comment:
Yes, we should use `backoff1`. Rate limit errors only reach to this point after 2 minutes of backoffs by `backoff1` silently inside of the future. Why do you think it's over-calculated? Each thread is doing its own insert job and it doesn't look strange to me to calculate the total throttling time by adding all backoff times from parallel threads.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robinyqiu commented on a change in pull request #12403: [BEAM-10597] Propagate BigQuery streaming insert throttled time to Dataflow worker
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on a change in pull request #12403:
URL: https://github.com/apache/beam/pull/12403#discussion_r471830220
##########
File path: runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/BatchModeExecutionContext.java
##########
@@ -543,6 +547,16 @@ public Long extractThrottleTime() {
totalThrottleTime += httpClientApiThrottlingTime.getCumulative();
}
+ CounterCell bigqueryStreamingInsertThrottleTime =
+ container.tryGetCounter(
+ MetricName.named(
+ BIGQUERY_STREAMING_INSERT_THROTTLE_TIME_NAMESPACE,
+ BIGQUERY_STREAMING_INSERT_THROTTLE_TIME_NAME));
Review comment:
Hi @chamikaramj @ihji ! I have made the change such that BQ, GCS, and Datastore all report throttled time in milliseconds at the beginning, and they now share the common counter name for consistency. The millisecond to second conversion is done only when later a throttled time in seconds is expected by worker side code. PTAL
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] chamikaramj commented on a change in pull request #12403: [BEAM-10597] Propagate BigQuery streaming insert throttled time to Dataflow worker
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on a change in pull request #12403:
URL: https://github.com/apache/beam/pull/12403#discussion_r474146593
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -424,6 +427,9 @@ public Job getJob(JobReference jobRef, Sleeper sleeper, BackOff backoff)
private final PipelineOptions options;
private final long maxRowsPerBatch;
private final long maxRowBatchSize;
+ // aggregate the total time spent in exponential backoff
Review comment:
Please consider adding this to Python as well (in a separate PR).
cc: @pabloem
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] chamikaramj commented on a change in pull request #12403: [BEAM-10597] Propagate BigQuery streaming insert throttled time to Dataflow worker
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on a change in pull request #12403:
URL: https://github.com/apache/beam/pull/12403#discussion_r464549909
##########
File path: runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/BatchModeExecutionContext.java
##########
@@ -543,6 +547,16 @@ public Long extractThrottleTime() {
totalThrottleTime += httpClientApiThrottlingTime.getCumulative();
}
+ CounterCell bigqueryStreamingInsertThrottleTime =
+ container.tryGetCounter(
+ MetricName.named(
+ BIGQUERY_STREAMING_INSERT_THROTTLE_TIME_NAMESPACE,
+ BIGQUERY_STREAMING_INSERT_THROTTLE_TIME_NAME));
Review comment:
Can we use the same name as above ("cumulativeThrottlingSeconds") and move it to a constant (and also do ms to sec conversion when setting) ?
##########
File path: runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/StreamingDataflowWorker.java
##########
@@ -520,8 +526,11 @@ public int getSize() {
private void translateKnownStepCounters(CounterUpdate stepCounterUpdate) {
CounterStructuredName structuredName =
stepCounterUpdate.getStructuredNameAndMetadata().getName();
- if (THROTTLING_MSECS_METRIC_NAME.getNamespace().equals(structuredName.getOriginNamespace())
- && THROTTLING_MSECS_METRIC_NAME.getName().equals(structuredName.getName())) {
+ if ((THROTTLING_MSECS_METRIC_NAME.getNamespace().equals(structuredName.getOriginNamespace())
+ && THROTTLING_MSECS_METRIC_NAME.getName().equals(structuredName.getName()))
+ || (BIGQUERY_STREAMING_INSERT_THROTTLE_TIME_NAMESPACE.equals(
Review comment:
Is there a reason why we needed to use a unique name for BQ but not for GCS or Datastore ?
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -867,6 +872,7 @@ public void deleteDataset(String projectId, String datasetId)
}
try {
sleeper.sleep(nextBackoffMillis);
+ throttlingMilliSeconds.inc(nextBackoffMillis);
Review comment:
This is for failures. Probably you need to increment the counter for backoff1 for rate limit errors above.
cc: @ihji
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robinyqiu commented on a change in pull request #12403: [BEAM-10597] Propagate BigQuery streaming insert throttled time to Dataflow worker
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on a change in pull request #12403:
URL: https://github.com/apache/beam/pull/12403#discussion_r472514238
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -867,6 +872,7 @@ public void deleteDataset(String projectId, String datasetId)
}
try {
sleeper.sleep(nextBackoffMillis);
+ throttlingMilliSeconds.inc(nextBackoffMillis);
Review comment:
> Yes, we should use backoff1. Rate limit errors only reach to this point after 2 minutes of backoffs by backoff1 silently inside of the future.
I see. That make sense to me.
> Why do you think it's over-calculated?
Because I am not sure how this metrics is used downstream. I vaguely remember Dataflow autoscaling will use this number divided by the total time spent on work item to yield a fraction which signals throttling. If the total time is not accumulating time spent on all threads then we may over-calculate.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org