navina commented on code in PR #10638:
URL: https://github.com/apache/pinot/pull/10638#discussion_r1170826428
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/ConnectionTimeouts.java:
##########
@@ -25,18 +25,26 @@ public class ConnectionTimeouts {
private final int _readTimeoutMs;
private final int _connectTimeoutMs;
private final int _handshakeTimeoutMs;
+ private final Integer _requestTimeoutMs;
Review Comment:
why not `int` here?
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/JsonAsyncHttpPinotClientTransport.java:
##########
@@ -114,9 +131,14 @@ public JsonAsyncHttpPinotClientTransport(Map<String,
String> headers, String sch
@Override
public BrokerResponse executeQuery(String brokerAddress, String query)
throws PinotClientException {
+ Future<BrokerResponse> future = null;
try {
- return executeQueryAsync(brokerAddress, query).get(_brokerReadTimeout,
TimeUnit.MILLISECONDS);
+ future = executeQueryAsync(brokerAddress, query);
+ return future.get(_brokerReadTimeout, TimeUnit.MILLISECONDS);
} catch (Exception e) {
+ if (e instanceof TimeoutException) {
Review Comment:
better to catch `TimeoutException` ?
Side note: is there any benefit of trying to cancel a future task here since
it is due to timeout? Iiuc, this would simply interrupt the thread already
running the query and may choose to ignore the interrupt.
--
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
