HDDS-16335. Change XceiverClientShortCircuit to support concurrent access - #11225
HDDS-16335. Change XceiverClientShortCircuit to support concurrent access#11225echonesis wants to merge 2 commits into
Conversation
echonesis
changed the title
Gargi-jais11
left a comment
Gargi-jais11
left a comment
There was a problem hiding this comment.
Thanks @echonesis for working on the PR. Left few review comemnts
| if (failure != null) { | ||
| LOG.error("Failed to send command {}", request, failure); | ||
| for (RequestEntry requestEntry : pending) { | ||
| requestEntry.fail(failure); | ||
| } | ||
| metrics.decrPendingContainerOpsMetrics(request.getCmdType()); | ||
| metrics.addContainerOpsLatency(request.getCmdType(), System.nanoTime() - entry.getCreateTimeNs()); |
There was a problem hiding this comment.
In sendRequest(), when the write faills we are currently fail all entries in pending, but metrics.decrPendingContainerOpsMetrics() and addContainerOpsLatency() are only called once for the current request, so other in-flight requests will leave infleted pending metrics.
| readDaemon.join(); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| pending.forEach(entry -> entry.fail(new ClosedChannelException())); |
There was a problem hiding this comment.
here when the close fails, we are not tracking any metrics failures.
| lock.unlock(); | ||
| } | ||
| if (entry != null) { | ||
| entry.getFuture().completeExceptionally(e); |
There was a problem hiding this comment.
In the generic catch (Throwable e) block,
if entry != null you are calling the entry.getFuture().completeExceptionally(e) and then pending.forEach(i -> i.fail(e)). If entry is still in sentRequests, it gets failed twice. CompletableFuture ignores the second completion, so this is harmless but redundant.
| } | ||
| readDaemon.interrupt(); | ||
| } | ||
| pending = new ArrayList<>(sentRequests.values()); |
There was a problem hiding this comment.
Nit: After failing pending requests on close, consider clearing sentRequests under the lock.
Similarly in sendRequests and recieveResponseTask
2 participants
What changes were proposed in this pull request?
XceiverClientShortCircuitdid not consistently synchronize access to its connection state and other non-final fields.This pull request uses the existing lock to protect all non-final fields and serialize connection setup, request registration and writes, and close transitions. Blocking reads, response waits, and receiver joins remain outside the lock.
Thread-safe final fields are used for response and timeout processing so they can proceed while a socket write is blocked. The client also remains one-shot: repeated
connect()calls are allowed while open, but reconnecting after failure or close is rejected.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16335
How was this patch tested?
Local Test
GitHub Actions CI: https://github.com/echonesis/ozone/actions/runs/34445616895
Generated-by: Codex (GPT-5)