xiangfu0 commented on code in PR #20022:
URL: https://github.com/apache/pinot/pull/20022#discussion_r4369091554
##########
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:
Good point; the Javadoc now says so (6778a0f850): `true` includes the
vacuous case of a server with no queried channel, so a caller that needs
"reachable" has to check `hasChannel` first, as `retryUnhealthyServer` does.
##########
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:
Right, and I kept it deliberately. The lock is per channel entry, and the
response side always runs on that channel's single event loop, so the only
other takers are the connect path when a connection is replaced and the
liveness thread's judgment, both rare. The critical section is two field
writes. In exchange it is what makes a response that lands while a judgment
runs impossible to lose, which the earlier unsynchronized version could do. An
uncontended monitor on that path costs a few nanoseconds against a response
that has just been deserialized.
##########
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:
Confirmed, that is the intent, and it is now called out in the description's
release note: on by default (threshold 5, grace 2 s); on brokers with pings
off, a channel that leaves five graced timeouts unanswered gets the server
reported to the failure detector, which with the `CONNECTION` detector takes it
out of routing until the retry probe re-establishes the channel, and with the
default `NO_OP` detector only replaces the channel. A threshold of 0 or less
turns it off.
--
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]
