gemini-code-assist[bot] commented on code in PR #38948:
URL: https://github.com/apache/beam/pull/38948#discussion_r3405453126
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIOWriteTest.java:
##########
@@ -1659,6 +1659,48 @@ public void
testStreamingStorageApiWriteWithErrorHandling() throws Exception {
storageWriteWithErrorHandling(false);
}
+ @Test
+ public void testStorageApiWriteFailureExhaustedRetries() throws Exception {
+ assumeTrue(useStorageApi);
+
+ // Set up fake dataset service to return PERMISSION_DENIED for appendRows
+ fakeDatasetService.setAppendRowsError(
+ new io.grpc.StatusRuntimeException(
+ io.grpc.Status.PERMISSION_DENIED.withDescription("Missing
permissions")));
+
+ List<Integer> elements = Lists.newArrayList(1, 2, 3);
+
+ BigQueryIO.Write<Integer> write =
+ BigQueryIO.<Integer>write()
+ .to("project-id:dataset-id.table-id")
+
.withCreateDisposition(BigQueryIO.Write.CreateDisposition.CREATE_NEVER)
+ .withFormatFunction(
+ (SerializableFunction<Integer, TableRow>)
+ input -> new TableRow().set("number", input))
+ .withSchema(
+ new TableSchema()
+ .setFields(
+ ImmutableList.of(
+ new
TableFieldSchema().setName("number").setType("INTEGER"))))
+ .withTestServices(fakeBqServices)
+ .withoutValidation();
+
+ if (useStreaming) {
+ write = write.withTriggeringFrequency(Duration.standardSeconds(30));
+ }
+
+ PCollection<Integer> input =
p.apply(Create.of(elements).withCoder(BigEndianIntegerCoder.of()));
+ input.apply("WriteToBQ", write);
+
+ thrown.expect(RuntimeException.class);
+ thrown.expectMessage("More than");
+ thrown.expectMessage("attempts to call AppendRows failed");
Review Comment:
Update the test assertions to match the simplified and more accurate error
message from the sharded write path.
```suggestion
thrown.expectMessage("AppendRows failed");
```
##########
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());
Review Comment:
The error message states `"More than %d attempts to call AppendRows
failed."` but this catch block catches any exception thrown by
`retryManager.run(true)`. If a persistent/non-retryable error (such as
`PERMISSION_DENIED` or `NOT_FOUND`) occurs on the very first attempt, the retry
manager will fail immediately without retrying. In such cases, claiming that
more than `%d` attempts failed is misleading. Consider using a more general
message like `"AppendRows failed."` which is accurate for both single-attempt
persistent failures and exhausted retries.
```suggestion
String errorMessage =
String.format(
"AppendRows failed. Last encountered error: %s",
e.toString());
```
--
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]
