Copilot commented on code in PR #16450:
URL: https://github.com/apache/pinot/pull/16450#discussion_r2254995849
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartZookeeperCommand.java:
##########
@@ -41,9 +41,20 @@ public class StartZookeeperCommand extends
AbstractBaseAdminCommand implements C
@CommandLine.Option(names = {"-zkPort"}, required = false, description =
"Port to start zookeeper server on.")
private int _zkPort = 2181;
- @CommandLine.Option(names = {"-dataDir"}, required = false, description =
"Directory for zookeper data.")
+ @CommandLine.Option(names = {"-dataDir"}, required = false, description =
"Directory for zookeeper data.")
Review Comment:
Fixed typo from 'zookeper' to 'zookeeper' in the description.
##########
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 method call changed from `getTimeSeriesQueryApiUrl(baseUrl)` to
`getTimeSeriesQueryApiUrl(getBrokerBaseApiUrl())` which ignores the `baseUrl`
parameter that was passed to the method. This could cause incorrect URL
construction when a specific base URL is intended.
```suggestion
String url = buildQueryUrl(getTimeSeriesQueryApiUrl(baseUrl),
queryParams);
```
##########
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();
Review Comment:
The return statement was changed from `return 0;` to `return
maxClientCnxns;` but the method signature suggests it should return an int.
This change might affect the behavior of the getMaxClientCnxns() method.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ZkStarter.java:
##########
@@ -154,15 +269,88 @@ public static String getDefaultZkStr() {
*/
public static ZookeeperInstance startLocalZkServer(final int port) {
return startLocalZkServer(port,
- org.apache.commons.io.FileUtils.getTempDirectoryPath() +
File.separator + "test-" + System.currentTimeMillis());
+ org.apache.commons.io.FileUtils.getTempDirectoryPath() +
File.separator + "test-" + System.currentTimeMillis(),
+ null);
+ }
+
+ /**
+ * Starts a local ZooKeeper instance with SSL enabled
+ * @return A ZookeeperInstance with SSL configuration
+ */
+ public static ZookeeperInstance startLocalZkServerWithSSL() {
+ // Create temporary directories for SSL certificates
+ File tempDir = new File(FileUtils.getTempDirectory(), "zkData" +
System.currentTimeMillis());
+ tempDir.mkdirs();
+ String keystorePath = new File(tempDir, "keystore.jks").getAbsolutePath();
+ String truststorePath = new File(tempDir,
"truststore.jks").getAbsolutePath();
+ String password = "testpassword";
+ return
startLocalZkServerWithSSL(NetUtils.findOpenPort(DEFAULT_ZK_TEST_PORT),
+ createTestSSLConfig(keystorePath, truststorePath, password));
+ }
+
+ /**
+ * Starts a local ZooKeeper instance with SSL enabled
+ * @param sslConfig SSL configuration containing keystore and truststore
paths
+ * @return A ZookeeperInstance with SSL configuration
+ */
+ public static ZookeeperInstance startLocalZkServerWithSSL(SSLConfig
sslConfig) {
+ return
startLocalZkServerWithSSL(NetUtils.findOpenPort(DEFAULT_ZK_TEST_PORT),
sslConfig);
+ }
+
+ /**
+ * Starts a local ZooKeeper instance with SSL enabled on a specific port
+ * @param port The port to listen on
+ * @param sslConfig SSL configuration containing keystore and truststore
paths
+ * @return A ZookeeperInstance with SSL configuration
+ */
+ public static ZookeeperInstance startLocalZkServerWithSSL(final int port,
SSLConfig sslConfig) {
+ return startLocalZkServerWithSSL(port,
+ org.apache.commons.io.FileUtils.getTempDirectoryPath() +
File.separator + "test-" + System.currentTimeMillis(),
+ sslConfig);
+ }
+
+ /**
+ * Starts a local ZooKeeper instance with SSL enabled
+ * @param port The port to listen on
+ * @param dataDirPath The path for the Zk data directory
+ * @param sslConfig SSL configuration containing keystore and truststore
paths
+ * @return A ZookeeperInstance with SSL configuration
+ */
+ public synchronized static ZookeeperInstance startLocalZkServerWithSSL(final
int port, final String dataDirPath,
+ SSLConfig sslConfig) {
+ // Configure ZooKeeper SSL system properties for server
+ System.setProperty("zookeeper.serverCnxnFactory",
"org.apache.zookeeper.server.NettyServerCnxnFactory");
+ System.setProperty("zookeeper.ssl.keyStore.location",
sslConfig.getKeyStorePath());
+ System.setProperty("zookeeper.ssl.keyStore.password",
sslConfig.getKeyStorePassword());
+ System.setProperty("zookeeper.ssl.keyStore.type", "JKS");
+ System.setProperty("zookeeper.ssl.trustStore.location",
sslConfig.getTrustStorePath());
+ System.setProperty("zookeeper.ssl.trustStore.password",
sslConfig.getTrustStorePassword());
+ System.setProperty("zookeeper.ssl.trustStore.type", "JKS");
+
+ // Configure secure port for SSL ZooKeeper
+ System.setProperty("zookeeper.secureClientPort", String.valueOf(port));
+ System.setProperty("zookeeper.secureClientPortAddress", "0.0.0.0");
+
+ // Enable SSL for server
+ System.setProperty("zookeeper.sslQuorum", "true");
+ System.setProperty("zookeeper.ssl.quorum.keyStore.location",
sslConfig.getKeyStorePath());
+ System.setProperty("zookeeper.ssl.quorum.keyStore.password",
sslConfig.getKeyStorePassword());
+ System.setProperty("zookeeper.ssl.quorum.trustStore.location",
sslConfig.getTrustStorePath());
+ System.setProperty("zookeeper.ssl.quorum.trustStore.password",
sslConfig.getTrustStorePassword());
+
+ // Disable regular client port for SSL-only mode
Review Comment:
Setting clientPort to -1 to disable regular client port for SSL-only mode.
Consider adding a comment explaining this is intentional for SSL-only mode, as
-1 might appear as an error value.
```suggestion
// Intentionally set clientPort to -1 to disable the regular (non-SSL)
client port for SSL-only mode.
// This is not an error value; it is required to prevent ZooKeeper from
listening on the non-SSL port.
```
##########
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:
Added 'L' suffix to prevent potential integer overflow when multiplying by
1000, which is a good practice for time calculations.
--
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]
