amogh-jahagirdar commented on code in PR #15051:
URL: https://github.com/apache/iceberg/pull/15051#discussion_r2742395316
##########
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:
Ah this is my bad, I had to look at the code further. You're right, given
how the http client is invoked, to trigger a failure and go through the error
handler, we can't just mock execute. The error handler invocation is handled
below the execution API. I think this is fine, maybe we want a separate helper
method for better simulation of errors that go through the error handlers.
On a separate note, I'm starting to think a bit more about how we look at
CleanableFailure. The original intent of the CleanableFailure marker interface
was to prevent issues in case arbitrary exceptions happened to be thrown on the
commit path. We generally expect Error handlers to map to the commit state
unknown, and any other exception we fail to handle goes through as a runtime
exception, which we validate is cleanable or not. Anyways this is separate.
Minor point, I'd just shorten the test name a bit:
`testNoCleanupOnCreate503` or something. The commit state unknown is more of an
internal handling detail for this case.
--
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]
