TAJO-1469 allocateQueryMaster can leak resources if it times-out (3sec, hardcoded)

[navis]

Tested with some hack to reproduce timeout.

WorkerResourceAllocationResponse response = null;
try {
  response = callFuture.get(3, queryInProgress.getQueryId().getSeq() < 3 ? TimeUnit.MICROSECONDS : TimeUnit.SECONDS);
} catch (Throwable t) {
  ....
}

exceptions for first two but successes for all following queries.

[babokim]

I think this line is not necessary if you change as the following in the immediately preceding line.

LOG.warn("Got exception while allocating QueryMaster: " + t, t);  

[navis]

This (response != null in catch block) means response is acquired just after timeout. I don't think it should be logged even after we got expected result in time.

[babokim]

I investigated the lock of CallFuture. CallFuture should be synchronized with run() and get(). Current code looks like this would be implemented but not. If the following situation is occur, some resources or tasks will be lost forever.

1. Worker: TaskRunner sends GetTask request.
2. QM: QueryMaster selects proper task and calls RpcCallback.
3. Worker: AsyncRpcClient receives the response and calls CallFuture.run(response).
   3-1. Worker: If TimeoutException occurs after 1) between 2) ~ 3), TaskRunner can't receive the response and doesn't run the allocated task, but QM doesn't know about that.

If my thought is wrong, please let me know.
If my thought is right, this patch is temporary solution and we need to create another issue for this problem.

[babokim]

I think that SimpleExchange name is not proper this class. How about "CancelableRpcCallback"?

[navis]

ok

[babokim]

If this patch is considered as a temporary solution, looks good to me.
@navis, thank you for your contribution, I left some comments.

[navis]

Yes, if resource allocation is done over network, we need more serious work to fix that. If the problem is recognized fully by this simple, in-process fix, I have a intent to fix that, too.

[jihoonson]

Thanks @navis! This is a critical problem that should be fixed.
You seem to handle a somple case in this issue. If so, would you book other remaining issues on Jira? It will be helpful to remind us.
Thanks.

[navis]

Addressed comments & added "tajo.qm.resource.allocation.timeout" to set timeout.

[navis]

@jihoonson Would you mind to review TAJO-1385 first? it seemed it would overlap in some part.

[jihoonson]

Ok. I'll review today.

[babokim]

+1
All tests passed. I'll commit soon.