xiangfu0 commented on code in PR #20022:
URL: https://github.com/apache/pinot/pull/20022#discussion_r4364963128
##########
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:
Minor/process, non-blocking: this and the write timeout and the write-buffer
watermark rejection are all new and default ON for every broker-to-server
channel (on master these channels had none of them). The defaults look sound
and each is configurable with non-positive to disable, so no change requested —
but it's worth an explicit release note and a rolling-upgrade mention, since on
a high-RTT or lossy broker-to-server link a 10s TCP_USER_TIMEOUT could close
connections a prior deployment tolerated, and operators there may want to raise
or disable it.
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/ServerChannels.java:
##########
@@ -227,35 +418,99 @@ protected void initChannel(SocketChannel ch) {
_serverRoutingInstance));
}
});
+ if (isTcpUserTimeoutApplied()) {
+ _bootstrap.option(EpollChannelOption.TCP_USER_TIMEOUT,
_tcpUserTimeoutMs);
+ }
}
void closeChannel() {
- if (_channel != null) {
- _channel.close();
+ Channel channel = _channel;
+ if (channel != null) {
+ channel.close();
}
}
+ /// See [ServerChannels#evictChannel]. Flags the entry first, then closes
whatever channel it holds; no lock is
+ /// taken so that the routing manager's Helix callback is never blocked
behind a connect or write in progress.
+ void evict() {
+ _evicted = true;
+ Channel channel = _channel;
+ if (channel != null) {
+ closeLocally(channel);
+ }
+ }
+
+ /// Closes a channel the broker decided to drop (eviction after a restart,
replacement of a suspect channel).
+ /// The queries still in flight on it fail through `channelInactive` as on
any closed channel, but the server is
+ /// not reported for it: the close was the broker's decision, not evidence
against the server.
+ private void closeLocally(Channel channel) {
+ DataTableHandler dataTableHandler =
channel.pipeline().get(DataTableHandler.class);
+ if (dataTableHandler != null) {
+ dataTableHandler.markClosedByBroker();
+ }
+ channel.close();
+ }
+
+ void recordResponse() {
+ _lastResponseTimeMs = System.currentTimeMillis();
+ _unansweredQueries.set(0);
+ }
+
+ /// Runs on the liveness thread a grace period after a query written at
`submitTimeMs` timed out. See
+ /// [ServerChannels#recordUnansweredQuery].
+ void judgeUnansweredQuery(long submitTimeMs) {
+ // Answered something since the query was written (a fresh channel
counts as answered from its connect time, so
+ // queries written to the channel it replaced do not count against it),
or no longer in use. Strictly after:
+ // the connect and the first write can share a millisecond, while a real
answer never shares one with its
+ // request.
+ if (_evicted || _lastResponseTimeMs > submitTimeMs) {
+ return;
+ }
+ int unansweredQueries = _unansweredQueries.incrementAndGet();
+ if (unansweredQueries < _unansweredQueriesThreshold) {
+ return;
+ }
+ _unansweredQueries.set(0);
+ _suspect = true;
Review Comment:
Minor, non-blocking: there's a narrow race here. The `_lastResponseTimeMs >
submitTimeMs` guard is read once at the top of the method, but
`recordResponse()` (event loop) and `connectWithoutLocking()` (query thread)
both reset `_unansweredQueries` and bump `_lastResponseTimeMs` without ordering
against this task. If a genuine response lands between the threshold check and
this `_suspect = true`, the channel can be marked suspect and the server
reported even though it just proved itself alive — a spurious reconnect plus
one spurious failure-detector report.
The impact is bounded and self-correcting (the fresh channel is healthy, the
next probe keeps it), so this is a MINOR. A cheap fix would be to re-read
`_lastResponseTimeMs` right before setting `_suspect` and bail if it now
exceeds `submitTimeMs`.
--
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]
