steveloughran commented on code in PR #15308:
URL: https://github.com/apache/iceberg/pull/15308#discussion_r3095252326
##########
bigquery/src/main/java/org/apache/iceberg/gcp/bigquery/BigQueryTableOperations.java:
##########
@@ -82,48 +84,49 @@ public void doRefresh() {
// atomically
@Override
public void doCommit(TableMetadata base, TableMetadata metadata) {
- String newMetadataLocation =
- base == null && metadata.metadataFileLocation() != null
- ? metadata.metadataFileLocation()
- : writeNewMetadata(metadata, currentVersion() + 1);
- BaseMetastoreOperations.CommitStatus commitStatus =
- BaseMetastoreOperations.CommitStatus.FAILURE;
+ CommitStatus commitStatus = CommitStatus.FAILURE;
+ RetryDetector retryDetector = new RetryDetector();
+
+ String newMetadataLocation = null;
try {
- if (base == null) {
- createTable(newMetadataLocation, metadata);
+ boolean newTable = base == null;
+ newMetadataLocation = writeNewMetadataIfRequired(newTable, metadata);
+
+ if (newTable) {
+ createTable(newMetadataLocation, metadata, retryDetector);
} else {
- updateTable(newMetadataLocation, metadata);
+ updateTable(newMetadataLocation, metadata, retryDetector);
}
- commitStatus = BaseMetastoreOperations.CommitStatus.SUCCESS;
- } catch (CommitFailedException | CommitStateUnknownException e) {
+ commitStatus = CommitStatus.SUCCESS;
+ } catch (CommitFailedException e) {
throw e;
} catch (Throwable e) {
LOG.error("Exception thrown on commit: ", e);
- if (e instanceof AlreadyExistsException) {
- throw e;
+ boolean isAlreadyExistsException = e instanceof AlreadyExistsException;
+ boolean isRuntimeIOException = e instanceof RuntimeIOException;
+
+ // If retries occurred, an earlier attempt may have succeeded. If we got
a
+ // RuntimeIOException, we have no way of knowing if the request reached
the server.
+ // In either case, check whether the commit actually succeeded.
+ if ((isRuntimeIOException || retryDetector.retried()) &&
newMetadataLocation != null) {
+ LOG.warn(
Review Comment:
ok, so this is the "first attempt succeeded but response was lost, second
request rejected as destination exists" case. And this case is guaranteed not
to raise an AlreadyExistsException?
Azure storage renames fail in odd ways here on internal retries, so it's
interesting to see how other stores behave
##########
bigquery/src/test/java/org/apache/iceberg/gcp/bigquery/TestBigQueryTableOperations.java:
##########
@@ -288,4 +294,44 @@ private static Optional<String> metadataFilePath(String
tableDir) throws IOExcep
return Optional.empty();
}
+
+ private TableMetadata createTestMetadata() {
+ return TableMetadata.newTableMetadata(
+ SCHEMA,
+ PartitionSpec.unpartitioned(),
+ SortOrder.unsorted(),
+ tempFolder.getPath() + "/test-table",
+ ImmutableMap.of());
+ }
+
+ @Test
+ public void testCommitFailedExceptionThrowsDirectly() {
+ // CommitFailedException should fail immediately without retry or status
checks
+ when(client.load(TABLE_REFERENCE)).thenThrow(new
NoSuchTableException("Table not found"));
+ when(client.create(any(), any())).thenThrow(new
CommitFailedException("Commit rejected"));
+
+ tableOps.refresh();
+
+ assertThatThrownBy(() -> tableOps.commit(null, createTestMetadata()))
+ .isInstanceOf(CommitFailedException.class)
+ .hasMessage("Commit rejected");
+
+ verify(client, times(1)).create(any(Table.class),
any(RetryDetector.class));
+ verify(client, times(1)).load(TABLE_REFERENCE);
+ }
+
+ @Test
+ public void testRuntimeIOExceptionTriggersCommitStatusCheck() {
+ // RuntimeIOException should trigger commit status check as the commit may
have succeeded
+ when(client.load(TABLE_REFERENCE)).thenThrow(new
NoSuchTableException("Table not found"));
+ when(client.create(any(), any())).thenThrow(new
RuntimeIOException("Network error"));
+
+ tableOps.refresh();
+
+ assertThatThrownBy(() -> tableOps.commit(null, createTestMetadata()))
+ .isInstanceOf(CommitStateUnknownException.class)
+ .hasMessageContaining("Network error");
+
+ verify(client, atLeast(2)).load(TABLE_REFERENCE);
+ }
Review Comment:
I do think that the AlreadyExistsException check would be good as we want to
make sure that an AlreadyExistsException is rethrown
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
