Copilot commented on code in PR #16450:
URL: https://github.com/apache/pinot/pull/16450#discussion_r2255010845
##########
pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/ClusterTest.java:
##########
@@ -571,8 +592,8 @@ public JsonNode getTimeseriesQuery(String baseUrl, String
query, long startTime,
Map<String, String> headers) {
try {
Map<String, String> queryParams = Map.of("language", "m3ql", "query",
query, "start",
- String.valueOf(startTime), "end", String.valueOf(endTime));
- String url = buildQueryUrl(getTimeSeriesQueryApiUrl(baseUrl),
queryParams);
+ String.valueOf(startTime), "end", String.valueOf(endTime));
+ String url =
buildQueryUrl(getTimeSeriesQueryApiUrl(getBrokerBaseApiUrl()), queryParams);
Review Comment:
The parameter to getTimeSeriesQueryApiUrl() was changed from 'baseUrl' to
'getBrokerBaseApiUrl()' without considering that the original 'baseUrl'
parameter might be needed for the function to work correctly. This could break
timeseries queries if baseUrl was intentionally different from the broker base
API URL.
```suggestion
String url = buildQueryUrl(getTimeSeriesQueryApiUrl(baseUrl),
queryParams);
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java:
##########
@@ -551,7 +555,7 @@ private void setUpPinotController() {
_connectionManager =
PoolingHttpClientConnectionManagerHelper.createWithSocketFactory();
_connectionManager.setDefaultSocketConfig(
SocketConfig.custom()
-
.setSoTimeout(Timeout.of(_config.getServerAdminRequestTimeoutSeconds() * 1000,
TimeUnit.MILLISECONDS))
+
.setSoTimeout(Timeout.of(_config.getServerAdminRequestTimeoutSeconds() * 1000L,
TimeUnit.MILLISECONDS))
Review Comment:
[nitpick] The multiplication by 1000L to convert seconds to milliseconds is
now explicit with the 'L' suffix, but this change appears unrelated to SSL
functionality and should be in a separate commit or explained in the PR
description.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ZkStarter.java:
##########
@@ -113,8 +228,8 @@ public int getMaxClientCnxns() {
tickTime = getTickTime();
minSessionTimeout = getMinSessionTimeout();
maxSessionTimeout = getMaxSessionTimeout();
- maxClientCnxns = 0;
- return 0;
+ maxClientCnxns = config.getMaxClientCnxns();
+ return maxClientCnxns;
Review Comment:
The logic was changed to return maxClientCnxns instead of hardcoded 0, but
the previous line still sets maxClientCnxns = 0. This creates inconsistent
behavior where maxClientCnxns is set to 0 but then overwritten with
config.getMaxClientCnxns().
```suggestion
return config.getMaxClientCnxns();
```
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ZkStarter.java:
##########
@@ -113,8 +228,8 @@ public int getMaxClientCnxns() {
tickTime = getTickTime();
minSessionTimeout = getMinSessionTimeout();
maxSessionTimeout = getMaxSessionTimeout();
- maxClientCnxns = 0;
- return 0;
+ maxClientCnxns = config.getMaxClientCnxns();
+ return maxClientCnxns;
Review Comment:
The method now returns maxClientCnxns instead of 0, but this appears to be
in the wrong context. The method getMaxClientCnxns() should return the maximum
client connections, but the logic seems to be setting and returning the value
inconsistently.
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java:
##########
@@ -230,20 +233,89 @@ protected Map<String, String>
getControllerRequestClientHeaders() {
public ControllerRequestClient getControllerRequestClient() {
if (_controllerRequestClient == null) {
_controllerRequestClient = new
ControllerRequestClient(_controllerRequestURLBuilder, getHttpClient(),
- getControllerRequestClientHeaders());
+ getControllerRequestClientHeaders());
}
return _controllerRequestClient;
}
public void startZk() {
if (_zookeeperInstance == null) {
- runWithHelixMock(() -> _zookeeperInstance =
ZkStarter.startLocalZkServer());
+ runWithHelixMock(() -> {
+ // Randomly choose between SSL and non-SSL (50% chance each)
+ boolean useSSL = RANDOM.nextBoolean();
+ if (useSSL) {
+ _zookeeperInstance = startSSLZooKeeper();
+ } else {
+ _zookeeperInstance = ZkStarter.startLocalZkServer();
+ }
+ });
}
}
public void startZk(int port) {
if (_zookeeperInstance == null) {
- runWithHelixMock(() -> _zookeeperInstance =
ZkStarter.startLocalZkServer(port));
+ runWithHelixMock(() -> {
+ // Randomly choose between SSL and non-SSL (50% chance each)
+ boolean useSSL = RANDOM.nextBoolean();
+ if (useSSL) {
+ _zookeeperInstance = startSSLZooKeeper(port);
+ } else {
+ _zookeeperInstance = ZkStarter.startLocalZkServer(port);
+ }
+ });
+ }
+ }
+
+ private ZkStarter.ZookeeperInstance startSSLZooKeeper() {
+ try {
+ // Create temporary SSL certificates for testing
+ File tempDir = new File(FileUtils.getTempDirectory(),
"controller-test-ssl-" + System.currentTimeMillis());
+ tempDir.mkdirs();
+
+ String keystorePath = new File(tempDir,
"test-keystore.jks").getAbsolutePath();
+ String truststorePath = new File(tempDir,
"test-truststore.jks").getAbsolutePath();
+ String password = "test-ssl-" + System.currentTimeMillis();
+
+ ZkStarter.SSLConfig sslConfig =
ZkStarter.createTestSSLConfig(keystorePath, truststorePath, password);
+
+ // Configure SSL for all clients in this test
+ ZkSSLUtils.configureSSL(sslConfig.getClientSSLProperties());
+ LOGGER.info("starting local zookeeper with SSL configs: {}",
sslConfig.getClientSSLProperties());
+
+ ZkStarter.ZookeeperInstance zkInstance =
ZkStarter.startLocalZkServerWithSSL(sslConfig);
+ LOGGER.info("Started local zookeeper {} with SSL configs: {}",
zkInstance.getZkUrl(),
+ zkInstance.getSSLConfig().getClientSSLProperties());
+
+ return zkInstance;
+ } catch (Exception e) {
+ // If SSL setup fails, fall back to non-SSL
+ System.err.println("Failed to setup SSL ZooKeeper, falling back to
non-SSL: " + e.getMessage());
+ ZkSSLUtils.configureSSL(new PinotConfiguration(Collections.emptyMap()));
Review Comment:
Missing import for Collections class. The code uses Collections.emptyMap()
but Collections is not imported in the file.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -245,6 +246,30 @@ public static class Instance {
@Deprecated(since = "1.5.0", forRemoval = true)
public static final String CONFIG_OF_ZOOKEEPR_SERVER = "pinot.zk.server";
Review Comment:
The constant name 'CONFIG_OF_ZOOKEEPR_SERVER' contains a typo. It should be
'CONFIG_OF_ZOOKEEPER_SERVER' (missing 'E' in 'ZOOKEEPER').
```suggestion
```
##########
pinot-controller/src/test/resources/log4j2.xml:
##########
@@ -26,10 +26,10 @@
</Console>
</Appenders>
<Loggers>
- <Logger name="org.apache.pinot" level="warn" additivity="false">
+ <Logger name="org.apache.pinot" level="info" additivity="false">
Review Comment:
[nitpick] The log level was changed from 'warn' to 'info' which will
increase log verbosity. This change appears unrelated to SSL functionality and
could impact test performance or output readability.
```suggestion
<Logger name="org.apache.pinot" level="warn" additivity="false">
```
--
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]
