xiangfu0 commented on code in PR #20022:
URL: https://github.com/apache/pinot/pull/20022#discussion_r4369050243
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/QueryRouter.java:
##########
@@ -177,18 +203,17 @@ public boolean hasChannel(ServerInstance serverInstance) {
return !channelRoutingInstances(serverInstance).isEmpty();
}
- /// Connects to the given server, returns `true` if the server is
successfully connected.
- ///
- /// This is the reachability probe the failure detector uses when pings are
off, opening the OFFLINE channel with a
- /// TCP connect only. Startup pre-connect uses [#preConnect] instead.
+ /// The reachability probe the failure detector uses when pings are off.
Re-establishes every channel this broker
+ /// has queried the server over, one per table type: reconnects the ones
that are down and replaces the ones marked
+ /// suspect (see [ServerChannels#recordUnansweredQuery]) with a fresh
connection, so that a half-open socket that
+ /// still reports active is never vouched for. Returns `true` if every such
channel is connected afterwards. It used
+ /// to open the OFFLINE channel only, so on a realtime-only cluster a
handshake to a freshly restarted process passed
+ /// the probe while the channel that carried the queries was dead. Startup
pre-connect uses [#preConnect] instead.
public boolean connect(ServerInstance serverInstance) {
try {
- if (_serverChannelsTls != null) {
- _serverChannelsTls.connect(
- serverInstance.toServerRoutingInstance(TableType.OFFLINE,
ServerInstance.RoutingType.NETTY_TLS));
- } else {
- _serverChannels.connect(
- serverInstance.toServerRoutingInstance(TableType.OFFLINE,
ServerInstance.RoutingType.NETTY));
+ ServerChannels serverChannels = channels();
+ for (ServerRoutingInstance routingInstance :
channelRoutingInstances(serverInstance)) {
+ serverChannels.probe(routingInstance);
}
return true;
Review Comment:
Minor: when `channelRoutingInstances` is empty this returns `true`
vacuously. It's safe today because the only caller (`retryUnhealthyServer`)
guards with `hasChannel` and returns UNKNOWN first, but the javadoc says "true
if every such channel is connected" — maybe note that no channels also yields
true, so a future caller doesn't read it as "reachable".
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1389,6 +1389,21 @@ public enum Type {
/// stop-the-world GC pause expected on a server, or a pause will
briefly take that server out of routing.
public static final String CONFIG_OF_PING_TIMEOUT_MS =
"pinot.broker.failure.detector.ping.timeout.ms";
public static final long DEFAULT_PING_TIMEOUT_MS = 5_000L;
+
+ /// Single-stage queries a server leaves unanswered are also counted per
channel, with or without pings. A query
+ /// counts once its timeout has passed and, a further grace period
later, the server has still sent nothing on
+ /// the channel since the query was written to it. The grace lets the
server's own late timeout response through,
+ /// so a burst of slow queries or a broker pause does not look like a
dead channel; a half-open channel answers
+ /// nothing, ever. At the threshold the channel is marked suspect, so
the next query to it (or the failure
+ /// detector's retry) replaces it with a fresh connection. With pings
disabled, the server is also reported to
+ /// the failure detector then; with pings enabled, the ping that follows
every timeout makes that call instead.
+ /// Any response from the server resets the count. A non-positive
threshold disables the check.
+ public static final String CONFIG_OF_UNANSWERED_QUERIES_THRESHOLD =
+ "pinot.broker.failure.detector.unanswered.queries.threshold";
+ public static final int DEFAULT_UNANSWERED_QUERIES_THRESHOLD = 5;
Review Comment:
Confirming intent: threshold defaults to 5 (> 0), so this check is on by
default. With pings off, a channel that leaves 5 graced timeouts unanswered now
gets the server reported to the failure detector (out of routing until the
retry probe). That's the feature, just flagging it's a default-on behavior
change for existing ping-off brokers.
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/QueryRouter.java:
##########
@@ -330,12 +380,38 @@ public boolean preConnect(ServerInstance serverInstance,
TableType tableType, lo
}
}
+ /// Closes and forgets every cached channel to the given server: both table
types, over TLS and plaintext. Returns
+ /// the number of channels evicted.
+ ///
+ /// Called through `BrokerRequestHandler#onServerEnabled` when the routing
manager enables a server, which is how a
+ /// restart shows up through Helix, and through
`BrokerRequestHandler#onServerRemoved` when a server leaves the
+ /// cluster. See [ServerChannels#evictChannel] for why the cached channel
cannot be trusted at that point.
+ public int evictChannels(ServerInstance serverInstance) {
+ int numEvicted = 0;
+ for (TableType tableType : TableType.values()) {
+ if (serverInstance.getPort() > 0 && _serverChannels.evictChannel(
+ serverInstance.toServerRoutingInstance(tableType,
ServerInstance.RoutingType.NETTY))) {
+ numEvicted++;
+ }
+ if (_serverChannelsTls != null && serverInstance.getNettyTlsPort() > 0
&& _serverChannelsTls.evictChannel(
+ serverInstance.toServerRoutingInstance(tableType,
ServerInstance.RoutingType.NETTY_TLS))) {
+ numEvicted++;
+ }
+ }
+ return numEvicted;
+ }
+
public void shutDown() {
_serverChannels.shutDown();
+ if (_serverChannelsTls != null) {
+ _serverChannelsTls.shutDown();
+ }
}
void receiveDataTable(ServerRoutingInstance serverRoutingInstance, DataTable
dataTable, int responseSize,
int deserializationTimeMs) {
+ // Any response proves the server and its channel alive, whether or not
the query it answers is still waiting
+
serverChannelsFor(serverRoutingInstance).recordResponse(serverRoutingInstance);
Review Comment:
Minor/heads-up: `recordResponse` takes `_livenessLock` on the event loop for
every response now, where before this path was lock-free. The section is tiny
and per-channel (single event-loop thread per channel), so contention is
negligible, but worth being aware of on the hot response path.
--
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]
