gemini-code-assist[bot] commented on code in PR #38948:
URL: https://github.com/apache/beam/pull/38948#discussion_r3404728498
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/StorageApiWriteUnshardedRecords.java:
##########
@@ -792,9 +792,18 @@ 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 "
+ + "TABLES_UPDATE_DATA permission.";
+ }
+ LOG.error("{}", errorMessage, error);
+ throw new RuntimeException(errorMessage, error);
Review Comment:
Logging the exception immediately before throwing it is an anti-pattern. In
Apache Beam, the runner will catch and log any thrown exception at the `ERROR`
level, so logging it here results in duplicate stack traces in the worker logs,
making troubleshooting more difficult. Removing the `LOG.error` call keeps the
logs clean.
```suggestion
String errorMessage =
String.format(
"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 "
+ "TABLES_UPDATE_DATA permission.";
}
throw new RuntimeException(errorMessage, error);
```
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/StorageApiWritesShardedRecords.java:
##########
@@ -1066,7 +1066,27 @@ public void process(
if (numAppends > 0) {
initializeContexts.accept(contexts);
- retryManager.run(true);
+ try {
+ retryManager.run(true);
+ } catch (Exception e) {
+ Throwable cause = e.getCause();
+ if (cause == null) {
+ cause = e;
+ }
+ Status.Code statusCode = Status.fromThrowable(cause).getCode();
+ String errorMessage =
+ String.format(
+ "More than %d attempts to call AppendRows failed. Last
encountered error: %s",
+ maxRetries, cause.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 "
+ + "TABLES_UPDATE_DATA permission.";
+ }
+ LOG.error("{}", errorMessage, cause);
+ throw new RuntimeException(errorMessage, cause);
+ }
Review Comment:
There are a few issues here:
1. **Duplicate Logging**: Logging the exception right before throwing it
causes duplicate stack traces in the runner logs.
2. **Lost Stack Trace**: Throwing `new RuntimeException(errorMessage,
cause)` where `cause = e.getCause()` discards the stack trace of the outer
exception `e`. It is better to pass `e` directly to preserve the full exception
chain.
3. **Simplification**: `Status.fromThrowable` already traverses the cause
chain internally, so you can pass `e` directly to it and avoid manual
null-checking and extraction of `e.getCause()`.
```java
try {
retryManager.run(true);
} 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 "
+ "TABLES_UPDATE_DATA permission.";
}
throw new RuntimeException(errorMessage, 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
