amogh-jahagirdar commented on code in PR #14196:
URL: https://github.com/apache/iceberg/pull/14196#discussion_r2400595346
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -1903,6 +2003,35 @@ 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 an equivalent final response
without re-running the
+ operation. The response body may reflect a more recent state of the
catalog than what it
Review Comment:
The response body may reflect a newer state of the `table` than existed at
the time of the commit.
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -1966,6 +2018,18 @@ components:
"GET /v1/{prefix}/namespaces/{namespace}/tables/{table}",
"GET /v1/{prefix}/namespaces/{namespace}/views/{view}"
]
+ idempotency-key-lifetime:
+ type: string
+ format: duration
+ description: >
+ Client reuse window for an Idempotency-Key (ISO-8601 duration,
e.g., PT30M, PT24H).
+ Interpreted as the maximum time from the first submission using a
key to the last retry
+ during which a client may reuse that key. Servers SHOULD accept
retries for at least this
+ duration and MAY include a grace period to account for
delays/clock skew. Clients SHOULD NOT
+ reuse an Idempotency-Key after this window elapses; they SHOULD
generate a new key for any
+ subsequent attempt. Presence of this field indicates the server
supports Idempotency-Key
Review Comment:
I think we just need to say that the client `should` generate a new key for
a request that is outside of the window, which seems to be what is said already.
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -4817,6 +4955,16 @@ components:
}
}
+ IdempotencyInProgressError:
Review Comment:
Sorry if this has been discussed before, but a "InProgressError" doesn't
sound quite right to me? I understand there's a window but wouldn't it be some
sort of "RequestAlreadyProcessed" (past tense)?
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -4817,6 +4955,16 @@ components:
}
}
+ IdempotencyInProgressError:
Review Comment:
My bad, this is the case for detecting a duplicate request for one that is
already in flight. If the request is already successfully processed, nothing to
do but return a sucessfull response.
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -1903,6 +2003,35 @@ 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 an equivalent final response
without re-running the
+ operation. The response body may reflect a more recent state of the
catalog than what it
Review Comment:
Also minor, tehres an extra period on the last sentence
##########
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:
I don't think the spec should get in the business of defining a required
format of the key.
My mental model at least is that these keys are effectively opaque
structures generated by the client under certain conditions, and the server
can either choose to say "Yeah I can work with that structure and dedupe" or
"No, this is not allowed".
If a client can generate these V7 UUIDs, and a server can handle it that's
great. If the client cannot generate these newer UUIDs, but a server can handle
it within the other requirements of this spec that's great. If a server cannot
handle it, it would reject the request. Keeping the spec open that way, allows
for different components to evolve as desired while still being able to derive
the most value out of this proposal.
--
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]
