Hi Jake, Please, see my answers below.
On 22/8/19 21:16, Jacob Barrett wrote: > >> 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. You are right if you look at the Java client. But if you look at the native clients, the timeout is there, both in the C++ and the C# cases which would indicate that it is a blocking call. See https://geode.apache.org/releases/latest/cppdocs/a00725.html#aa918a5e193745950e12ca4feb9c5d776 and https://geode.apache.org/releases/latest/dotnetdocs/a00882.html#ae0a814049482ca424f89c13ab1099c3d > >> 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. I did not mean to introduce Futures in the API. My idea was to use Java Futures internally so that the ResultCollector returned by the getResult() method would wrap the Java Future with the ResultCollector that would actually hold the result. An alternative would be to leave the logic of blocking to each implementation ResultCollector. In the case of the DefaultResultCollector, we could use a CountDownLatch that would be decremented when endResults() is called and that would make getResult() block by using CountDown.await(...). The advantage of using Futures internally is that the blocking logic would not have to be implemented on every ResultCollector implementation. > > -Jake > > - Alberto
