gemini-code-assist[bot] commented on code in PR #38948:
URL: https://github.com/apache/beam/pull/38948#discussion_r3405463750
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/StorageApiWritesShardedRecords.java:
##########
@@ -1066,7 +1066,25 @@ public void process(
if (numAppends > 0) {
initializeContexts.accept(contexts);
- retryManager.run(true);
+ try {
+ retryManager.run(true);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw e;
+ } catch (Exception e) {
+ Status.Code statusCode = Status.fromThrowable(e).getCode();
+ String errorMessage =
+ String.format(
+ "More than %d attempts to call AppendRows failed. Last
encountered error: %s",
+ maxRetries, e.toString());
+ if (statusCode == Status.Code.PERMISSION_DENIED
+ || statusCode == Status.Code.NOT_FOUND) {
+ errorMessage +=
+ ". Please check if the destination table exists and if the
service account has the "
+ + "bigquery.tables.updateData permission.";
+ }
Review Comment:
The error message formatting logic here is duplicated with
`StorageApiWriteUnshardedRecords`. Consider reusing the extracted shared helper
method `StorageApiWriteUnshardedRecords.buildAppendRowsErrorMessage` to
simplify this catch block and avoid code duplication.
```java
String errorMessage =\n
StorageApiWriteUnshardedRecords.buildAppendRowsErrorMessage(maxRetries, e);
```
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/StorageApiWriteUnshardedRecords.java:
##########
@@ -792,9 +792,17 @@ long flush(
// Maximum number of times we retry before we fail the work item.
if (failedContext.failureCount > allowedRetry) {
- throw new RuntimeException(
+ String errorMessage =
String.format(
- "More than %d attempts to call AppendRows failed.",
allowedRetry));
+ "More than %d attempts to call AppendRows failed. Last
encountered error: %s",
+ allowedRetry, error != null ? error.toString() :
"unknown");
+ if (statusCode == Status.Code.PERMISSION_DENIED
+ || statusCode == Status.Code.NOT_FOUND) {
+ errorMessage +=
+ ". Please check if the destination table exists and if
the service account has the "
+ + "bigquery.tables.updateData permission.";
+ }
Review Comment:
The error message formatting logic is duplicated between
`StorageApiWriteUnshardedRecords` and `StorageApiWritesShardedRecords`. To
improve maintainability and ensure consistency, consider extracting this logic
into a shared package-private static helper method in
`StorageApiWriteUnshardedRecords`.\n\nFor example, you can define the helper
method in `StorageApiWriteUnshardedRecords`:\n\n```java\nstatic String
buildAppendRowsErrorMessage(int allowedRetry, Throwable error) {\n
io.grpc.Status.Code statusCode =
io.grpc.Status.fromThrowable(error).getCode();\n String errorMessage =\n
String.format(\n "More than %d attempts to call AppendRows failed.
Last encountered error: %s",\n allowedRetry, error != null ?
error.toString() : "unknown");\n if (statusCode ==
io.grpc.Status.Code.PERMISSION_DENIED\n || statusCode ==
io.grpc.Status.Code.NOT_FOUND) {\n errorMessage +=\n ". Please check
if the destination table exists and if the service account has
the "\n + "bigquery.tables.updateData permission.";\n }\n return
errorMessage;\n}\n```
```suggestion
String errorMessage =
buildAppendRowsErrorMessage(allowedRetry, error);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
