Copilot commented on code in PR #17989:
URL: https://github.com/apache/pinot/pull/17989#discussion_r2998784613
##########
pinot-core/src/test/java/org/apache/pinot/core/transport/QueryServerTest.java:
##########
@@ -92,6 +98,54 @@ public void testAllChannelsCleanupOnClose()
}
}
+ @Test
+ public void testServerCanRestartOnSamePortImmediately() {
+ PinotMetricUtils.init(new PinotConfiguration());
+ PinotMetricsRegistry registry = PinotMetricUtils.getPinotMetricsRegistry();
+ ServerMetrics.register(new ServerMetrics(registry));
+
+ QueryServer firstServer = new QueryServer(0, new NettyConfig(), null,
mock(ChannelHandler.class));
+ firstServer.start();
+ int port = firstServer.getChannel().localAddress().getPort();
+ firstServer.shutDown();
+
+ QueryServer secondServer = new QueryServer(port, new NettyConfig(), null,
mock(ChannelHandler.class));
+ secondServer.start();
+ try {
+ assertTrue(connectionOk(secondServer.getChannel().localAddress()));
+ } finally {
+ secondServer.shutDown();
+ }
+ }
+
+ @Test
+ public void
testCloseAcceptedChannelSnapshotClosesRemainingChannelsAfterTimeout() {
+ QueryServer server = new QueryServer(0, new NettyConfig(), null,
mock(ChannelHandler.class));
+ SocketChannel slowChannel = mock(SocketChannel.class);
+ SocketChannel fastChannel = mock(SocketChannel.class);
+ ChannelFuture slowCloseFuture = mock(ChannelFuture.class);
+ ChannelFuture fastCloseFuture = mock(ChannelFuture.class);
+
+ org.mockito.InOrder inOrder = inOrder(slowChannel, fastChannel,
slowCloseFuture, fastCloseFuture);
+ when(slowChannel.close()).thenReturn(slowCloseFuture);
+ when(fastChannel.close()).thenReturn(fastCloseFuture);
+ when(slowChannel.closeFuture()).thenReturn(slowCloseFuture);
+ when(fastChannel.closeFuture()).thenReturn(fastCloseFuture);
+ when(slowCloseFuture.awaitUninterruptibly(anyLong())).thenReturn(false);
+ when(fastCloseFuture.awaitUninterruptibly(anyLong())).thenReturn(true);
+
+ server.closeAcceptedChannelSnapshot(new SocketChannel[]{slowChannel,
fastChannel},
+ System.currentTimeMillis() + 1_000L);
+
+ inOrder.verify(slowChannel).close();
+ inOrder.verify(fastChannel).close();
+ inOrder.verify(slowChannel).closeFuture();
+ inOrder.verify(slowCloseFuture).awaitUninterruptibly(anyLong());
+ inOrder.verify(fastChannel).closeFuture();
+ inOrder.verify(fastCloseFuture).awaitUninterruptibly(anyLong());
+ verify(fastChannel).close();
Review Comment:
`testCloseAcceptedChannelSnapshotClosesRemainingChannelsAfterTimeout()`
creates a `QueryServer` (which allocates Netty event loop threads in the
constructor) but never calls `server.shutDown()`. This can leak threads and
make the test suite hang/flaky; add a `try/finally` (or similar) to ensure
shutdown even though the server is never started.
```suggestion
try {
SocketChannel slowChannel = mock(SocketChannel.class);
SocketChannel fastChannel = mock(SocketChannel.class);
ChannelFuture slowCloseFuture = mock(ChannelFuture.class);
ChannelFuture fastCloseFuture = mock(ChannelFuture.class);
org.mockito.InOrder inOrder = inOrder(slowChannel, fastChannel,
slowCloseFuture, fastCloseFuture);
when(slowChannel.close()).thenReturn(slowCloseFuture);
when(fastChannel.close()).thenReturn(fastCloseFuture);
when(slowChannel.closeFuture()).thenReturn(slowCloseFuture);
when(fastChannel.closeFuture()).thenReturn(fastCloseFuture);
when(slowCloseFuture.awaitUninterruptibly(anyLong())).thenReturn(false);
when(fastCloseFuture.awaitUninterruptibly(anyLong())).thenReturn(true);
server.closeAcceptedChannelSnapshot(new SocketChannel[]{slowChannel,
fastChannel},
System.currentTimeMillis() + 1_000L);
inOrder.verify(slowChannel).close();
inOrder.verify(fastChannel).close();
inOrder.verify(slowChannel).closeFuture();
inOrder.verify(slowCloseFuture).awaitUninterruptibly(anyLong());
inOrder.verify(fastChannel).closeFuture();
inOrder.verify(fastCloseFuture).awaitUninterruptibly(anyLong());
verify(fastChannel).close();
} finally {
server.shutDown();
}
```
##########
pinot-clients/pinot-jdbc-client/src/test/java/org/apache/pinot/client/controller/PinotControllerTransportTest.java:
##########
@@ -0,0 +1,52 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.client.controller;
+
+import java.lang.reflect.Field;
+import javax.net.ssl.SSLContext;
+import org.apache.pinot.client.ConnectionTimeouts;
+import org.apache.pinot.client.TlsProtocols;
+import org.asynchttpclient.AsyncHttpClient;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertNull;
+
+
+public class PinotControllerTransportTest {
+
+ @Test
+ public void testLegacyConstructorLeavesEndpointIdentificationAlgorithmUnset()
+ throws Exception {
+ SSLContext sslContext = SSLContext.getInstance("TLS");
+ sslContext.init(null, null, null);
+
+ PinotControllerTransport transport =
+ new PinotControllerTransport(null, "https", sslContext,
ConnectionTimeouts.create(1000, 1000, 1000),
+ TlsProtocols.defaultProtocols(false), null);
+
+ Field httpClientField =
PinotControllerTransport.class.getDeclaredField("_httpClient");
+ httpClientField.setAccessible(true);
+ AsyncHttpClient httpClient = (AsyncHttpClient)
httpClientField.get(transport);
+
+
assertNull(httpClient.getConfig().getSslEngineFactory().newSslEngine(httpClient.getConfig(),
"localhost", 443)
+ .getSSLParameters().getEndpointIdentificationAlgorithm());
+
+ transport.close();
Review Comment:
`transport.close()` is called only at the end of the test; if an assertion
fails earlier, the AsyncHttpClient may not be closed and can leak
threads/resources. Wrap the assertion section in a `try/finally` to guarantee
`transport.close()` executes.
```suggestion
PinotControllerTransport transport = null;
try {
transport =
new PinotControllerTransport(null, "https", sslContext,
ConnectionTimeouts.create(1000, 1000, 1000),
TlsProtocols.defaultProtocols(false), null);
Field httpClientField =
PinotControllerTransport.class.getDeclaredField("_httpClient");
httpClientField.setAccessible(true);
AsyncHttpClient httpClient = (AsyncHttpClient)
httpClientField.get(transport);
assertNull(httpClient.getConfig().getSslEngineFactory().newSslEngine(httpClient.getConfig(),
"localhost", 443)
.getSSLParameters().getEndpointIdentificationAlgorithm());
} finally {
if (transport != null) {
transport.close();
}
}
```
##########
pinot-common/src/test/java/org/apache/pinot/common/config/TlsConfigTest.java:
##########
@@ -39,19 +41,21 @@ public void
copyConstructorShouldCopyInsecureAndAllowedProtocols() {
}
@Test
- public void equalsAndHashCodeShouldIncludeAllowedProtocols() {
+ public void
equalsAndHashCodeShouldIncludeAllowedProtocolsAndEndpointIdentificationAlgorithm()
{
TlsConfig a = new TlsConfig();
a.setInsecure(true);
a.setAllowedProtocols(new String[]{"TLSv1.2"});
+ a.setEndpointIdentificationAlgorithm("HTTPS");
TlsConfig b = new TlsConfig();
b.setInsecure(true);
b.setAllowedProtocols(new String[]{"TLSv1.2"});
+ b.setEndpointIdentificationAlgorithm("HTTPS");
Assert.assertEquals(a, b);
Assert.assertEquals(a.hashCode(), b.hashCode());
Review Comment:
`equalsAndHashCodeShouldIncludeAllowedProtocolsAndEndpointIdentificationAlgorithm()`
no longer verifies that changes to `allowedProtocols` affect
`equals()`/`hashCode()`. The test name claims both fields are covered, but the
assertions only exercise `endpointIdentificationAlgorithm`; add an assertion
that mutating `allowedProtocols` makes the objects unequal (in addition to the
endpoint algorithm check).
```suggestion
// Changing allowedProtocols should affect equals/hashCode
b.setAllowedProtocols(new String[]{"TLSv1.3"});
Assert.assertNotEquals(a, b);
// Changing endpointIdentificationAlgorithm should also affect
equals/hashCode
```
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/admin/PinotAdminTransport.java:
##########
@@ -86,9 +90,9 @@ public PinotAdminTransport(Properties properties, Map<String,
String> authHeader
// Configure SSL if needed
if (CommonConstants.HTTPS_PROTOCOL.equalsIgnoreCase(scheme)) {
try {
- SSLContext sslContext = SSLContext.getDefault();
- builder.setSslContext(new
io.netty.handler.ssl.JdkSslContext(sslContext, true,
- io.netty.handler.ssl.ClientAuth.OPTIONAL));
+ TlsConfig tlsConfig =
ConnectionUtils.getTlsConfigFromProperties(properties);
Review Comment:
`ConnectionUtils.getSSLContextFromProperties()` calls
`TlsUtils.installDefaultSSLSocketFactory(...)`, which mutates JVM-global SSL
state (default `HttpsURLConnection` socket factory + static `TlsUtils`
SSLContext). Introducing that side effect into `PinotAdminTransport`
construction can interfere with other HTTPS clients in the same process;
consider creating a dedicated `SSLContext` for AsyncHttpClient without
installing it as the JVM default (or clearly documenting this global side
effect).
```suggestion
TlsConfig tlsConfig =
ConnectionUtils.getTlsConfigFromProperties(properties);
// NOTE: ConnectionUtils.getSSLContextFromProperties(...) installs a
JVM-global default SSLSocketFactory
// and updates the static TlsUtils SSLContext. Constructing
PinotAdminTransport with HTTPS enabled
// therefore mutates process-wide SSL/TLS state and may affect other
HTTPS clients in the same JVM.
// This behavior is currently relied upon by existing Pinot
components; callers should be aware that
// using HTTPS here has global side effects on SSL configuration.
```
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/SslContextProvider.java:
##########
@@ -42,4 +42,18 @@ public interface SslContextProvider {
*/
DefaultAsyncHttpClientConfig.Builder
configure(DefaultAsyncHttpClientConfig.Builder builder,
@Nullable SSLContext sslContext, TlsProtocols tlsProtocols);
+
+ /**
+ * Configure the AsyncHttpClient builder with SSL/TLS settings and an
explicit endpoint identification algorithm.
+ *
+ * @param builder the client config builder to update
+ * @param sslContext optional SSL context to use
+ * @param tlsProtocols configured TLS protocol list
+ * @param endpointIdentificationAlgorithm endpoint identification algorithm
for hostname verification
+ * @return the same builder for chaining
+ */
+ default DefaultAsyncHttpClientConfig.Builder
configure(DefaultAsyncHttpClientConfig.Builder builder,
+ @Nullable SSLContext sslContext, TlsProtocols tlsProtocols, @Nullable
String endpointIdentificationAlgorithm) {
Review Comment:
The new `configure(..., TlsProtocols tlsProtocols, @Nullable String
endpointIdentificationAlgorithm)` overload is being called with `tlsProtocols =
null` (e.g., admin transport). To avoid an unclear contract and potential NPEs
in other `SslContextProvider` implementations, either annotate `tlsProtocols`
as `@Nullable` (and document that null means “don’t configure protocols”), or
require a non-null value at call sites.
```suggestion
* @param tlsProtocols configured TLS protocol list; {@code null} means
protocols should not be configured
* @param endpointIdentificationAlgorithm endpoint identification
algorithm for hostname verification
* @return the same builder for chaining
*/
default DefaultAsyncHttpClientConfig.Builder
configure(DefaultAsyncHttpClientConfig.Builder builder,
@Nullable SSLContext sslContext, @Nullable TlsProtocols tlsProtocols,
@Nullable String endpointIdentificationAlgorithm) {
if (tlsProtocols == null) {
// Do not configure protocols when none are provided.
return builder;
}
```
--
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]
