Sorry for the slow response, I've been trying to decide what I think is the right approach here.
For (1) - conceptually, I don't have a problem with having both blocking and non blocking methods on Execution. So adding blocking versions of execute() with a timeout seems ok. But I do think if we add them we need to implement them on both the client and the server to behave the same way. That shouldn't be too hard on the server since execute(timeout) can just call getResult(timeout) internally. For (2) - Although I think the original authors of this API probably did intend for execute() to be non-blocking, the fact is that it does block on the client and most users are probably calling execute from a client. So I do agree we probably shouldn't change the behavior at this point. Perhaps we can just clearly document the current behavior of execute() as part of adding these new methods. Going forward we can add new methods to Execution that are clearly non-blocking (submit?, invoke?) and implement them consistently on *both* the client in the server, but that doesn't have to be in the scope of this proposal. -Dan On Fri, Aug 23, 2019 at 4:28 AM Alberto Gomez <alberto.go...@est.tech> wrote: > 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 >
