dennishuo commented on code in PR #14196:
URL: https://github.com/apache/iceberg/pull/14196#discussion_r2452836457


##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -1903,6 +1926,34 @@ components:
       schema:
         type: string
 
+    idempotency-key:
+      name: Idempotency-Key
+      in: header
+      required: false
+      schema:
+        type: string
+        format: uuid
+        minLength: 36
+        maxLength: 36
+        example: "550e8400-e29b-41d4-a716-446655440000"
+      description: |
+        Optional client-provided idempotency key for safe request retries.
+
+        When present, the server ensures no additional effects for requests 
that carry the same
+        Idempotency-Key within the same operation/resource scope. If a prior 
request with this key
+        has been finalized, the server returns the previously finalized 
response instead of
+        re-executing the mutation.
+
+        Finalization rules:
+        - Finalize & replay: 200, 201, 204, and deterministic terminal 4xx
+        - Do not finalize (not stored/replayed): 5xx, 409 request_in_progress
+
+        Key Requirements:
+        - Key format: UUID (V7 preferred)

Review Comment:
   My main concern about documenting mandatory v7 is if that's paired with an 
underspecified spec for required server-side behaviors relative to internal 
semantics of the v7 uuid:
   
   1. Is the server required to parse the timestamp out of the v7 uuid and also 
reject "old" uuids that are *implicitly* older than the idempotency key 
lifetime?
   2. Is the server required to enforce that it's a valid v7 uuid?
   3. Is the server required to reject v7 uuids with an implied timestamp that 
is "in the future"?
   
   A potential problem would be if these are left unspecified in the spec, and 
then certain behaviors become de-facto expectations *by convention* such that 
callers start silently depending on the subtleties of those behaviors for 
correctness.
   
   This is different from only saying "v7 preferred" because once v7 is 
mandatory, server implementations may argue that it's now safe to make 
behavioral decisions based on deeper semantic inferences from the internal 
contents of the uuid.
   
   This could be mitigated by either adding server-behavior requirements *or* 
explicitly documenting the non-expectation of server behaviors, e.g.: `Server 
implementations are *not* required to validate v7 uuid` or `Server 
implementations *must* validate syntactic validity of the v7 uuid but must not 
make behavioral decisions based on the internal timestamp semantics of the 
uuid` (could have a reference test case for REST Catalogs that always uses a 
fixed uuid v7 that corresponds to "time 0" epoch time), or go all the way to 
something like `Server implementations must reject uuids with an implied 
timestamp older than the advertised 'idempotency-key-lifetime', if it exists`



-- 
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]

Reply via email to