Add request timeout to java client

[lakshmanan-v]

This PR adds couple of small improvements to pinot java client:

1. Provide the ability to set an optional request timeout along with read, connect and handshake timeouts.
2. Specify custom name for thread pool used by async http client.
3. Cancel the the timed out requests.

[navina]

why not int here?

[lakshmanan-v]

Would like to keep this optional, not change the default behavior of the existing clients which are using asyncHttpClient's default request timeout (which is 60 seconds i believe) when they upgrade to new version.

[navina]

I see. In that case, you can just drop in the default request timeout in the constructor, instead of using null.

  # Replace
        return create(readTimeoutMs, connectTimeoutMs, handshakeTimeoutMs, null);
 # with 
        return create(readTimeoutMs, connectTimeoutMs, handshakeTimeoutMs, 60 * 1000);

This will keep it backwards compatible and flexible. I would like to avoid unnecessary boxing/unboxing due to Integer object type. Feels unnecessary.

[lakshmanan-v]

The intent was to not to set any default value here unless the user has spcified it and continue using current behavior (AsyncHttpClient's default timeout). Was deciding between Integer object with null vs Optional, instead of small int with 0/-1 to indicate the user not setting the request timeout property. Didn't see Optional being used in the code, so used null checks for both threadpool name and request timeout.

I think we can set 60 secs default, which may be okay. Was just playing it safe. If you guys are fine with it, i can set 60s default (which is what we use for read time out).

[navina]

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.

[lakshmanan-v]

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.

[navina]

Label: pinot-client