buska88 commented on code in PR #3824:
URL: https://github.com/apache/celeborn/pull/3824#discussion_r3849780107
##########
common/src/main/java/org/apache/celeborn/common/network/util/TransportFrameDecoder.java:
##########
@@ -51,11 +51,37 @@ public class TransportFrameDecoder extends
ChannelInboundHandlerAdapter implemen
private static final int MAX_FRAME_SIZE = Integer.MAX_VALUE;
private static final int UNKNOWN_FRAME_SIZE = -1;
+ /**
+ * Netty's {@code AdaptiveRecvByteBufAllocator} caps a single {@code
channelRead} at this many
+ * bytes. Used to spot likely-large stuck frames; see {@link
#hasLikelyLargeIncompleteFrame()}.
+ */
+ public static final long MAX_SINGLE_READ_BYTES = 65536;
+
private final LinkedList<ByteBuf> buffers = new LinkedList<>();
- private long totalSize = 0;
+ // Written by the Netty I/O thread; read by checkService via
hasLikelyLargeIncompleteFrame().
+ private volatile long totalSize = 0;
private long nextFrameSize = UNKNOWN_FRAME_SIZE;
+ /**
+ * When set, this channel is resumed only to drain its stuck half-received
frame (in {@link
+ * #buffers}) and release the memory, not to resume normal traffic.
+ */
+ private volatile boolean frameDrain = false;
+
+ /** Enables single-frame drain mode: the next decoded frame fires {@link
FrameDrainCompleted}. */
+ public void enableFrameDrain() {
+ this.frameDrain = true;
+ }
+
+ /**
+ * True if the stuck frame's buffered bytes exceed one {@code channelRead}
worth ({@link
+ * #MAX_SINGLE_READ_BYTES}), making it a strong candidate for the
backpressure root cause.
+ */
+ public boolean hasLikelyLargeIncompleteFrame() {
+ return totalSize > MAX_SINGLE_READ_BYTES;
Review Comment:
Good point, thanks! You're right that for a typical 256 KiB frame paused
right after its first 64 KiB read, totalSize alone can stay under the cap and
never flag it.
Fixed in the latest commit by making hasLikelyLargeIncompleteFrame take a
byFrameSize flag and picking the criterion per module, since the "large frame"
signal means different things on each side:
For PUSH_MODULE (draining channels where this worker is the receiver), we
keep using totalSize — that's the actual bytes already buffered in this
worker's memory, so it's the most precise/direct signal for relieving this
worker's own memory pressure.
For REPLICATE_MODULE (draining channels where this worker is the sender to a
peer), we now use the decoded frame length (nextFrameSize) instead, because in
this direction the large frame is actually piling up on the peer's (primary's)
push-receiving side, not locally. Ranking by nextFrameSize correctly identifies
which peer connections are stuck behind a large frame, so draining them
unblocks the peer and relieves memory pressure on the primary side faster —
which matters a lot for us since our jobs with Gluten enabled can push frames
of several hundred MB.
Let me know if this addresses the concern, or if you'd prefer a different
split.
--
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]
