The overall approach looks solid to me. My only concern is with the use of the Retry-After header, it doesn't guarantee whether the server actually processed the commit. It's possible the server completed the commit and then became unavailable, still returning the Retry-After header. If the client retries in that situation, it could corrupt the table. This risk is even worse when load balancers are involved. Since they typically sit between REST catalogs and clients, and may be managed by different teams or vendors. They often have no visibility into whether a commit was fully processed or not.
Given these uncertainties, I still believe it's safer to avoid retrying on a 503 response, even if the Retry-After header is present. Admittedly, skipping the retry means the client may need to recompute the commit, which is costly. But we could mitigate that cost by implementing a reconciliation step, like reloading the table. In most cases, reloading would be significantly lighter than a full recomputation. Yufei On Thu, Aug 14, 2025 at 10:02 AM Daniel Weeks <dwe...@apache.org> wrote: > I would agree with making 503 non-retryable by default. Though I don't > necessarily agree with the spec change in the PR. I feel like the only > thing that needs to be updated is the component definition in the spec to > say that clients may only retry if a `Retry-After` header is present. > Otherwise, they should treat it as non-retryable. > > I believe we only included that language due to services like S3 using > "503 Service Unavailable" as opposed to the more appropriate "429 Too Many > Requests" for throttling, but I don't think 503 is a good practice to > propagate for this scenario. We don't call out 429 in the spec, but most > clients support this by default and I believe the reference implementation > does as well. > > -Dan > > > > > On Mon, Aug 11, 2025 at 5:26 PM huaxin gao <huaxin.ga...@gmail.com> wrote: > >> I'm in favor of marking 503 as non-retryable for updateTable to avoid >> possible data corruption. Even though 503 can sometimes mean throttling, I >> think treating it as CommitStateUnknown is the safer choice for commit >> operations. We can look into better handling for throttling cases, like >> using 429, in the future. >> >> On Tue, Aug 5, 2025 at 11:00 AM Steve Loughran >> <ste...@cloudera.com.invalid> wrote: >> >>> 503 is throttling on many (but not all) of the AWS services (s3, >>> lambda, redshift..) - though not dynamodb or others that return 400 + some >>> message. >>> >>> If aws were to deploy an endpoint, they'd have to decide what to do. or >>> even better, the spec could say "there is a specific error code for >>> throttling, "429: Too Many Requests" to be returned in such a situation. >>> >>> On Sat, 2 Aug 2025 at 03:32, Dennis Huo <huoi...@gmail.com> wrote: >>> >>>> PR LGTM, thanks for following up on this! >>>> >>>> Reiterating my comments in the PR, IMO especially now that Russell's >>>> https://github.com/apache/iceberg/pull/13449 is merged so that all the >>>> read-only methods can still perform low-level retries, this change makes >>>> sense as a safer default for commit behaviors. >>>> >>>> I could imagine a few potential follow-up features if people want to >>>> achieve better availability in situations where more out-of-band knowledge >>>> of specific server behaviors can be used on a case-by-case basis: >>>> >>>> 1. If anyone feels very strongly that in a given service environment >>>> some particular 5xx code really is safely retriable without reconciliation, >>>> we could consider extracting a runtime config that lists the non-retriable >>>> HTTP codes instead of only having the hard-coded switch statement coming at >>>> the cost of some code complexity >>>> 2. We could make it easier to inject a >>>> dynamically-classloaded HttpRequestRetryStrategy in the HttpClient ( >>>> https://github.com/apache/iceberg/blob/a8fdb23682a7c083ea6ff9873f1531dd9d465aa7/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java#L127) >>>> - it seems there's already some precedent for injectable helpers here such >>>> as rest.client.tls.configurer-impl -- custom impls could either be as >>>> simple as using a different set of 5xx non-retriable codes, or as complex >>>> as cooperating with other customized client-side logic to use an end-to-end >>>> idempotency request-uuid to distinguish safe retries >>>> 3. We could consider letting other response headers such as >>>> "Retry-After" ( >>>> https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Retry-After) >>>> be a heuristic to interpret the *intent* of the server to communicate a >>>> "graceful unavailability" that is safe to retry, vs an unexpected failure >>>> where state is unknown >>>> >>>> In any case though, it does seem prudent to start with the safe default. >>>> >>>> On Wed, Jul 23, 2025 at 1:22 PM Prashant Singh < >>>> prashant010...@gmail.com> wrote: >>>> >>>>> Hey All, >>>>> Presently we treat 503 status code in spec here <http:///> as if >>>>> the request was not tried at all. While doing 1.9.2 it was brought up that >>>>> this exception might not be true and some of the very common services like >>>>> Istio and Envoy were brought up as examples. >>>>> >>>>> As a result I wanted to formally bring up disabling retries on 503 on >>>>> updateTable and treating that as CommitStateUnknown similar to what we do >>>>> for 500, 502, 504. The rationale for this being 503 doesn't necessarily >>>>> mean the request was not tried at all and systems like ENVOY can return >>>>> 503 >>>>> in the middle of processing. >>>>> >>>>> Adding detailed analysis of my colleague Dennis here : >>>>> https://lists.apache.org/thread/h8xowcntomh7r5woz24cv27v3o8r9wgd >>>>> >>>>> It seemed like most people participating in the thread above agreed, >>>>> starting this thread to see if there are objections to it in a wider >>>>> forum. >>>>> >>>>> Meanwhile, I have filed a PR for the same >>>>> [1] https://github.com/apache/iceberg/pull/13619 >>>>> >>>>> Please let us know your thoughts and provide your feedback ! >>>>> >>>>> Best, >>>>> Prashant Singh >>>>> >>>>> >>>>> >>>>>
