xiangfu0 commented on code in PR #20021:
URL: https://github.com/apache/pinot/pull/20021#discussion_r4366341592
##########
pinot-common/src/main/java/org/apache/pinot/common/config/NettyConfig.java:
##########
@@ -18,13 +18,56 @@
*/
package org.apache.pinot.common.config;
+import com.google.common.base.Preconditions;
import org.apache.pinot.spi.env.PinotConfiguration;
/// Container object for netty configuration of pinot clients and servers
(netty, grizzly, etc.)
+///
+/// Every setting is read as `<namespace>.<suffix>`, where the namespace is
`pinot.broker.netty` on the broker and
+/// `pinot.server.netty` on the server. The channel liveness settings below
only apply to the client side, i.e. the
+/// broker-to-server channels opened by `ServerChannels`; the server-side
`QueryServer` ignores them.
public class NettyConfig {
private static final String NATIVE_TRANSPORTS_ENABLED =
"native.transports.enabled";
+
+ /// Upper bound on how long a request write may stay pending before the
channel is closed, in milliseconds. A write
+ /// is pending while the kernel has not accepted the bytes, which on a
half-open or stalled socket is forever (the
+ /// peer stops acknowledging, the send buffer fills, and `isActive()` keeps
reporting `true` until the kernel gives
+ /// up on retransmits, typically after many minutes). Closing the channel
fails the in-flight queries on it fast
+ /// and lets the failure detector quarantine the server. Non-positive
disables the check. Broker key:
+ /// `pinot.broker.netty.write.timeout.ms`.
+ public static final String WRITE_TIMEOUT_MS = "write.timeout.ms";
+ public static final long DEFAULT_WRITE_TIMEOUT_MS = 10_000L;
+
+ /// Linux `TCP_USER_TIMEOUT`, in milliseconds: how long transmitted data may
remain unacknowledged before the
+ /// kernel closes the connection, replacing the default retransmission
timeout of several minutes. Covers the
+ /// case the write timeout cannot see: small writes that the kernel accepted
into a send buffer that the peer never
+ /// drains. Only applied when the broker uses the Epoll transport
(`native.transports.enabled` on Linux); NIO and
+ /// KQueue have no such socket option. Non-positive leaves the kernel
default. Broker key:
+ /// `pinot.broker.netty.tcp.user.timeout.ms`.
+ public static final String TCP_USER_TIMEOUT_MS = "tcp.user.timeout.ms";
+ public static final int DEFAULT_TCP_USER_TIMEOUT_MS = 10_000;
Review Comment:
Worth calling out in release notes: on upgrade these default to on for every
broker (10s write timeout, 10s TCP_USER_TIMEOUT on Epoll, 16MB/8MB watermarks),
which is a behavior change from the previous implicit "no limits". The 10s
TCP_USER_TIMEOUT in particular will abort a connection if a server GC-pauses
past 10s mid-request. That's arguably the right call (reconnect beats hanging),
just flagging it as a notable new default rather than a bug.
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/ServerChannels.java:
##########
@@ -143,10 +170,53 @@ public void sendRequest(String rawTableName,
AsyncQueryResponse asyncQueryRespon
ServerRoutingInstance serverRoutingInstance, InstanceRequest
instanceRequest, long timeoutMs)
throws Exception {
byte[] requestBytes =
THREAD_LOCAL_T_SERIALIZER.get().serialize(instanceRequest);
- ServerChannel serverChannel =
_serverToChannelMap.computeIfAbsent(serverRoutingInstance, ServerChannel::new);
- // This server is now query-carrying, whoever opened the channel. See
[#hasChannel].
- serverChannel._openedByQuery = true;
- serverChannel.sendRequest(rawTableName, asyncQueryResponse,
serverRoutingInstance, requestBytes, timeoutMs);
+ for (int attempt = 1; attempt <= MAX_SEND_ATTEMPTS; attempt++) {
Review Comment:
Minor: each retry re-acquires the lock with the full `timeoutMs`, so a
pathological case could block up to 3x the timeout before giving up. Eviction
is rare and racing it three times running is essentially impossible, so this is
fine in practice; just noting the worst-case budget isn't bounded by the
caller's single `timeoutMs`.
--
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]
