> On Aug 21, 2019, at 8:49 AM, Alberto Gomez <alberto.go...@est.tech> wrote:
> 2. Timeout in ResultCollector::getResult() and Execution::execute() blocking
>
> Regarding the timeout in the ResultCollector::getResult() method problem and
> the blocking/non-blocking confusion for Execution::execute() two alternatives
> are considered:
>
> a) Remove the possibility of setting a timeout on the
> ResultCollector::getResult() method on the client side as with the current
> client implementation it is useless. This could be done by removing the
> method with the timeout parameter from the public API.
>
> It would be advisable to make explicit in the documentation that the
> getResult() method does not wait for results to arrive as that should have
> already been done in the Execution::execute() invocation.
>
> This alternative is very simple and would keep things pretty much as they are
> today.
To be honest I think approach would go against what a user “thinks” is going
on. Given that there hasn’t been a timeout on execute I assumed it was
asynchronous and that the getResult blocked until timeout or results arrived.
Typically these two calls were done back to back.
> b) Transform the Execution::execute() method on the client side into a
> non-blocking method.
>
> This alternative is more complex and requires changes in all the clients.
> Apart from that it has implications on the public client API it requires
> moving the exceptions offered currently by the Execution::execute() method to
> the ResultCollector::getResult() and new threads will have to be managed.
I think this is more in line with what users expect is going on based on the
current API, I know I have. If were are going to make any change I think this
is the one. I don’t think the behavior change is a problem since it's what is
expected to be happening anyway.
> An outline of a possible implementation for option b) would be:
>
> * Instead of invoking the ServerRegionProxy::executeFunction() directly as
> it is done today, create a Future that invokes this method and returns the
> resultCollector passed as parameter.
Do you really think we need to introduce Futures here into the API? I feel like
the ResultCollector acts as the Future. I don’t think any change needs to be
made to the API in this regard. The ResultCollector implementation would just
simply block as implied by the api for the timeout period. I would change the
method to have units though and deprecate the method without units.
-Jake
