xiangfu0 commented on code in PR #20022:
URL: https://github.com/apache/pinot/pull/20022#discussion_r4366342562
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/ServerChannels.java:
##########
@@ -264,17 +565,39 @@ void sendRequest(String rawTableName, AsyncQueryResponse
asyncQueryResponse,
}
}
+ private void checkNotEvicted() {
+ if (_evicted) {
+ closeChannel();
+ throw new IllegalStateException("Channel to server: " +
_serverRoutingInstance + " has been evicted");
+ }
+ }
+
/// Lazy query path: opens the TCP connection only. Any TLS handshake is
left to proceed
/// asynchronously so the channel lock is released as soon as the socket
is up, keeping the first
/// query's critical section short. Startup pre-connect uses
[#preConnectWithoutLocking(long)]
/// instead, which additionally pays the handshake.
+ ///
+ /// Also replaces a channel marked suspect: the server stopped answering
on it while the socket still looked
+ /// active, which is what a half-open socket looks like until the kernel
gives up on it, so `isActive()` is not
+ /// trusted. The suspect channel is dropped before the connect, so a
failed connect does not leave it in place,
+ /// and whatever is still in flight on it fails without the server being
reported (nothing on it was being
+ /// answered anyway).
void connectWithoutLocking()
throws InterruptedException {
- if (_channel == null || !_channel.isActive()) {
- long startTime = System.currentTimeMillis();
- _channel = _bootstrap.connect().sync().channel();
- recordConnectTime(System.currentTimeMillis() - startTime);
+ Channel channel = _channel;
+ if (channel != null && channel.isActive() && !_suspect) {
+ return;
+ }
+ if (channel != null && _suspect) {
+ LOGGER.info("Replacing suspect channel to server: {} with a fresh
connection", _serverRoutingInstance);
+ _channel = null;
+ closeLocally(channel);
}
+ _suspect = false;
+ long startTime = System.currentTimeMillis();
+ _channel = _bootstrap.connect().sync().channel();
+ resetLiveness();
Review Comment:
Small window here: `_suspect = false` on line 596 is cleared before the
(potentially slow) `connect().sync()`, and `resetLiveness()` only bumps
`_lastResponseTimeMs` afterward. A stale `judgeUnansweredQuery` for an old
query landing during the connect could see `!_suspect` + an un-reset
`_lastResponseTimeMs <= submitTimeMs` and re-mark the brand-new channel suspect
(and even report the server). It self-corrects on the next send, but if it
crosses the threshold it costs a spurious exclusion. Worth clearing `_suspect`
under `_livenessLock` together with the reset?
--
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]
