danny0405 commented on issue #19902: URL: https://github.com/apache/hudi/issues/19902#issuecomment-5628930290
The root-cause analysis makes sense. I checked the coordinator and EventBuffers on current master (`a46909b09bbb`). I would suggest a smaller first fix based on the standard asynchronous request–reply pattern: **start once, return PENDING promptly, and poll the same operation until it is READY**. The key distinction is that instant creation is already offloaded to an executor, but the coordination RPC still waits for it to finish. Moving creation to another executor alone does not fix that coupling. A minimal design could be: 1. Make the RPC handler a fast in-memory lookup/registration path that returns `READY(instant)` or `PENDING`. It must not wait on prior commits, table locks, or an unfinished future. 2. Reuse the existing `instantRequestExecutor` as the serialized creation worker, and serve status independently through thread-safe operation state. This avoids adding a third executor. 3. Atomically register one operation per checkpoint within the current coordinator generation **before** submitting creation. Concurrent writers and repeated polls share that operation; polling must never enqueue duplicate creation. 4. Keep the prior-commit wait and `startInstant()` on the worker. Install the event buffer before publishing READY, and preserve the checkpoint-to-instant mapping so a lost response does not cause another instant to be created. 5. Let writers poll with capped backoff and jitter, with one RPC outstanding per writer and a bounded overall wait budget. Transient transport retries must reuse the same checkpoint identity; creation failures should remain terminal and propagate through the normal failure path. I would leave phase/progress tokens and server-side long polling out of the initial fix. A healthy operation waiting for a cleaner's lock may show no progress at all, while unrelated activity could advance a global progress token without helping this request. A monotonic overall deadline is simpler and easier to reason about. The existing `write.commit.ack.timeout` is worth evaluating as the wait budget, checking its suitability for both blocking and non-blocking modes; PENDING responses must not reset that deadline. A few correctness details still need explicit coverage: - **Ordering:** If retaining CommitGuard on the worker, wait in a predicate loop with the remaining deadline. The current `blockFor(String)` only waits for a signal; a wakeup does not establish that every required prior commit completed. The supplier-based overload is a useful starting point. Also, replacing the existing gate with `getPendingInstantsBefore(cid).isEmpty()` changes how empty buffers are treated, so that should be validated rather than assumed behavior-preserving. - **Recovery/close:** Fence results by coordinator generation and coordinate recovery with the running worker. Cancelling a future or ignoring its result alone does not stop `startInstant()` from mutating shared clients or the timeline. Prevent old work from racing with restored state or closed clients. - **Failure/idempotency:** Do not automatically retry creation after a potentially partial failure. Retire operation records with checkpoint lifecycle handling, and reject stale requests rather than interpreting them as new work. - **Validation:** Reject mismatched checkpoint/instant events through normal coordinator/job failure handling; introducing JVM exit seems unnecessary for this fix. The main integration test should hold the lock longer than the configured RPC timeout but shorter than the operation/checkpoint budgets, and verify prompt polling responses, no restart, eventual success, and exactly one instant across concurrent writers. Lost responses, creation failure, and recovery during creation are the other essential cases. This addresses lock-induced RPC timeouts while keeping the existing serialized creation model. It cannot guarantee success when contention exceeds the lock-acquisition or checkpoint deadline; those remain legitimate failure boundaries. This is a source-based design review, not a tested patch. -- 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]
