buska88 commented on code in PR #3824:
URL: https://github.com/apache/celeborn/pull/3824#discussion_r3900232727
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -411,6 +443,41 @@ public void switchServingState() {
}
}
+ /**
+ * Fires a drainIncompleteFrame tick when backpressure is active and
appActiveMemory is at/below
+ * the watermark.
+ */
+ private void checkAndTriggerDrainIncompleteFrame() {
+ if (!drainIncompleteFrameEnabled) return;
+
+ long appActiveMemory =
+ sortMemoryCounter.get()
+ + diskBufferCounter.get()
+ + memoryFileStorageCounter.sum()
+ + pendingReplicateBytesCounter.get();
Review Comment:
Good catch, thanks! You're right that pendingReplicateBytesCounter alone
crossing the watermark shouldn't gate draining, since it only decrements via
replication ack callbacks, which in turn depend on this worker's own channels
being resumed to read those acks — a circular dependency that never resolves on
its own.
Fixed in the latest commit: we now split the watermark check into two
independent gates —
PUSH_MODULE drains only when localActiveMemory +
pendingReplicateBytesCounter is at/below the watermark (unchanged, full
accounting), and
REPLICATE_MODULE drains based on localActiveMemory alone (excluding
pendingReplicateBytesCounter), since that counter reflects a
cluster-wide/peer-dependent state rather than this worker's own memory
footprint, and letting it block REPLICATE_MODULE would recreate exactly the
deadlock you described.
This way, even if pendingReplicateBytesCounter stays high, the replicate
channels can still be drained first to unblock the peer's push side, which lets
acks flow back and the counter naturally fall.
--
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]
