lakshmanan-v commented on code in PR #10638:
URL: https://github.com/apache/pinot/pull/10638#discussion_r1175842921
##########
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:
Since the exception handling logic is same for both generic exception and
timeout exception, (other than canceling the future), added casting vs two
catch blocks.
On the benefits of canceling future, its a best effort attempt to free up
resources without any side. effects. In theory, it doesn't makes sense to
continue the work handled by the underlying task and keep using the resources,
after the client decides not to wait (future timedout).
It is more effective if the underlying task hasn't started yet.
If the job is running, if we pass the mayBeInterrupting as true, the task
will receive InterruptedException. But the task may catch and ignore the
interrupted exception, like you pointed out. Only thing i was worried about is
the client not handling interrupted exception properly (not closing files/
connections etc). I checked the asyncHttpClient code, it seems to handle the
InterruptedException.
future.cancel(false) is still safest option as it helps cancel the task
before its started, but doesn't do anything if the task has completed / still
running.
--
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
