hudi-agent commented on code in PR #20186:
URL: https://github.com/apache/hudi/pull/20186#discussion_r4235903998
##########
rfc/rfc-91/rfc-91.md:
##########
@@ -76,6 +76,47 @@ Once a lock is acquired, a dedicated heartbeat task
periodically calls renewLock
- Clock drift: we allow for a maximum of 500ms of clock drift between nodes. A
requirement of this lock provider is that all writers competing for the same
lock must be writing from the same cloud provider (AWS/Azure/GCP).
- This will not be configurable at this time. If a storage-specific
implementation needs to customize this the config can be added at that time but
it should never go below 500ms.
+### Ambiguous write results
+
+The operations above treat each conditional write as having a definitive
answer:
+- success;
+- 412, the file changed since we read or wrote it;
+- 429, throttled and not applied, retried with backoff;
+- on S3, 409 ConditionalRequestConflict, not applied and to be retried.
+
+A write can also come back with an *ambiguous* answer, where storage does not
say whether it applied the write: a server-side error (HTTP 5xx), a client-side
timeout, or a dropped connection. The write may have landed, so the provider
must not assume either way. SDK-level retries are disabled in all three storage
clients, so the provider sees every such answer.
+
+The lock file makes this resolvable. Each provider instance has a random
`owner` UUID and no other instance writes it. A lock file that carries our
owner was therefore written by this instance.
+
+**Rule.** After an ambiguous answer, the provider retries the operation with
the same precondition (bounded attempts, with backoff). It also remembers that
the write is unresolved until a definitive answer arrives. While a write is
unresolved, a 412 on any later operation does not count as another writer's
write straight away: that covers the retry and a later `unlock()`. The same
goes for a lock that looks held on the next acquire attempt. The provider first
reads the lock file back. Only if the read-back does not match one of the rows
below is the result treated as another writer's.
+
+| Operation | The read-back is our owner, and... | Outcome |
+|---|---|---|
+| `tryLock()` | `expired = false`, still valid (expiration plus the clock
drift buffer is in the future), and the expiration this call wrote | The
acquire write landed. Adopt that lock file, including its version tag, and
start the heartbeat. |
+| `unlock()` | `expired = true` and the same expiration as the lock being
released | The release landed. The release succeeded. |
+
+Any other read-back keeps the existing behaviour:
+- **A different owner** is not proof that our earlier write landed. On unlock
it is reported as the lock being acquired by others; on acquire it means the
lock is held.
+- **A failed read** (the read fails or finds no lock file) leaves the outcome
unknown, and the result is treated as another writer's write:
+ - `tryLock()` reports the lock as not acquired. If our acquire write did
land, that lock has no heartbeat and stays held until its expiration.
+ - `unlock()` reports the release as failed and clears the local lock state.
The lock file then belongs to another writer, or is ours until its expiration.
+
+Notes:
+- **Version tags.** The adopted lock file's version tag comes from the same
read as its content, so it is a valid precondition for the next write.
+ - S3: the ETag and the content come from one GetObject response.
+ - Azure: the ETag and the content come from one download response.
+ - GCS: the content read is pinned to the generation returned with the
metadata.
+- **Renewal is out of scope.** A renewal that lands behind an ambiguous answer
is not read back: the next renewal's precondition fails and the lock is treated
as lost, as before. That fails safe, since the heartbeat stops and the writer
thread is interrupted.
Review Comment:
🤖 non-blocking: Renewal runs every heartbeat, so it is the write most likely
to hit an ambiguous result, and leaving it out means a landed renewal still
aborts the writer; could you add a sentence on why it is out of scope, and
narrow line 118 ("prevents a writer giving up a lock it actually holds") so it
covers only acquire and release?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
