xiangfu0 commented on code in PR #20022:
URL: https://github.com/apache/pinot/pull/20022#discussion_r4368892259
##########
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:
Fixed, and closed from both sides. The replacement now starts a clean
liveness record (not suspect, nothing unanswered, last response set to now)
under `_livenessLock` before it connects, and again once connected, so a
judgment that runs while the connect is in progress sees the reset record
instead of the stale one. On top of that, each timed-out query now carries the
channel it was written to, and a judgment only counts a query whose channel is
still the one the entry holds, so a query written to the dropped channel can
never count against its successor, however the timing falls.
`ServerChannelsTest#testJudgmentOfAQueryOnAReplacedChannelDoesNotCount` covers
it with a threshold of 1, where a single counted judgment would mark the fresh
channel suspect at once.
--
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]
