huaxingao commented on code in PR #17947:
URL: https://github.com/apache/iceberg/pull/17947#discussion_r3943097746
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -166,6 +166,9 @@ private static class HeaderValidatingAdapter extends
RESTCatalogAdapter {
private final HTTPHeaders contextHeaders;
private final java.util.concurrent.ConcurrentMap<String, RuntimeException>
simulateFailureOnFirstSuccessByKey = new
java.util.concurrent.ConcurrentHashMap<>();
+ // Records the Idempotency-Key value seen on every mutation request, in
arrival order.
+ private final List<String> observedMutationIdempotencyKeys =
+ new java.util.concurrent.CopyOnWriteArrayList<>();
Review Comment:
nit: import `java.util.concurrent.CopyOnWriteArrayList`
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -3477,30 +3488,49 @@ public void
testIdempotentCreateReplayAfterSimulated503() {
IdempotentEnv env = idempotentEnv(key, ns, "t_idemp");
CreateTableRequest req = createReq(env.ident);
- // First attempt: server finalizes success but responds 503
- assertThatThrownBy(
- () ->
- env.http.post(
-
ResourcePaths.forCatalogProperties(ImmutableMap.of()).tables(ns),
- req,
- LoadTableResponse.class,
- env.headers,
- ErrorHandlers.tableErrorHandler()))
- .isInstanceOf(RuntimeException.class)
- .hasMessageContaining("simulated transient 503");
+ // The client auto-retries the keyed POST on 503; the server replays the
finalized 200, so
+ // the call succeeds transparently without the caller needing to retry
manually.
+ LoadTableResponse response =
+ env.http.post(
+ ResourcePaths.forCatalogProperties(ImmutableMap.of()).tables(ns),
+ req,
+ LoadTableResponse.class,
+ env.headers,
+ ErrorHandlers.tableErrorHandler());
+ assertThat(response).isNotNull();
// Verify request shape (method, path, headers including Idempotency-Key)
verifyCreatePost(ns, env.headers);
+ }
+
+ @Test
+ public void testIdempotentCreateRetryCarriesSameKey() {
+ // Pin the invariant: when the client auto-retries a keyed POST
(503-then-200), every transport
+ // attempt must carry the identical Idempotency-Key so the server can
replay the cached result.
+ String key = "idemp-same-key-retry";
+ adapterForRESTServer.simulate503OnFirstSuccessForKey(key);
+ Namespace ns = Namespace.of("ns_samekey");
+ IdempotentEnv env = idempotentEnv(key, ns, "t_samekey");
+ CreateTableRequest req = createReq(env.ident);
- // Retry with same key: server should replay 200 OK
- LoadTableResponse replay =
+ // Trigger the 503-then-200 retry cycle; the call must succeed
transparently.
+ LoadTableResponse response =
env.http.post(
ResourcePaths.forCatalogProperties(ImmutableMap.of()).tables(ns),
req,
LoadTableResponse.class,
env.headers,
ErrorHandlers.tableErrorHandler());
- assertThat(replay).isNotNull();
+ assertThat(response).isNotNull();
+
+ // The adapter must have observed the Idempotency-Key on exactly 2
transport attempts
+ // (initial attempt + one auto-retry) and both values must be identical.
+ List<String> observedKeys =
+ adapterForRESTServer.observedMutationIdempotencyKeys().stream()
+ .filter(k -> k.equals(key))
+ .collect(Collectors.toList());
+ assertThat(observedKeys).hasSize(2);
+ assertThat(observedKeys.get(0)).isNotNull().isEqualTo(observedKeys.get(1));
Review Comment:
Nit: this can't fail. The `filter(k -> k.equals(key))` above already
guarantees every surviving element equals `key`, so this compares `key` to
`key`. `hasSize(2)` is the real check, so this line can just be dropped.
--
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]
