gortiz commented on code in PR #18519:
URL: https://github.com/apache/pinot/pull/18519#discussion_r3273927611
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/channel/ChannelManager.java:
##########
@@ -62,18 +63,31 @@ public class ChannelManager {
private final PooledByteBufAllocator _bufAllocator;
@Nullable
private final SslContext _clientSslContext;
+ private final int _writeBufferHighWaterMarkBytes;
+ private final int _writeBufferLowWaterMarkBytes;
/**
* Constructs a {@code ChannelManager}.
*
* @param clientSslContext optional cached client {@link SslContext} to
reuse across channels
* @param maxInboundMessageSize maximum inbound message size for gRPC
channels
* @param idleTimeout idle timeout for gRPC channels; channels close after
this period of inactivity
+ * @param writeBufferHighWaterMarkBytes Netty per-channel {@link
WriteBufferWaterMark} high watermark. This limit is
+ * per {@code (host, port)} peer and is
shared across all streams multiplexed on
+ * that channel. The low watermark must
be ≤ the high watermark; misordered values
+ * will be rejected by Netty at channel
construction.
+ * @param writeBufferLowWaterMarkBytes Netty per-channel {@link
WriteBufferWaterMark} low mark. Once the channel's
+ * pending write queue grows above the
high watermark, the channel is marked
+ * unwritable; it becomes writable again
only when the queue drains below this
+ * low watermark. Must be ≤ {@code
writeBufferHighWaterMarkBytes}.
*/
- public ChannelManager(@Nullable SslContext clientSslContext, int
maxInboundMessageSize, Duration idleTimeout) {
+ public ChannelManager(@Nullable SslContext clientSslContext, int
maxInboundMessageSize, Duration idleTimeout,
+ int writeBufferHighWaterMarkBytes, int writeBufferLowWaterMarkBytes) {
_clientSslContext = clientSslContext;
_maxInboundMessageSize = maxInboundMessageSize;
_idleTimeout = idleTimeout;
+ _writeBufferHighWaterMarkBytes = writeBufferHighWaterMarkBytes;
+ _writeBufferLowWaterMarkBytes = writeBufferLowWaterMarkBytes;
Review Comment:
Done in `1d29438dc0`. Two validations added:
1. **`ChannelManager`**: eagerly constructs the `WriteBufferWaterMark` in
the constructor (so Netty's own `0 <= low <= high` check fires at startup, not
on first `getChannel`), plus an explicit
`Preconditions.checkArgument(writeBufferLowWaterMarkBytes > 0, …)` for the `low
= 0` degenerate case Netty accepts. The two int fields collapsed into a single
`_writeBufferWaterMark` field reused across all peers.
2. **`GrpcMailboxServer`**:
`Preconditions.checkArgument(_flowControlWindowBytes >= maxInboundMessageSize,
…)`, sitting next to the existing `_inboundMessageCredit > 0` check.
Both match the precondition-with-config-key-name error-message style used
for `_inboundMessageCredit`.
Side effect: the new validation caught `GrpcSenderBackpressureTightGateTest`
on first run — it was overriding `flowControlWindow` to `65535` without also
shrinking `maxInboundMessageSize` (default 16 MiB). The test now shrinks both
together so the application gate, not the new validation, is what bounds the
sender. The fact that the validation flagged a real (test-time)
misconfiguration on first run is reassuring — exactly what fail-fast is for.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
