Jackie-Jiang commented on a change in pull request #7784:
URL: https://github.com/apache/pinot/pull/7784#discussion_r752601442
##########
File path:
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/base/AbstractBaseResultSet.java
##########
@@ -229,7 +229,7 @@ public int getFetchSize()
@Override
public void setFetchSize(int rows)
throws SQLException {
- throw new SQLFeatureNotSupportedException();
+
Review comment:
Why do we need to remove the exception?
##########
File path:
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/base/AbstractBaseStatement.java
##########
@@ -110,7 +110,7 @@ public int getFetchSize()
@Override
public void setFetchSize(int rows)
throws SQLException {
- throw new SQLFeatureNotSupportedException();
+
Review comment:
Why do we need to remove the exception?
##########
File path:
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/JsonAsyncHttpPinotClientTransport.java
##########
@@ -47,6 +47,9 @@
private final Map<String, String> _headers;
private final String _scheme;
+ private static final long BROKER_READ_TIMEOUT = 1200000L;
Review comment:
20 minutes timeout seems too long. I feel 1 minute might be enough?
##########
File path:
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/JsonAsyncHttpPinotClientTransport.java
##########
@@ -47,6 +47,9 @@
private final Map<String, String> _headers;
private final String _scheme;
+ private static final long BROKER_READ_TIMEOUT = 1200000L;
Review comment:
Also suggest appending `_MS` to be more clear (`BROKER_READ_TIMEOUT_MS`
and `BROKER_CONNECT_TIMEOUT_MS`)
##########
File path:
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/Connection.java
##########
@@ -209,7 +222,7 @@ public boolean isDone() {
public ResultSetGroup get()
throws InterruptedException, ExecutionException {
try {
- return get(1000L, TimeUnit.DAYS);
+ return get(200000L, TimeUnit.NANOSECONDS);
Review comment:
This is still not fixed
##########
File path:
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/JsonAsyncHttpPinotClientTransport.java
##########
@@ -117,7 +123,7 @@ public BrokerResponse executeQuery(String brokerAddress,
String query)
public BrokerResponse executeQuery(String brokerAddress, Request request)
throws PinotClientException {
try {
- return executeQueryAsync(brokerAddress, request).get();
+ return executeQueryAsync(brokerAddress, request).get(600000L,
TimeUnit.MILLISECONDS);
Review comment:
```suggestion
return executeQueryAsync(brokerAddress,
request).get(BROKER_READ_TIMEOUT, TimeUnit.MILLISECONDS);
```
##########
File path:
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotConnection.java
##########
@@ -44,7 +44,7 @@
}
PinotConnection(String controllerURL, PinotClientTransport transport, String
tenant,
- PinotControllerTransport controllerTransport) {
+ PinotControllerTransport controllerTransport) {
Review comment:
(minor) this does not align with the `Pinot Style` format
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
