hudi-agent commented on code in PR #20186: URL: https://github.com/apache/hudi/pull/20186#discussion_r4161186941
########## rfc/rfc-91/rfc-91.md: ########## @@ -76,6 +76,48 @@ 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 in-cycle retry, the next heartbeat's renewal, and `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. | Review Comment: 🤖 The tryLock adoption test treats the lock as valid while expiration plus the drift buffer is in the future. That is the lenient bound other writers use before taking over a lock. For a holder, shouldn't the bound be the strict one, expiration minus the buffer, with enough lease left to renew before anyone else could take over? @nsivabalan could you check whether this keeps mutual exclusion when clocks are skewed? <sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub> ########## rfc/rfc-91/rfc-91.md: ########## @@ -76,6 +76,48 @@ 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 in-cycle retry, the next heartbeat's renewal, and `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. Review Comment: 🤖 Could you spell out the retry bounds (count, backoff, total time) and how they relate to the heartbeat interval and lock validity? Are they new configs or hardcoded? And for tryLock, does each retry write the same expiration, or a new one? The table matches against 'the expiration this call wrote'. <sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub> ########## rfc/rfc-91/rfc-91.md: ########## @@ -76,6 +76,48 @@ 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. Review Comment: 🤖 Have you considered writing a per-write nonce (or attempt ID) into the lock file, instead of telling writes apart by owner and expiration? The match would then be exact and wouldn't depend on the clock. It might be worth adding a short 'Alternatives considered' paragraph either way. <sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub> ########## rfc/rfc-91/rfc-91.md: ########## @@ -76,6 +76,48 @@ 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. Review Comment: 🤖 The whole rule depends on no other instance ever writing the same owner UUID. Can you confirm the owner is generated per provider instance and not taken from config, a hostname or a job ID? And what happens if one instance runs tryLock/unlock cycles back to back while an earlier write is still unresolved? <sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub> ########## rfc/rfc-91/rfc-91.md: ########## @@ -76,6 +76,48 @@ 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. Review Comment: 🤖 How does the design handle an ambiguous write that is still in flight and lands after the read-back? Is the read-back guaranteed to see the write's final outcome, or can the decision be made on stale state? <sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub> ########## rfc/rfc-91/rfc-91.md: ########## @@ -76,6 +76,48 @@ 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 in-cycle retry, the next heartbeat's renewal, and `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. | +| `renewLock()` | `expired = false` and an expiration later than the one we hold | A renewal landed. Adopt it and keep the heartbeat running. | +| `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 renew or 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. + - `renewLock()` treats the lock as lost: the heartbeat stops and the writer thread is interrupted. + - `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. +- **Clock.** The renewal comparison uses this writer's own clock. A backward clock step can make it reject our own landed renewal. That fails safe: the renewal is reported as lost and the writer thread is interrupted. +- **Shutdown hook and heartbeat-start failure.** These release paths stay single-attempt and best-effort. +- **Per-client classification.** Each storage client must map timeouts and dropped connections to an ambiguous result, rather than rethrowing them or reporting them as unknown and unretried. The same applies to the S3 409 above. On Azure, a 409 on create means the blob already exists, so it is not a retriable conflict. Review Comment: 🤖 Could you add a short section on observability and testing, such as metrics or logs for ambiguous results, read-backs and adoptions, and how the per-client classification will be checked with fault injection (timeouts, 5xx, dropped connections)? <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]
