Copilot commented on code in PR #17989:
URL: https://github.com/apache/pinot/pull/17989#discussion_r2997779090
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/QueryServer.java:
##########
@@ -167,23 +173,37 @@ protected void initChannel(SocketChannel ch) {
}
public void shutDown() {
+ _shuttingDown = true;
try {
- _channel.close().sync();
+ if (_channel != null) {
+ _channel.close().sync();
+ }
} catch (Exception e) {
throw new RuntimeException(e);
} finally {
- // Explicitly close all client channels and wait for completion so that
remote peers detect
- // the shutdown promptly (Netty 4.2 no longer force-closes channels
during shutdownGracefully
- // with a zero timeout).
- for (SocketChannel ch : _allChannels.keySet()) {
- try {
- ch.close().sync();
- } catch (Exception e) {
- LOGGER.warn("Failed to close client channel: {}", ch, e);
+ closeAcceptedChannels();
+ _workerGroup.shutdownGracefully(0, 0,
TimeUnit.SECONDS).syncUninterruptibly();
+ _bossGroup.shutdownGracefully(0, 0,
TimeUnit.SECONDS).syncUninterruptibly();
+ }
+ }
+
+ private void closeAcceptedChannels() {
+ long deadlineMs = System.currentTimeMillis() + CHANNEL_SHUTDOWN_TIMEOUT_MS;
+ while (!_allChannels.isEmpty() && System.currentTimeMillis() < deadlineMs)
{
+ SocketChannel[] channels = _allChannels.keySet().toArray(new
SocketChannel[0]);
+ if (channels.length == 0) {
+ break;
+ }
+ for (SocketChannel ch : channels) {
+ long remainingMs = Math.max(1L, deadlineMs -
System.currentTimeMillis());
+ if (!ch.close().awaitUninterruptibly(remainingMs)) {
+ LOGGER.warn("Timed out waiting for client channel to close during
shutdown: {}", ch);
+ return;
Review Comment:
`closeAcceptedChannels()` returns immediately on the first channel that
doesn't close within the remaining deadline. That stops attempting to close any
other tracked channels and also skips the final "{} client channels" warning,
which can leave additional client connections open and undermine the goal of
draining channels before event loop shutdown. Consider initiating `close()` on
all current channels first, then awaiting their `closeFuture()` up to the
shared deadline, and avoid the early `return` (e.g., log per-channel timeouts
but continue and emit the summary warning at the end).
```suggestion
// Initiate close on all channels first so they can progress in
parallel.
for (SocketChannel ch : channels) {
ch.close();
}
// Now wait for each channel to close, bounded by the shared deadline.
for (SocketChannel ch : channels) {
long remainingMs = Math.max(1L, deadlineMs -
System.currentTimeMillis());
if (!ch.closeFuture().awaitUninterruptibly(remainingMs)) {
LOGGER.warn("Timed out waiting for client channel to close during
shutdown: {}", ch);
```
--
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]
