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: [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]
