linliu-code commented on PR #20028: URL: https://github.com/apache/hudi/pull/20028#issuecomment-5942473355
Thanks @yihua, fair ask. I went back to RFC-91 and walked every conditional write the provider makes, instead of only the release path this PR started from. Summary first, then the details. **The gap is in the RFC's model, not in one code path.** RFC-91 assumes every conditional write gets a definitive answer: 2xx (written), 412 (someone else changed the file), or 409 (retry). In practice a write can also come back with an *ambiguous* answer: a 5xx, a client timeout, or a dropped connection. The write may or may not have landed. The RFC has no rule for this, so each code path handles it differently today, and some of those paths turn our own landed write into a false "lock lost". **What makes it solvable is already in the RFC's lock file design.** Every lock file carries the writer's `owner` UUID (unique per provider instance), its `expiration`, and the `expired` flag. Because only this provider instance ever writes its own UUID, the writer can resolve any ambiguous write by reading the lock file back: if storage holds a lock with our owner and the content our write intended, then the write landed. If the file holds anything else, it did not land, or someone else wrote after it. ### Every conditional write, on an ambiguous answer The S3 client runs with `maxAttempts(1)`, so the SDK never retries. The provider's handling is the only retry layer. | Write (RFC-91 op) | master today | with this PR | still open | |---|---|---|---| | **Acquire** (create, or overwrite an expired lock) | 5xx maps to UNKNOWN_ERROR; `tryLock(time, unit)` loops every 1s until the timeout. If the write **did not land**, the next pass acquires. If it **did land**, the next pass reads our own valid lock and reports "Lock already held by <self>". That lock has no heartbeat, so every writer, including us, waits out the full lease, and it then shows up as a dangling lock. | 5xx maps to TRANSIENT_ERROR, plus one inner retry. **The landed case is unchanged; @voonhous is right.** The not-landed case was already covered by the outer 1s loop, so the inner retry adds little here. | Reconcile: if the stored lock is unexpired and carries our owner, adopt it and start the heartbeat. | | **Renew** (heartbeat) | 5xx maps to UNKNOWN_ERROR: keep the heartbeat and try again next cycle (30s). If the write **landed**, the next cycle's precondition fails with 412, the provider reads that as "acquired by others", stops the heartbeat and interrupts the writer thread. The commit is aborted even though we still hold a valid lease. | One in-cycle retry; on 412 after a retriable error, read back and adopt our own landed renewal (3bdc6d3). Fixes the single-5xx case. | The reconcile is gated on "the previous attempt in this cycle was retriable", so a 5xx followed by another 5xx on the retry, or an UNKNOWN_ERROR that landed, still ends in a false steal next cycle (the hudi-agent comment on `:853`). Drop the gate: an unexpired lock with our owner and a later expiration can only be one of our own renewals. | | **Release** (mark `expired`) | 5xx maps to UNKNOWN_ERROR and is not retried: the lock stays held until its lease ends, and is reported as dangling. This is the production failure behind this PR (GCS 503 on the release write). | 5xx is retried up to 3 times with backoff. A 412 on the retry reads back; if storage holds our own expired lock, the earlier attempt landed and the release counts as successful. | UNKNOWN_ERROR (timeout, connection reset) on release is still not retried; #20129 adds that. A foreign owner is still reported as a steal (kept deliberately: it is not proof our release landed). | | **Shutdown hook and heartbeat-start failure** (release) | Single attempt. | Unchanged. | Out of scope; best effort by design. | Two smaller divergences from the RFC turned up along the way: - The RFC says a 409 ConditionalRequestConflict on S3 should be retried. The S3 client logs it as "Retriable" but returns UNKNOWN_ERROR, both on master and in this PR. - The renewal reconcile compares `validUntil` with our previous expiration, which assumes our clock never steps back. If NTP steps it back, the reconcile rejects our own landed renewal. That fails safe (a false steal), but it is worth a comment. ### Why it fails safe today, and what it costs None of these paths can let two writers run at once: every write is conditional on the version we last read, and a false "lock lost" interrupts the writer thread (`LockProviderHeartbeatManager`, heartbeat failure path). What these paths cost is availability: - every writer stalls for up to one lease (300s by default); - an aborted commit or table service; - a dangling-lock alert. That cost is what we see in production. ### Proposal One rule for all three write paths, now written up as an RFC-91 update in #20186: **after an ambiguous answer, the provider remembers the write as unresolved and, until a definitive answer arrives, checks any 412 or "held" result against our own owner before treating it as foreign.** That includes the next heartbeat cycle and `unlock()`, not only the in-cycle retry. - Acquire: our owner, unexpired, still valid by the same test other writers use, and the expiration this call wrote: adopt and start the heartbeat. - Renew: our owner, unexpired, with a later expiration: adopt. - Release: our owner, expired, with the same expiration as the lock being released: release succeeded. Everything else keeps the current RFC behaviour, including reporting a foreign owner as a steal. I'd like us to agree on #20186 before I touch the code again. Once it's agreed, I'll rework this PR to implement the rule rather than keep adding per-path fixes. The UNKNOWN_ERROR release retry stays in #20129, which stacks on this one. Does that direction look right to you? -- 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]
