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/06/26 21:06:47 UTC
[GitHub] [beam] danielxjd opened a new pull request #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
danielxjd opened a new pull request #12106:
URL: https://github.com/apache/beam/pull/12106
**Please** add a meaningful description for your change here
Change the IOException handling in InsertAll to prevent retrying forever for all IOException.
Now will only retry indefinitely for rate limited and quota exceeded error.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Dataflow | Flink | Samza | Spark
--- | --- | --- | --- | --- | ---
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/)
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/) | [](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_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/) | [](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] chamikaramj commented on a change in pull request #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on a change in pull request #12106:
URL: https://github.com/apache/beam/pull/12106#discussion_r451179908
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -793,6 +795,14 @@ public void deleteDataset(String projectId, String datasetId)
String.format(
"BigQuery insertAll error, retrying: %s",
ApiErrorExtractor.INSTANCE.getErrorMessage(e)));
+ GoogleJsonError.ErrorInfo errorInfo = getErrorInfo(e);
+ if (errorInfo == null) {
+ return insert.execute().getInsertErrors();
+ }
+ if (!ApiErrorExtractor.INSTANCE.rateLimited(e)
+ && !errorInfo.getReason().equals("quotaExceeded")) {
Review comment:
I would move this to a String constant. Also probably add a reference to here: https://cloud.google.com/bigquery/docs/error-messages
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -793,6 +795,14 @@ public void deleteDataset(String projectId, String datasetId)
String.format(
"BigQuery insertAll error, retrying: %s",
ApiErrorExtractor.INSTANCE.getErrorMessage(e)));
+ GoogleJsonError.ErrorInfo errorInfo = getErrorInfo(e);
+ if (errorInfo == null) {
+ return insert.execute().getInsertErrors();
Review comment:
Why are we performing an extra retry here ?
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -893,6 +903,20 @@ public void deleteDataset(String projectId, String datasetId)
ignoreInsertIds);
}
+ private GoogleJsonError.ErrorInfo getErrorInfo(IOException e) {
+ GoogleJsonResponseException jsonCause = null;
+ Throwable eCause = e;
+ if (eCause instanceof GoogleJsonResponseException) {
Review comment:
How do these exceptions extend both GoogleJsonResponseException and IOException ?
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -893,6 +903,20 @@ public void deleteDataset(String projectId, String datasetId)
ignoreInsertIds);
}
+ private GoogleJsonError.ErrorInfo getErrorInfo(IOException e) {
+ GoogleJsonResponseException jsonCause = null;
+ Throwable eCause = e;
+ if (eCause instanceof GoogleJsonResponseException) {
+ jsonCause = (GoogleJsonResponseException) eCause;
+ } else {
+ return null;
+ }
+ GoogleJsonError jsonError = jsonCause.getDetails();
Review comment:
Please add a unit test for this method.
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -793,6 +795,14 @@ public void deleteDataset(String projectId, String datasetId)
String.format(
"BigQuery insertAll error, retrying: %s",
ApiErrorExtractor.INSTANCE.getErrorMessage(e)));
+ GoogleJsonError.ErrorInfo errorInfo = getErrorInfo(e);
+ if (errorInfo == null) {
+ return insert.execute().getInsertErrors();
+ }
+ if (!ApiErrorExtractor.INSTANCE.rateLimited(e)
+ && !errorInfo.getReason().equals("quotaExceeded")) {
+ return insert.execute().getInsertErrors();
Review comment:
Ditto regarding the extra retry.
----------------------------------------------------------------
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] danielxjd commented on a change in pull request #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
danielxjd commented on a change in pull request #12106:
URL: https://github.com/apache/beam/pull/12106#discussion_r451244400
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -893,6 +906,19 @@ public void deleteDataset(String projectId, String datasetId)
ignoreInsertIds);
}
+ protected GoogleJsonError.ErrorInfo getErrorInfo(IOException e) {
+ GoogleJsonResponseException jsonCause = null;
Review comment:
True, I have further simplified the method.
----------------------------------------------------------------
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 #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on a change in pull request #12106:
URL: https://github.com/apache/beam/pull/12106#discussion_r451242654
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -893,6 +906,19 @@ public void deleteDataset(String projectId, String datasetId)
ignoreInsertIds);
}
+ protected GoogleJsonError.ErrorInfo getErrorInfo(IOException e) {
+ GoogleJsonResponseException jsonCause = null;
Review comment:
We can further simplify :)
protected GoogleJsonError.ErrorInfo getErrorInfo(IOException e) {
if (!(e instanceof GoogleJsonResponseException)) {
return null;
}
GoogleJsonError jsonError = ((GoogleJsonResponseException) e).getDetails();
...
----------------------------------------------------------------
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 #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on pull request #12106:
URL: https://github.com/apache/beam/pull/12106#issuecomment-668787873
LGTM. Thanks.
----------------------------------------------------------------
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 edited a comment on pull request #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
aaltay edited a comment on pull request #12106:
URL: https://github.com/apache/beam/pull/12106#issuecomment-663162025
@danielxjd - could you rebase your change?
----------------------------------------------------------------
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] danielxjd commented on a change in pull request #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
danielxjd commented on a change in pull request #12106:
URL: https://github.com/apache/beam/pull/12106#discussion_r462571602
##########
File path: sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImplTest.java
##########
@@ -733,26 +734,29 @@ public void testInsertOtherRetry() throws Throwable {
when(response.getContent())
.thenReturn(toStream(errorWithReasonAndStatus("actually forbidden", 403)))
.thenReturn(toStream(new TableDataInsertAllResponse()));
-
- DatasetServiceImpl dataService =
- new DatasetServiceImpl(bigquery, PipelineOptionsFactory.create());
- dataService.insertAll(
- ref,
- rows,
- null,
- BackOffAdapter.toGcpBackOff(TEST_BACKOFF.backoff()),
- TEST_BACKOFF,
- new MockSleeper(),
- InsertRetryPolicy.alwaysRetry(),
- null,
- null,
- false,
- false,
- false);
- verify(response, times(2)).getStatusCode();
- verify(response, times(2)).getContent();
- verify(response, times(2)).getContentType();
- expectedLogs.verifyInfo("BigQuery insertAll error, retrying:");
+ try {
+ DatasetServiceImpl dataService =
+ new DatasetServiceImpl(bigquery, PipelineOptionsFactory.create());
+ dataService.insertAll(
+ ref,
+ rows,
+ null,
+ BackOffAdapter.toGcpBackOff(TEST_BACKOFF.backoff()),
+ TEST_BACKOFF,
+ new MockSleeper(),
+ InsertRetryPolicy.alwaysRetry(),
+ null,
+ null,
+ false,
+ false,
+ false);
+ fail();
+ } catch (RuntimeException e) {
+ assertTrue(e instanceof RuntimeException);
Review comment:
I have changed it to use thrown.
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -806,6 +811,19 @@ public void deleteDataset(String projectId, String datasetId)
try {
return insert.execute().getInsertErrors();
} catch (IOException e) {
+ GoogleJsonError.ErrorInfo errorInfo = getErrorInfo(e);
+ if (errorInfo == null) {
+ throw e;
+ }
+ /**
+ * TODO:Check for quotaExceededError The check will be replaced by
Review comment:
Jira issue added
----------------------------------------------------------------
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] danielxjd commented on a change in pull request #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
danielxjd commented on a change in pull request #12106:
URL: https://github.com/apache/beam/pull/12106#discussion_r451554078
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -893,6 +903,20 @@ public void deleteDataset(String projectId, String datasetId)
ignoreInsertIds);
}
+ private GoogleJsonError.ErrorInfo getErrorInfo(IOException e) {
+ GoogleJsonResponseException jsonCause = null;
+ Throwable eCause = e;
+ if (eCause instanceof GoogleJsonResponseException) {
+ jsonCause = (GoogleJsonResponseException) eCause;
+ } else {
+ return null;
+ }
+ GoogleJsonError jsonError = jsonCause.getDetails();
Review comment:
Test added
----------------------------------------------------------------
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] danielxjd commented on a change in pull request #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
danielxjd commented on a change in pull request #12106:
URL: https://github.com/apache/beam/pull/12106#discussion_r451241677
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -893,6 +903,20 @@ public void deleteDataset(String projectId, String datasetId)
ignoreInsertIds);
}
+ private GoogleJsonError.ErrorInfo getErrorInfo(IOException e) {
+ GoogleJsonResponseException jsonCause = null;
+ Throwable eCause = e;
+ if (eCause instanceof GoogleJsonResponseException) {
Review comment:
Yup, that should work better.
----------------------------------------------------------------
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 #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on a change in pull request #12106:
URL: https://github.com/apache/beam/pull/12106#discussion_r451237362
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -893,6 +903,20 @@ public void deleteDataset(String projectId, String datasetId)
ignoreInsertIds);
}
+ private GoogleJsonError.ErrorInfo getErrorInfo(IOException e) {
+ GoogleJsonResponseException jsonCause = null;
+ Throwable eCause = e;
+ if (eCause instanceof GoogleJsonResponseException) {
Review comment:
Thanks. In that case let's simplify this to following.
if (!(e instanceof GoogleJsonResponseException)) {
return null;
}
GoogleJsonResponseException jsonCause = (GoogleJsonResponseException) e;
...
----------------------------------------------------------------
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] danielxjd commented on pull request #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
danielxjd commented on pull request #12106:
URL: https://github.com/apache/beam/pull/12106#issuecomment-650479948
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] danielxjd removed a comment on pull request #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
danielxjd removed a comment on pull request #12106:
URL: https://github.com/apache/beam/pull/12106#issuecomment-665152872
----------------------------------------------------------------
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] danielxjd commented on pull request #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
danielxjd commented on pull request #12106:
URL: https://github.com/apache/beam/pull/12106#issuecomment-668274752
@chamikaramj Do you have any additional comments?
----------------------------------------------------------------
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] danielxjd commented on a change in pull request #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
danielxjd commented on a change in pull request #12106:
URL: https://github.com/apache/beam/pull/12106#discussion_r451554555
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -793,6 +795,14 @@ public void deleteDataset(String projectId, String datasetId)
String.format(
"BigQuery insertAll error, retrying: %s",
ApiErrorExtractor.INSTANCE.getErrorMessage(e)));
+ GoogleJsonError.ErrorInfo errorInfo = getErrorInfo(e);
+ if (errorInfo == null) {
+ return insert.execute().getInsertErrors();
Review comment:
I have changed it to throw the exception directly and changed the corresponding unit test.
----------------------------------------------------------------
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 #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
aaltay commented on pull request #12106:
URL: https://github.com/apache/beam/pull/12106#issuecomment-663162025
@danielxjd - could you rebased your change?
----------------------------------------------------------------
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 #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
ihji commented on a change in pull request #12106:
URL: https://github.com/apache/beam/pull/12106#discussion_r462534513
##########
File path: sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImplTest.java
##########
@@ -733,26 +734,29 @@ public void testInsertOtherRetry() throws Throwable {
when(response.getContent())
.thenReturn(toStream(errorWithReasonAndStatus("actually forbidden", 403)))
.thenReturn(toStream(new TableDataInsertAllResponse()));
-
- DatasetServiceImpl dataService =
- new DatasetServiceImpl(bigquery, PipelineOptionsFactory.create());
- dataService.insertAll(
- ref,
- rows,
- null,
- BackOffAdapter.toGcpBackOff(TEST_BACKOFF.backoff()),
- TEST_BACKOFF,
- new MockSleeper(),
- InsertRetryPolicy.alwaysRetry(),
- null,
- null,
- false,
- false,
- false);
- verify(response, times(2)).getStatusCode();
- verify(response, times(2)).getContent();
- verify(response, times(2)).getContentType();
- expectedLogs.verifyInfo("BigQuery insertAll error, retrying:");
+ try {
+ DatasetServiceImpl dataService =
+ new DatasetServiceImpl(bigquery, PipelineOptionsFactory.create());
+ dataService.insertAll(
+ ref,
+ rows,
+ null,
+ BackOffAdapter.toGcpBackOff(TEST_BACKOFF.backoff()),
+ TEST_BACKOFF,
+ new MockSleeper(),
+ InsertRetryPolicy.alwaysRetry(),
+ null,
+ null,
+ false,
+ false,
+ false);
+ fail();
+ } catch (RuntimeException e) {
+ assertTrue(e instanceof RuntimeException);
Review comment:
I think it looks better to use `thrown.expect` and `thrown.expectMessage` instead of `assertTrue` with `instanceof`.
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -806,6 +811,19 @@ public void deleteDataset(String projectId, String datasetId)
try {
return insert.execute().getInsertErrors();
} catch (IOException e) {
+ GoogleJsonError.ErrorInfo errorInfo = getErrorInfo(e);
+ if (errorInfo == null) {
+ throw e;
+ }
+ /**
+ * TODO:Check for quotaExceededError The check will be replaced by
Review comment:
TODO(BEAM-10584): Check for QUOTA_EXCEEDED error will be replaced by
----------------------------------------------------------------
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] danielxjd commented on a change in pull request #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
danielxjd commented on a change in pull request #12106:
URL: https://github.com/apache/beam/pull/12106#discussion_r451213408
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -893,6 +903,20 @@ public void deleteDataset(String projectId, String datasetId)
ignoreInsertIds);
}
+ private GoogleJsonError.ErrorInfo getErrorInfo(IOException e) {
+ GoogleJsonResponseException jsonCause = null;
+ Throwable eCause = e;
+ if (eCause instanceof GoogleJsonResponseException) {
Review comment:
GoogleJsonResponseException extends HttpResponseException and HttpResponseException extends IOException
----------------------------------------------------------------
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 #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
aaltay commented on pull request #12106:
URL: https://github.com/apache/beam/pull/12106#issuecomment-665203110
Run Spotless 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] ihji commented on pull request #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
ihji commented on pull request #12106:
URL: https://github.com/apache/beam/pull/12106#issuecomment-665922796
LGTM. Thanks.
@chamikaramj Any additional comment?
----------------------------------------------------------------
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] danielxjd commented on pull request #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
danielxjd commented on pull request #12106:
URL: https://github.com/apache/beam/pull/12106#issuecomment-663266467
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] ihji commented on pull request #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
ihji commented on pull request #12106:
URL: https://github.com/apache/beam/pull/12106#issuecomment-650413159
CC: @chamikaramj
----------------------------------------------------------------
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] danielxjd commented on a change in pull request #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
danielxjd commented on a change in pull request #12106:
URL: https://github.com/apache/beam/pull/12106#discussion_r461174221
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java
##########
@@ -909,6 +927,15 @@ public void deleteDataset(String projectId, String datasetId)
ignoreInsertIds);
}
+ protected GoogleJsonError.ErrorInfo getErrorInfo(IOException e) {
+ if (!(e instanceof GoogleJsonResponseException)) {
+ return null;
+ }
+ GoogleJsonError jsonError = ((GoogleJsonResponseException) e).getDetails();
+ GoogleJsonError.ErrorInfo errorInfo = Iterables.getFirst(jsonError.getErrors(), null);
Review comment:
Only return the first element ErrorInfo. This is according to the logic and comment of getErrorInfo() from ApiErrorExtractor.
----------------------------------------------------------------
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] danielxjd commented on pull request #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
danielxjd commented on pull request #12106:
URL: https://github.com/apache/beam/pull/12106#issuecomment-665152872
----------------------------------------------------------------
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] danielxjd commented on pull request #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
danielxjd commented on pull request #12106:
URL: https://github.com/apache/beam/pull/12106#issuecomment-650405295
R: @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] chamikaramj commented on pull request #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on pull request #12106:
URL: https://github.com/apache/beam/pull/12106#issuecomment-650426479
R: @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] chamikaramj merged pull request #12106: [BEAM-9792] Fixing the IOException handling in InsertAll for BigQuery
Posted by GitBox <gi...@apache.org>.
chamikaramj merged pull request #12106:
URL: https://github.com/apache/beam/pull/12106
----------------------------------------------------------------
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