Copilot commented on code in PR #17989:
URL: https://github.com/apache/pinot/pull/17989#discussion_r2998576842
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/ServerChannels.java:
##########
@@ -205,8 +213,20 @@ protected void initChannel(SocketChannel ch) {
}
void closeChannel() {
- if (_channel != null) {
- _channel.close();
+ closeChannel(false);
+ }
+
+ void closeChannel(boolean silentShutdown) {
+ Channel channel = _channel;
+ if (channel == null) {
+ return;
+ }
+ if (silentShutdown) {
+ setSilentShutdown();
+ }
+ io.netty.channel.ChannelFuture closeFuture = channel.close();
+ if (closeFuture != null) {
+ closeFuture.syncUninterruptibly();
Review Comment:
`closeChannel(...)` now blocks with `syncUninterruptibly()` on the channel
close future. This method is invoked from
`DirectOOMHandler.exceptionCaught(...)` (runs on the Netty event-loop thread),
so blocking here can stall the event loop or deadlock shutdown. Prefer
non-blocking close (listener) and only await close completion from
non-event-loop threads (e.g., in `ServerChannels.shutDown()` with an
`inEventLoop()` guard).
```suggestion
closeFuture.addListener(future -> {
if (!future.isSuccess()) {
LOGGER.warn("Failed to close channel for server routing
instance: {}", _serverRoutingInstance,
future.cause());
}
});
```
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/DefaultSslContextProvider.java:
##########
@@ -48,6 +55,9 @@ public DefaultAsyncHttpClientConfig.Builder
configure(DefaultAsyncHttpClientConf
builder.setSslEngineFactory((config, peerHost, peerPort) -> {
SSLEngine engine = sslContext.createSSLEngine(peerHost, peerPort);
engine.setUseClientMode(true);
+ SSLParameters sslParameters = engine.getSSLParameters();
+
sslParameters.setEndpointIdentificationAlgorithm(endpointIdentificationAlgorithm);
+ engine.setSSLParameters(sslParameters);
Review Comment:
`endpointIdentificationAlgorithm` is set on `SSLParameters` unconditionally.
Several call sites pass an empty string to mean "disabled"; in JSSE an
empty-but-non-null algorithm can be treated as an unknown endpoint ID algorithm
and fail the TLS handshake. Treat null/blank as "not set" (skip calling
`setEndpointIdentificationAlgorithm`) and consider having the 3-arg
`configure(...)` delegate pass null rather than "" to preserve prior behavior.
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/QueryServer.java:
##########
@@ -167,13 +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 {
- _workerGroup.shutdownGracefully(0, 0, TimeUnit.SECONDS);
- _bossGroup.shutdownGracefully(0, 0, TimeUnit.SECONDS);
+ 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:
On the first channel that times out, the method logs and `return`s, which
stops attempting to close any remaining accepted channels. For deterministic
shutdown it’s better to continue closing the rest and then emit a final warning
if any channels are still open (e.g., track a timeout flag rather than
early-returning).
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/controller/PinotControllerTransportFactory.java:
##########
@@ -86,6 +92,13 @@ public PinotControllerTransportFactory
withConnectionProperties(Properties prope
Boolean.parseBoolean(properties.getProperty("controllerTlsV10Enabled",
DEFAULT_CONTROLLER_TLS_V10_ENABLED))
|| Boolean.parseBoolean(
System.getProperties().getProperty("controller.tlsV10Enabled",
DEFAULT_CONTROLLER_TLS_V10_ENABLED));
+ if (_scheme.contentEquals(CommonConstants.HTTPS_PROTOCOL)) {
+ _endpointIdentificationAlgorithm =
+
org.apache.pinot.client.utils.DriverUtils.getTlsConfigFromJDBCProps(properties)
+ .getEndpointIdentificationAlgorithm();
Review Comment:
This file uses the fully-qualified
`org.apache.pinot.client.utils.DriverUtils` inline, while other JDBC classes
import it directly (e.g.,
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotDriver.java:41).
Consider adding an import and using `DriverUtils` to keep usage consistent and
improve readability.
--
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]
