Re: [PR] feat: Add comprehensive ZooKeeper SSL support across Pinot cluster [pinot]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #16450:
URL: https://github.com/apache/pinot/pull/16450#discussion_r2252960238


##########
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 passed to `getTimeSeriesQueryApiUrl()` has been changed from `baseUrl` to `getBrokerBaseApiUrl()` which may not be the intended behavior. 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 change from `* 1000` to `* 1000L` is unnecessary since the result will be promoted to long anyway when passed to `Timeout.of()`. This change doesn't add value and could be considered noise.
   ```suggestion
               .setSoTimeout(Timeout.of(_config.getServerAdminRequestTimeoutSeconds() * 1000, TimeUnit.MILLISECONDS))
   ```



##########
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:
   This return statement conflicts with the original behavior that returned 0. The change from `return 0` to `return maxClientCnxns` may break existing functionality that expects a return value of 0.



##########
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 should return `maxClientCnxns` after setting it, but line 232 still returns `maxClientCnxns` instead of the expected return value. The logic appears inconsistent with the original code that returned 0.
   ```suggestion
             return 0;
   ```



##########
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 contains a typo: `ZOOKEEPR_SERVER` should be `ZOOKEEPER_SERVER`. While this is marked as deprecated, the typo should be noted for clarity.



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org