amogh-jahagirdar commented on code in PR #15051:
URL: https://github.com/apache/iceberg/pull/15051#discussion_r2714955820
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -2543,6 +2543,74 @@ public void
testNoCleanupForNonCleanableCreateTransaction() {
});
}
+ @Test
+ public void testNoCleanup503MappedToCommitStateUnknownCreateTransaction() {
+ RESTCatalogAdapter adapter =
+ Mockito.spy(
+ new RESTCatalogAdapter(backendCatalog) {
+ @Override
+ protected <T extends RESTResponse> T execute(
+ HTTPRequest request,
+ Class<T> responseType,
+ Consumer<ErrorResponse> errorHandler,
+ Consumer<Map<String, String>> responseHeaders) {
+ if (request.method() == HTTPMethod.POST &&
request.path().contains("some_table")) {
+ // Simulate a 503 Service Unavailable error
+ ErrorResponse error =
+ ErrorResponse.builder()
+ .responseCode(503)
+ .withMessage("Service unavailable")
+ .build();
+
+ errorHandler.accept(error);
+ throw new IllegalStateException("Error handler should have
thrown");
+ }
+ return super.execute(request, responseType, errorHandler,
responseHeaders);
Review Comment:
I feel like we should be able to slim this setup down a bit by just doing
something like:
```
RESTCatalogAdapter adapter = Mockito.spy(new
RESTCatalogAdapter(backendCatalog));
Mockito.doThrow(new ServiceFailureException("some service failure"))
.when(adapter)
.execute(reqMatcher(HTTPMethod.POST), any(), any(), any());
```
Think it's OK to mock the commit state unknown here, since all we're trying
to test is the client's reaction to it on create? I see the other tests in this
class do that as well.
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -2543,6 +2543,74 @@ public void
testNoCleanupForNonCleanableCreateTransaction() {
});
}
+ @Test
+ public void testNoCleanup503MappedToCommitStateUnknownCreateTransaction() {
+ RESTCatalogAdapter adapter =
+ Mockito.spy(
+ new RESTCatalogAdapter(backendCatalog) {
+ @Override
+ protected <T extends RESTResponse> T execute(
+ HTTPRequest request,
+ Class<T> responseType,
+ Consumer<ErrorResponse> errorHandler,
+ Consumer<Map<String, String>> responseHeaders) {
+ if (request.method() == HTTPMethod.POST &&
request.path().contains("some_table")) {
+ // Simulate a 503 Service Unavailable error
+ ErrorResponse error =
+ ErrorResponse.builder()
+ .responseCode(503)
+ .withMessage("Service unavailable")
+ .build();
+
+ errorHandler.accept(error);
+ throw new IllegalStateException("Error handler should have
thrown");
+ }
+ return super.execute(request, responseType, errorHandler,
responseHeaders);
+ }
+ });
+
+ RESTCatalog catalog = catalog(adapter);
+
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TABLE.namespace());
+ }
+
+ catalog.createTable(TABLE, SCHEMA);
+ TableIdentifier newTable = TableIdentifier.of(TABLE.namespace(),
"some_table");
+
+ Transaction createTableTransaction =
catalog.newCreateTableTransaction(newTable, SCHEMA);
+ createTableTransaction.newAppend().appendFile(FILE_A).commit();
+
+ // Verify that 503 is mapped to CommitStateUnknownException (not just
ServiceFailureException)
+ assertThatThrownBy(createTableTransaction::commitTransaction)
+ .isInstanceOf(CommitStateUnknownException.class)
+ .cause()
+ .isInstanceOf(ServiceFailureException.class)
+ .hasMessageContaining("Service failed: 503");
+
+ // Verify files are NOT cleaned up (because commit state is unknown)
+ assertThat(allRequests(adapter))
+ .anySatisfy(
+ req -> {
+ assertThat(req.method()).isEqualTo(HTTPMethod.POST);
+ assertThat(req.path()).isEqualTo(RESOURCE_PATHS.table(newTable));
+ assertThat(req.body()).isInstanceOf(UpdateTableRequest.class);
+ UpdateTableRequest body = (UpdateTableRequest) req.body();
+ Optional<MetadataUpdate> appendSnapshot =
+ body.updates().stream()
+ .filter(update -> update instanceof
MetadataUpdate.AddSnapshot)
+ .findFirst();
+ assertThat(appendSnapshot).isPresent();
+
+ MetadataUpdate.AddSnapshot addSnapshot =
+ (MetadataUpdate.AddSnapshot) appendSnapshot.get();
+ String manifestListLocation =
addSnapshot.snapshot().manifestListLocation();
+ // Files should still exist because we don't know if commit
succeeded
+
assertThat(catalog.loadTable(TABLE).io().newInputFile(manifestListLocation).exists())
+ .isTrue();
Review Comment:
Maybe a more assertJ "fluent" way:
```
assertThat(body.updates().stream()
.filter(MetadataUpdate.AddSnapshot.class::isInstance)
.map(MetadataUpdate.AddSnapshot.class::cast)
.findFirst())
.hasValueSatisfying(addSnapshot -> {
String manifestListLocation =
addSnapshot.snapshot().manifestListLocation();
assertThat(catalog.loadTable(TABLE).io().newInputFile(manifestListLocation).exists())
.isTrue();
});
```
--
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]
