Re: [DISCUSS] Improvements on client function execution API
Thanks Anil. This is how this started, trying to pop the timeout up from a system property to a parameter in the Execution methods. -Alberto G. On 16/9/19 21:37, Anilkumar Gingade wrote: > Alberto, > > Sorry for late responseCurrently geode (java client) does provide > ability to set function timeout; but its through internal system property > "gemfire.CLIENT_FUNCTION_TIMEOUT"Some of the tests using this property > are Tests extending "FunctionRetryTestBase". > > Since this is through internal system property; we need a cleaner api to > achieve the timeout behavior. > > +1 for the option a) proposed. > > -Anil > > > > > On Mon, Sep 16, 2019 at 9:03 AM Dan Smith wrote: > >> Thanks for following up on this! >> >> -Dan >> >> On Mon, Sep 16, 2019 at 3:07 AM Alberto Gomez >> wrote: >> >>> Thanks for the feedback. I also give a +1 to option a) including Dan's >>> comments. >>> >>> I'll move the RFC to the Development state and will open a ticket to >>> follow up on the implementation. >>> >>> -Alberto G. >>> >>> On 12/9/19 8:15, Jacob Barrett wrote: +1 I echo Dan’s comments as well. Thanks for tackling this. -jake > On Sep 11, 2019, at 2:36 PM, Dan Smith wrote: > > +1 - Ok, I think I've come around to option (a). We can go head and >>> add a > new execute(timeout, TimeUnit) method to the java API that is >> blocking. >>> We > can leave the existing execute() method alone, except for documenting >>> what > it is doing. > > I would like implement execute(timeout, TimeUnit) on the server side >> as > well. Since this Execution class is shared between both client and >>> server > APIs, it would be unfortunate to have a method on Execution that >> simply > doesn't work on the server side. > > -Dan > > >> On Thu, Sep 5, 2019 at 9:25 AM Alberto Gomez >> wrote: >> Hi all, >> >> First of all, thanks a lot Dan and Jacob for your feedback. >> >> As we are getting close to the deadline I am adding here some >>> conclusions >> and a refined proposal in order to get some more feedback and if >>> possible >> some voting on the two alternatives proposed (or any other in between >>> if >> you feel any of them is lacking something). >> >> I also add some draft code to try to clarify a bit the more complex >> of >>> the >> alternatives. >> >> >> Proposal summary (needs a decision on which option to implement): >> >> >> --- >> In order to make the API more coherent two alternatives are proposed: >> >> a) Remove the timeout from the ResultCollector::getResult() / >> document >> that the timeout has no effect, taking into account that >> Execution::execute() is always blocking. >> Additionally we could add the timeout parameter to the >> Execution::execute() method of the Java API in order to align it with >>> the >> native client APIs. This timeout would not be the read timeout on the >> socket but a timeout for the execution of the operation. >> >> b) Change the implementation of the Execution::execute() method >> without >> timeout to be non-blocking on both the Java and native APIs. This >>> change >> has backward compatibility implications, would probably bring some >> performance decrease and could pose some difficulties in the >>> implementation >> on the C++ side (in the handling of timed out operations that hold >> resources). >> >> >> The first option (a) is less risky and does not have impacts >> regarding >> backward compatibility and performance. >> >> The second one (b) is the preferred alternative in terms of the >>> expected >> behavior from the users of the API. This option is more complex to >> implement and as mentioned above has performance and backward >>> compatibility >> issues not easy to be solved. >> >> Following is a draft version of the implementation of b) on the Java >> client: >> >> >> https://github.com/Nordix/geode/commit/507a795e34c6083c129bda7e976b9223d1a893da >> Following is a draft version of the implementation of b) on the C++ >>> native >> client: >> >> >> https://github.com/apache/geode-native/commit/a03a56f229bb8d75ee71044cf6196df07f43150d >> Note that the above implementation of b) in the C++ client implies >> that >> the Execution object returned by the FunctionService cannot be >>> destroyed >> until the thread executing the function asynchronously has finished. >>> If the >> function times out, the Execution object must be kept until the >> thread >> finishes. >> >> >> Other considerations >> - >> >> * Currently, in the function execution Java client there is not a >> possibility to set a timeout for the
Re: [DISCUSS] Improvements on client function execution API
Alberto, Sorry for late responseCurrently geode (java client) does provide ability to set function timeout; but its through internal system property "gemfire.CLIENT_FUNCTION_TIMEOUT"Some of the tests using this property are Tests extending "FunctionRetryTestBase". Since this is through internal system property; we need a cleaner api to achieve the timeout behavior. +1 for the option a) proposed. -Anil On Mon, Sep 16, 2019 at 9:03 AM Dan Smith wrote: > Thanks for following up on this! > > -Dan > > On Mon, Sep 16, 2019 at 3:07 AM Alberto Gomez > wrote: > > > Thanks for the feedback. I also give a +1 to option a) including Dan's > > comments. > > > > I'll move the RFC to the Development state and will open a ticket to > > follow up on the implementation. > > > > -Alberto G. > > > > On 12/9/19 8:15, Jacob Barrett wrote: > > > +1 > > > > > > I echo Dan’s comments as well. > > > > > > Thanks for tackling this. > > > > > > -jake > > > > > > > > >> On Sep 11, 2019, at 2:36 PM, Dan Smith wrote: > > >> > > >> +1 - Ok, I think I've come around to option (a). We can go head and > > add a > > >> new execute(timeout, TimeUnit) method to the java API that is > blocking. > > We > > >> can leave the existing execute() method alone, except for documenting > > what > > >> it is doing. > > >> > > >> I would like implement execute(timeout, TimeUnit) on the server side > as > > >> well. Since this Execution class is shared between both client and > > server > > >> APIs, it would be unfortunate to have a method on Execution that > simply > > >> doesn't work on the server side. > > >> > > >> -Dan > > >> > > >> > > >>> On Thu, Sep 5, 2019 at 9:25 AM Alberto Gomez > > > wrote: > > >>> > > >>> Hi all, > > >>> > > >>> First of all, thanks a lot Dan and Jacob for your feedback. > > >>> > > >>> As we are getting close to the deadline I am adding here some > > conclusions > > >>> and a refined proposal in order to get some more feedback and if > > possible > > >>> some voting on the two alternatives proposed (or any other in between > > if > > >>> you feel any of them is lacking something). > > >>> > > >>> I also add some draft code to try to clarify a bit the more complex > of > > the > > >>> alternatives. > > >>> > > >>> > > >>> Proposal summary (needs a decision on which option to implement): > > >>> > > >>> > > > --- > > >>> > > >>> In order to make the API more coherent two alternatives are proposed: > > >>> > > >>> a) Remove the timeout from the ResultCollector::getResult() / > document > > >>> that the timeout has no effect, taking into account that > > >>> Execution::execute() is always blocking. > > >>> Additionally we could add the timeout parameter to the > > >>> Execution::execute() method of the Java API in order to align it with > > the > > >>> native client APIs. This timeout would not be the read timeout on the > > >>> socket but a timeout for the execution of the operation. > > >>> > > >>> b) Change the implementation of the Execution::execute() method > without > > >>> timeout to be non-blocking on both the Java and native APIs. This > > change > > >>> has backward compatibility implications, would probably bring some > > >>> performance decrease and could pose some difficulties in the > > implementation > > >>> on the C++ side (in the handling of timed out operations that hold > > >>> resources). > > >>> > > >>> > > >>> The first option (a) is less risky and does not have impacts > regarding > > >>> backward compatibility and performance. > > >>> > > >>> The second one (b) is the preferred alternative in terms of the > > expected > > >>> behavior from the users of the API. This option is more complex to > > >>> implement and as mentioned above has performance and backward > > compatibility > > >>> issues not easy to be solved. > > >>> > > >>> Following is a draft version of the implementation of b) on the Java > > >>> client: > > >>> > > >>> > > > https://github.com/Nordix/geode/commit/507a795e34c6083c129bda7e976b9223d1a893da > > >>> > > >>> Following is a draft version of the implementation of b) on the C++ > > native > > >>> client: > > >>> > > >>> > > > https://github.com/apache/geode-native/commit/a03a56f229bb8d75ee71044cf6196df07f43150d > > >>> > > >>> Note that the above implementation of b) in the C++ client implies > that > > >>> the Execution object returned by the FunctionService cannot be > > destroyed > > >>> until the thread executing the function asynchronously has finished. > > If the > > >>> function times out, the Execution object must be kept until the > thread > > >>> finishes. > > >>> > > >>> > > >>> Other considerations > > >>> - > > >>> > > >>> * Currently, in the function execution Java client there is not a > > >>> possibility to set a timeout for the execution of functions. The > > closest to > > >>> this is the read timeout that may be set globally for
Re: [DISCUSS] Improvements on client function execution API
Thanks for following up on this! -Dan On Mon, Sep 16, 2019 at 3:07 AM Alberto Gomez wrote: > Thanks for the feedback. I also give a +1 to option a) including Dan's > comments. > > I'll move the RFC to the Development state and will open a ticket to > follow up on the implementation. > > -Alberto G. > > On 12/9/19 8:15, Jacob Barrett wrote: > > +1 > > > > I echo Dan’s comments as well. > > > > Thanks for tackling this. > > > > -jake > > > > > >> On Sep 11, 2019, at 2:36 PM, Dan Smith wrote: > >> > >> +1 - Ok, I think I've come around to option (a). We can go head and > add a > >> new execute(timeout, TimeUnit) method to the java API that is blocking. > We > >> can leave the existing execute() method alone, except for documenting > what > >> it is doing. > >> > >> I would like implement execute(timeout, TimeUnit) on the server side as > >> well. Since this Execution class is shared between both client and > server > >> APIs, it would be unfortunate to have a method on Execution that simply > >> doesn't work on the server side. > >> > >> -Dan > >> > >> > >>> On Thu, Sep 5, 2019 at 9:25 AM Alberto Gomez > wrote: > >>> > >>> Hi all, > >>> > >>> First of all, thanks a lot Dan and Jacob for your feedback. > >>> > >>> As we are getting close to the deadline I am adding here some > conclusions > >>> and a refined proposal in order to get some more feedback and if > possible > >>> some voting on the two alternatives proposed (or any other in between > if > >>> you feel any of them is lacking something). > >>> > >>> I also add some draft code to try to clarify a bit the more complex of > the > >>> alternatives. > >>> > >>> > >>> Proposal summary (needs a decision on which option to implement): > >>> > >>> > --- > >>> > >>> In order to make the API more coherent two alternatives are proposed: > >>> > >>> a) Remove the timeout from the ResultCollector::getResult() / document > >>> that the timeout has no effect, taking into account that > >>> Execution::execute() is always blocking. > >>> Additionally we could add the timeout parameter to the > >>> Execution::execute() method of the Java API in order to align it with > the > >>> native client APIs. This timeout would not be the read timeout on the > >>> socket but a timeout for the execution of the operation. > >>> > >>> b) Change the implementation of the Execution::execute() method without > >>> timeout to be non-blocking on both the Java and native APIs. This > change > >>> has backward compatibility implications, would probably bring some > >>> performance decrease and could pose some difficulties in the > implementation > >>> on the C++ side (in the handling of timed out operations that hold > >>> resources). > >>> > >>> > >>> The first option (a) is less risky and does not have impacts regarding > >>> backward compatibility and performance. > >>> > >>> The second one (b) is the preferred alternative in terms of the > expected > >>> behavior from the users of the API. This option is more complex to > >>> implement and as mentioned above has performance and backward > compatibility > >>> issues not easy to be solved. > >>> > >>> Following is a draft version of the implementation of b) on the Java > >>> client: > >>> > >>> > https://github.com/Nordix/geode/commit/507a795e34c6083c129bda7e976b9223d1a893da > >>> > >>> Following is a draft version of the implementation of b) on the C++ > native > >>> client: > >>> > >>> > https://github.com/apache/geode-native/commit/a03a56f229bb8d75ee71044cf6196df07f43150d > >>> > >>> Note that the above implementation of b) in the C++ client implies that > >>> the Execution object returned by the FunctionService cannot be > destroyed > >>> until the thread executing the function asynchronously has finished. > If the > >>> function times out, the Execution object must be kept until the thread > >>> finishes. > >>> > >>> > >>> Other considerations > >>> - > >>> > >>> * Currently, in the function execution Java client there is not a > >>> possibility to set a timeout for the execution of functions. The > closest to > >>> this is the read timeout that may be set globally for function > executions > >>> but this is not really a timeout for operations. > >>> > >>> * Even if the API for function execution is the same on clients and > >>> servers, the implementation is different between them. On the clients, > the > >>> execute() methods are blocking while on the servers it is non-blocking > and > >>> the invoker of the function blocks on the getResult() method of the > >>> ResultCollector returned by the execute() method. > >>> Even if having both blocking and non-blocking implementation of > execute() > >>> in both clients and servers sounds desirable from the point of view of > >>> orthogonality, this could bring complications in terms of backward > >>> compatibility. Besides, a need for a blocking version of function
Re: [DISCUSS] Improvements on client function execution API
Thanks for the feedback. I also give a +1 to option a) including Dan's comments. I'll move the RFC to the Development state and will open a ticket to follow up on the implementation. -Alberto G. On 12/9/19 8:15, Jacob Barrett wrote: > +1 > > I echo Dan’s comments as well. > > Thanks for tackling this. > > -jake > > >> On Sep 11, 2019, at 2:36 PM, Dan Smith wrote: >> >> +1 - Ok, I think I've come around to option (a). We can go head and add a >> new execute(timeout, TimeUnit) method to the java API that is blocking. We >> can leave the existing execute() method alone, except for documenting what >> it is doing. >> >> I would like implement execute(timeout, TimeUnit) on the server side as >> well. Since this Execution class is shared between both client and server >> APIs, it would be unfortunate to have a method on Execution that simply >> doesn't work on the server side. >> >> -Dan >> >> >>> On Thu, Sep 5, 2019 at 9:25 AM Alberto Gomez wrote: >>> >>> Hi all, >>> >>> First of all, thanks a lot Dan and Jacob for your feedback. >>> >>> As we are getting close to the deadline I am adding here some conclusions >>> and a refined proposal in order to get some more feedback and if possible >>> some voting on the two alternatives proposed (or any other in between if >>> you feel any of them is lacking something). >>> >>> I also add some draft code to try to clarify a bit the more complex of the >>> alternatives. >>> >>> >>> Proposal summary (needs a decision on which option to implement): >>> >>> --- >>> >>> In order to make the API more coherent two alternatives are proposed: >>> >>> a) Remove the timeout from the ResultCollector::getResult() / document >>> that the timeout has no effect, taking into account that >>> Execution::execute() is always blocking. >>> Additionally we could add the timeout parameter to the >>> Execution::execute() method of the Java API in order to align it with the >>> native client APIs. This timeout would not be the read timeout on the >>> socket but a timeout for the execution of the operation. >>> >>> b) Change the implementation of the Execution::execute() method without >>> timeout to be non-blocking on both the Java and native APIs. This change >>> has backward compatibility implications, would probably bring some >>> performance decrease and could pose some difficulties in the implementation >>> on the C++ side (in the handling of timed out operations that hold >>> resources). >>> >>> >>> The first option (a) is less risky and does not have impacts regarding >>> backward compatibility and performance. >>> >>> The second one (b) is the preferred alternative in terms of the expected >>> behavior from the users of the API. This option is more complex to >>> implement and as mentioned above has performance and backward compatibility >>> issues not easy to be solved. >>> >>> Following is a draft version of the implementation of b) on the Java >>> client: >>> >>> https://github.com/Nordix/geode/commit/507a795e34c6083c129bda7e976b9223d1a893da >>> >>> Following is a draft version of the implementation of b) on the C++ native >>> client: >>> >>> https://github.com/apache/geode-native/commit/a03a56f229bb8d75ee71044cf6196df07f43150d >>> >>> Note that the above implementation of b) in the C++ client implies that >>> the Execution object returned by the FunctionService cannot be destroyed >>> until the thread executing the function asynchronously has finished. If the >>> function times out, the Execution object must be kept until the thread >>> finishes. >>> >>> >>> Other considerations >>> - >>> >>> * Currently, in the function execution Java client there is not a >>> possibility to set a timeout for the execution of functions. The closest to >>> this is the read timeout that may be set globally for function executions >>> but this is not really a timeout for operations. >>> >>> * Even if the API for function execution is the same on clients and >>> servers, the implementation is different between them. On the clients, the >>> execute() methods are blocking while on the servers it is non-blocking and >>> the invoker of the function blocks on the getResult() method of the >>> ResultCollector returned by the execute() method. >>> Even if having both blocking and non-blocking implementation of execute() >>> in both clients and servers sounds desirable from the point of view of >>> orthogonality, this could bring complications in terms of backward >>> compatibility. Besides, a need for a blocking version of function execution >>> has not been found. >>> >>> -Alberto G. >>> >>> On 29/8/19 16:48, Alberto Gomez wrote: >>> >>> Sorry, some corrections on my comments after revisiting the native >>> client code. >>> >>> When I said that the timeout used in the execution() method (by means of >>> a system property) was to set a read timeout on the socket, I was only >>> talking
Re: [DISCUSS] Improvements on client function execution API
+1 I echo Dan’s comments as well. Thanks for tackling this. -jake > On Sep 11, 2019, at 2:36 PM, Dan Smith wrote: > > +1 - Ok, I think I've come around to option (a). We can go head and add a > new execute(timeout, TimeUnit) method to the java API that is blocking. We > can leave the existing execute() method alone, except for documenting what > it is doing. > > I would like implement execute(timeout, TimeUnit) on the server side as > well. Since this Execution class is shared between both client and server > APIs, it would be unfortunate to have a method on Execution that simply > doesn't work on the server side. > > -Dan > > >> On Thu, Sep 5, 2019 at 9:25 AM Alberto Gomez wrote: >> >> Hi all, >> >> First of all, thanks a lot Dan and Jacob for your feedback. >> >> As we are getting close to the deadline I am adding here some conclusions >> and a refined proposal in order to get some more feedback and if possible >> some voting on the two alternatives proposed (or any other in between if >> you feel any of them is lacking something). >> >> I also add some draft code to try to clarify a bit the more complex of the >> alternatives. >> >> >> Proposal summary (needs a decision on which option to implement): >> >> --- >> >> In order to make the API more coherent two alternatives are proposed: >> >> a) Remove the timeout from the ResultCollector::getResult() / document >> that the timeout has no effect, taking into account that >> Execution::execute() is always blocking. >> Additionally we could add the timeout parameter to the >> Execution::execute() method of the Java API in order to align it with the >> native client APIs. This timeout would not be the read timeout on the >> socket but a timeout for the execution of the operation. >> >> b) Change the implementation of the Execution::execute() method without >> timeout to be non-blocking on both the Java and native APIs. This change >> has backward compatibility implications, would probably bring some >> performance decrease and could pose some difficulties in the implementation >> on the C++ side (in the handling of timed out operations that hold >> resources). >> >> >> The first option (a) is less risky and does not have impacts regarding >> backward compatibility and performance. >> >> The second one (b) is the preferred alternative in terms of the expected >> behavior from the users of the API. This option is more complex to >> implement and as mentioned above has performance and backward compatibility >> issues not easy to be solved. >> >> Following is a draft version of the implementation of b) on the Java >> client: >> >> https://github.com/Nordix/geode/commit/507a795e34c6083c129bda7e976b9223d1a893da >> >> Following is a draft version of the implementation of b) on the C++ native >> client: >> >> https://github.com/apache/geode-native/commit/a03a56f229bb8d75ee71044cf6196df07f43150d >> >> Note that the above implementation of b) in the C++ client implies that >> the Execution object returned by the FunctionService cannot be destroyed >> until the thread executing the function asynchronously has finished. If the >> function times out, the Execution object must be kept until the thread >> finishes. >> >> >> Other considerations >> - >> >> * Currently, in the function execution Java client there is not a >> possibility to set a timeout for the execution of functions. The closest to >> this is the read timeout that may be set globally for function executions >> but this is not really a timeout for operations. >> >> * Even if the API for function execution is the same on clients and >> servers, the implementation is different between them. On the clients, the >> execute() methods are blocking while on the servers it is non-blocking and >> the invoker of the function blocks on the getResult() method of the >> ResultCollector returned by the execute() method. >> Even if having both blocking and non-blocking implementation of execute() >> in both clients and servers sounds desirable from the point of view of >> orthogonality, this could bring complications in terms of backward >> compatibility. Besides, a need for a blocking version of function execution >> has not been found. >> >> -Alberto G. >> >> On 29/8/19 16:48, Alberto Gomez wrote: >> >> Sorry, some corrections on my comments after revisiting the native >> client code. >> >> When I said that the timeout used in the execution() method (by means of >> a system property) was to set a read timeout on the socket, I was only >> talking about the Java client. In the case of the native clients, the >> timeout set in the execute() method is not translated into a socket >> timeout but it is the time to wait for the operation to complete, i.e., >> to get all the results back. >> >> Things being so, I would change my proposal to: >> >> - Change the
Re: [DISCUSS] Improvements on client function execution API
+1 - Ok, I think I've come around to option (a). We can go head and add a new execute(timeout, TimeUnit) method to the java API that is blocking. We can leave the existing execute() method alone, except for documenting what it is doing. I would like implement execute(timeout, TimeUnit) on the server side as well. Since this Execution class is shared between both client and server APIs, it would be unfortunate to have a method on Execution that simply doesn't work on the server side. -Dan On Thu, Sep 5, 2019 at 9:25 AM Alberto Gomez wrote: > Hi all, > > First of all, thanks a lot Dan and Jacob for your feedback. > > As we are getting close to the deadline I am adding here some conclusions > and a refined proposal in order to get some more feedback and if possible > some voting on the two alternatives proposed (or any other in between if > you feel any of them is lacking something). > > I also add some draft code to try to clarify a bit the more complex of the > alternatives. > > > Proposal summary (needs a decision on which option to implement): > > --- > > In order to make the API more coherent two alternatives are proposed: > > a) Remove the timeout from the ResultCollector::getResult() / document > that the timeout has no effect, taking into account that > Execution::execute() is always blocking. > Additionally we could add the timeout parameter to the > Execution::execute() method of the Java API in order to align it with the > native client APIs. This timeout would not be the read timeout on the > socket but a timeout for the execution of the operation. > > b) Change the implementation of the Execution::execute() method without > timeout to be non-blocking on both the Java and native APIs. This change > has backward compatibility implications, would probably bring some > performance decrease and could pose some difficulties in the implementation > on the C++ side (in the handling of timed out operations that hold > resources). > > > The first option (a) is less risky and does not have impacts regarding > backward compatibility and performance. > > The second one (b) is the preferred alternative in terms of the expected > behavior from the users of the API. This option is more complex to > implement and as mentioned above has performance and backward compatibility > issues not easy to be solved. > > Following is a draft version of the implementation of b) on the Java > client: > > https://github.com/Nordix/geode/commit/507a795e34c6083c129bda7e976b9223d1a893da > > Following is a draft version of the implementation of b) on the C++ native > client: > > https://github.com/apache/geode-native/commit/a03a56f229bb8d75ee71044cf6196df07f43150d > > Note that the above implementation of b) in the C++ client implies that > the Execution object returned by the FunctionService cannot be destroyed > until the thread executing the function asynchronously has finished. If the > function times out, the Execution object must be kept until the thread > finishes. > > > Other considerations > - > > * Currently, in the function execution Java client there is not a > possibility to set a timeout for the execution of functions. The closest to > this is the read timeout that may be set globally for function executions > but this is not really a timeout for operations. > > * Even if the API for function execution is the same on clients and > servers, the implementation is different between them. On the clients, the > execute() methods are blocking while on the servers it is non-blocking and > the invoker of the function blocks on the getResult() method of the > ResultCollector returned by the execute() method. > Even if having both blocking and non-blocking implementation of execute() > in both clients and servers sounds desirable from the point of view of > orthogonality, this could bring complications in terms of backward > compatibility. Besides, a need for a blocking version of function execution > has not been found. > > -Alberto G. > > On 29/8/19 16:48, Alberto Gomez wrote: > > Sorry, some corrections on my comments after revisiting the native > client code. > > When I said that the timeout used in the execution() method (by means of > a system property) was to set a read timeout on the socket, I was only > talking about the Java client. In the case of the native clients, the > timeout set in the execute() method is not translated into a socket > timeout but it is the time to wait for the operation to complete, i.e., > to get all the results back. > > Things being so, I would change my proposal to: > > - Change the implementation of execute() on both Java and native clients > to be non-blocking (having the blocking/non-blocking behavior > configurable in the release this is introduced and leaving only the > non-blocking behavior in the next release). > > - Either remove the execute() with timeout
Re: [DISCUSS] Improvements on client function execution API
Hi all, First of all, thanks a lot Dan and Jacob for your feedback. As we are getting close to the deadline I am adding here some conclusions and a refined proposal in order to get some more feedback and if possible some voting on the two alternatives proposed (or any other in between if you feel any of them is lacking something). I also add some draft code to try to clarify a bit the more complex of the alternatives. Proposal summary (needs a decision on which option to implement): --- In order to make the API more coherent two alternatives are proposed: a) Remove the timeout from the ResultCollector::getResult() / document that the timeout has no effect, taking into account that Execution::execute() is always blocking. Additionally we could add the timeout parameter to the Execution::execute() method of the Java API in order to align it with the native client APIs. This timeout would not be the read timeout on the socket but a timeout for the execution of the operation. b) Change the implementation of the Execution::execute() method without timeout to be non-blocking on both the Java and native APIs. This change has backward compatibility implications, would probably bring some performance decrease and could pose some difficulties in the implementation on the C++ side (in the handling of timed out operations that hold resources). The first option (a) is less risky and does not have impacts regarding backward compatibility and performance. The second one (b) is the preferred alternative in terms of the expected behavior from the users of the API. This option is more complex to implement and as mentioned above has performance and backward compatibility issues not easy to be solved. Following is a draft version of the implementation of b) on the Java client: https://github.com/Nordix/geode/commit/507a795e34c6083c129bda7e976b9223d1a893da Following is a draft version of the implementation of b) on the C++ native client: https://github.com/apache/geode-native/commit/a03a56f229bb8d75ee71044cf6196df07f43150d Note that the above implementation of b) in the C++ client implies that the Execution object returned by the FunctionService cannot be destroyed until the thread executing the function asynchronously has finished. If the function times out, the Execution object must be kept until the thread finishes. Other considerations - * Currently, in the function execution Java client there is not a possibility to set a timeout for the execution of functions. The closest to this is the read timeout that may be set globally for function executions but this is not really a timeout for operations. * Even if the API for function execution is the same on clients and servers, the implementation is different between them. On the clients, the execute() methods are blocking while on the servers it is non-blocking and the invoker of the function blocks on the getResult() method of the ResultCollector returned by the execute() method. Even if having both blocking and non-blocking implementation of execute() in both clients and servers sounds desirable from the point of view of orthogonality, this could bring complications in terms of backward compatibility. Besides, a need for a blocking version of function execution has not been found. -Alberto G. On 29/8/19 16:48, Alberto Gomez wrote: Sorry, some corrections on my comments after revisiting the native client code. When I said that the timeout used in the execution() method (by means of a system property) was to set a read timeout on the socket, I was only talking about the Java client. In the case of the native clients, the timeout set in the execute() method is not translated into a socket timeout but it is the time to wait for the operation to complete, i.e., to get all the results back. Things being so, I would change my proposal to: - Change the implementation of execute() on both Java and native clients to be non-blocking (having the blocking/non-blocking behavior configurable in the release this is introduced and leaving only the non-blocking behavior in the next release). - Either remove the execute() with timeout methods in the native clients (with a deprecation release) or implement the execute(timeout) method in the Java client to be blocking (to work as the native client does today). In case the method times out, the connection will not be closed. If the operation times out due to the socket timeout (system property), then the connection will be closed as it is now done in the Java client. - Do not implement the blocking execute(timeout) method on the server and leave the current execute() implementation on the server as it is (non-blocking) Does this make sense? -Alberto On 29/8/19 12:56, Alberto Gómez wrote: Hi Dan, Discussing these matters by e-mail is getting tricky. Let's see if I
Re: [DISCUSS] Improvements on client function execution API
Sorry, some corrections on my comments after revisiting the native client code. When I said that the timeout used in the execution() method (by means of a system property) was to set a read timeout on the socket, I was only talking about the Java client. In the case of the native clients, the timeout set in the execute() method is not translated into a socket timeout but it is the time to wait for the operation to complete, i.e., to get all the results back. Things being so, I would change my proposal to: - Change the implementation of execute() on both Java and native clients to be non-blocking (having the blocking/non-blocking behavior configurable in the release this is introduced and leaving only the non-blocking behavior in the next release). - Either remove the execute() with timeout methods in the native clients (with a deprecation release) or implement the execute(timeout) method in the Java client to be blocking (to work as the native client does today). In case the method times out, the connection will not be closed. If the operation times out due to the socket timeout (system property), then the connection will be closed as it is now done in the Java client. - Do not implement the blocking execute(timeout) method on the server and leave the current execute() implementation on the server as it is (non-blocking) Does this make sense? -Alberto On 29/8/19 12:56, Alberto Gómez wrote: > Hi Dan, > > Discussing these matters by e-mail is getting tricky. > > Let's see if I understand you correctly and also if I am being clear > enough. > > Please, see my comments inline. > > On 29/8/19 0:49, Dan Smith wrote: >> 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. > > We have to take into account that, currently, the timeout in execute() > is not the same thing as the timeout in getResult(). > > On the one hand, the timeout set in execute() (via System property in > the Java client, and with a parameter in the native client) sets a > readtimeout on the socket which just means that if nothing is read > from the socket after sending the request to the server for the given > timeout, the corresponding exception will be thrown. It looks to me > more like a protection against possible communication failures rather > than a mechanism to decide if results took too long to be provided. So > I would not link the presence of the timeout parameter in the method > to the nature of the method (blocking or non-blocking). I think we > could have this read timeout set and at the same time keep that method > as non-blocking. > > On the other hand, the timeout set in getResult() is a timeout to wait > for all the results to be received from the moment the method is invoked. > > Therefore, I would not implement the blocking version of execute() on > the server by calling getResult() at the end. > > Apart from that, I doubt if it would make sense to set this > readTimeout in the execute() methods from servers given that the > communication is very different to the one done with clients. I also > doubt that anyone would be interested in the blocking version of > execute() on the server. > > My proposal was to add the readtimeout to the execute() methods in the > Java client in order to align the Java client and the native client. > This change would be independent to the decision we make regarding the > change of execute() to non-blocking. To achieve this alignment, > alternatively, we could remove the timeout parameter in execute() from > the native clients and have it as a global property for the client to > be set by whatever mechanism available as it is done in the Java > client today. > > Were you proposing that the execute() methods with timeout were > blocking and the ones without timeout non-blocking? Not sure if this > is something you meant. > >> >> 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
Re: [DISCUSS] Improvements on client function execution API
Hi Dan, Discussing these matters by e-mail is getting tricky. Let's see if I understand you correctly and also if I am being clear enough. Please, see my comments inline. On 29/8/19 0:49, Dan Smith wrote: > 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. We have to take into account that, currently, the timeout in execute() is not the same thing as the timeout in getResult(). On the one hand, the timeout set in execute() (via System property in the Java client, and with a parameter in the native client) sets a readtimeout on the socket which just means that if nothing is read from the socket after sending the request to the server for the given timeout, the corresponding exception will be thrown. It looks to me more like a protection against possible communication failures rather than a mechanism to decide if results took too long to be provided. So I would not link the presence of the timeout parameter in the method to the nature of the method (blocking or non-blocking). I think we could have this read timeout set and at the same time keep that method as non-blocking. On the other hand, the timeout set in getResult() is a timeout to wait for all the results to be received from the moment the method is invoked. Therefore, I would not implement the blocking version of execute() on the server by calling getResult() at the end. Apart from that, I doubt if it would make sense to set this readTimeout in the execute() methods from servers given that the communication is very different to the one done with clients. I also doubt that anyone would be interested in the blocking version of execute() on the server. My proposal was to add the readtimeout to the execute() methods in the Java client in order to align the Java client and the native client. This change would be independent to the decision we make regarding the change of execute() to non-blocking. To achieve this alignment, alternatively, we could remove the timeout parameter in execute() from the native clients and have it as a global property for the client to be set by whatever mechanism available as it is done in the Java client today. Were you proposing that the execute() methods with timeout were blocking and the ones without timeout non-blocking? Not sure if this is something you meant. > > 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. The problem I see with adding new non-blocking methods (new/submit...) is that it would be a solution for the current users of the client regarding backwards compatibility. But, on the server side, we would have to move the current logic of execute() which is non-blocking to the new methods and change the current execute() behavior to blocking. We would not impact the users of the client but we would impact the users of the server. Again, I would propose to aim at: a) either leave execute() on the client as blocking b) or change execute() on the client to be non-blocking but on the Geode release this is introduced, have it configurable. The default behavior would be blocking (deprecated behavior) but could be set to non-blocking with a system property. On the next release, the blocking behavior would be removed. - Alberto G. > -Dan > > On Fri, Aug 23, 2019 at 4:28 AM Alberto Gomez > 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 >> 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
Re: [DISCUSS] Improvements on client function execution API
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 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 > 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 >
Re: [DISCUSS] Improvements on client function execution API
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 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
Re: Fwd: [DISCUSS] Improvements on client function execution API
Hi Dan, Please, see my answers below. On 22/8/19 20:29, Dan Smith wrote: > For the two new execute() methods you are proposing, are you also > suggesting that they be implemented on the server? I was only thinking about the client. > > I took a look at the C++ API. The C++ API also has a > ResultCollector.getResult(timeout) method. Does that timeout do anything, > given that execute(timeout) already waited for the given timeout? The DefaultResultCollector in the C++ API handles the timeout with a condition variable in the getResult method (unlike the Java API) but as Jacob Barrett pointed out on another e-mail, given that the execute method is blocking, the timeout is pointless here too. > > Long term, are you thinking that we should remove execute() with no > parameters and getResult(timeout) and just have these two blocking > execute(timeout) methods on the server and clients? I think you are right > that we shouldn't change the behavior of the existing execute() to avoid > breaking our users. But I also want to understand what our long term plan > is for a consistent and sensible API. It depends on the option that gets selected. If we go for option a), the idea would be to keep in clients execute() with no parameters and have also execute() with timeout, and remove the getResult(timeout) method keeping just the getResult() one (which would be non-blocking as it is today). On the servers, I would leave things as they are, execute() is not blocking. Not sure if it makes sense to keep these differences between clients and servers and if it does, how we could maintain them in the API. The fact is that currently execute is very different in clients and in servers. If we go for option b), the idea would be to keep in clients execute() with no parameters and have also execute() with timeout in order not to lose the feature of closing connections if read timeout expires. The socket timeout exception would have to be thrown by the getResult() method instead of the execute method. The execute() methods would be non-blocking and it would be getResult() the one which is blocking, both on servers and clients. For option b) in order to deprecate the blocking behavior in execute() we could keep both implementations (blocking and non-blocking) and have a system property to select the behavior, which could be the blocking one by default in the release this is introduced. But long term, the aim would be to have just the non-blocking implementation of execute(). > > -Dan - Alberto
Re: [DISCUSS] Improvements on client function execution API
> On Aug 21, 2019, at 8:49 AM, Alberto Gomez 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
Re: [DISCUSS] Improvements on client function execution API
> On Aug 22, 2019, at 11:55 AM, Jacob Barrett wrote: >> On Aug 22, 2019, at 11:29 AM, Dan Smith wrote: >> I took a look at the C++ API. The C++ API also has a >> ResultCollector.getResult(timeout) method. Does that timeout do anything, >> given that execute(timeout) already waited for the given timeout? > > The C++ client will wait on getResult until all the results have arrived or > timeout has expired. If timeout has expired an exception is thrown. It does > not try to cleanup the connection or threads processing the responses from > the server. Those connections should either eventually get data and fill in a > now abandoned ResultsCollector or timeout based on the execute timeout and > close the socket. Scratch that… On further investigation the execute is blocking, but can timeout when timeout expires. The getResult timeout appears to really be pointless. -Jake
Re: [DISCUSS] Improvements on client function execution API
> On Aug 22, 2019, at 11:29 AM, Dan Smith wrote: > I took a look at the C++ API. The C++ API also has a > ResultCollector.getResult(timeout) method. Does that timeout do anything, > given that execute(timeout) already waited for the given timeout? The C++ client will wait on getResult until all the results have arrived or timeout has expired. If timeout has expired an exception is thrown. It does not try to cleanup the connection or threads processing the responses from the server. Those connections should either eventually get data and fill in a now abandoned ResultsCollector or timeout based on the execute timeout and close the socket. -Jake
Fwd: [DISCUSS] Improvements on client function execution API
For the two new execute() methods you are proposing, are you also suggesting that they be implemented on the server? I took a look at the C++ API. The C++ API also has a ResultCollector.getResult(timeout) method. Does that timeout do anything, given that execute(timeout) already waited for the given timeout? Long term, are you thinking that we should remove execute() with no parameters and getResult(timeout) and just have these two blocking execute(timeout) methods on the server and clients? I think you are right that we shouldn't change the behavior of the existing execute() to avoid breaking our users. But I also want to understand what our long term plan is for a consistent and sensible API. -Dan
Re: [DISCUSS] Improvements on client function execution API
Hi, Please, see my answers below. - Alberto On 21/8/19 20:30, Anilkumar Gingade wrote: > Just to be clear between java and native-client api: > > - Read timeout in function execution Java client API - This is to change > the java client behavior Yes > > And following are the native client problems and solution applies to > native-client? > - Timeout in ResultCollector::getResult() and Execution::execute() blocking This applies to all the clients, both to the Java and the native one (C++ and C# actually). > > -Anil. > > > On Wed, Aug 21, 2019 at 8:49 AM Alberto Gomez > wrote: > >> Hi, >> >> I have just added the following proposal in the wiki for discussion and >> would like to get feedback from the community. >> >> >> https://cwiki.apache.org/confluence/display/GEODE/%5BDiscussion%5D+Improvements+on+client+Function+execution+API >> >> Problem >> >> The client API for function execution is inconsistent in the following >> aspects: >> >> 1.Read timeout in function execution client API >> >> The client API for Function execution allows to set a timeout to wait for >> the execution of a function. >> >> Setting this timeout has the effect of setting a read timeout in the >> socket. If the timeout expires during the execution of a function before >> data has been received, the connection is closed and if the number of >> retries is reached, the execute method throws an exception. >> >> Nevertheless, how this timeout is set is not uniform across the different >> clients: >> >>* In the native C++ and C# clients, the timeout can be set on a per >> function execution basis by passing the timeout to the Execution::execute() >> method. >>* In the Java API, the timeout can only be set globally by a system >> property when starting the client process. >> >> 2. Timeout in ResultCollector::getResult() >> >> Apart from the timeout on the function execution, the client API offers >> the possibility of setting a timeout on the getResult() method of the >> ResultCollector object returned by the Execution::execute() method. >> >> Given that this method is blocking when invoked from a client (until all >> results have been received) then the setting of this timeout has no effect >> at all. In fact, the DefaultResultCollector in the Java API just ignores >> the value of the timeout. >> >> Note that this timeout in the ResultCollector::getResult() method is >> useful when used inside a peer as function invocations are not blocking. >> >> 3. Blocking Execution::execute() >> >> As mentioned above the Execution::execute() method behavior is different >> when invoked from clients than from peers. When invoked from clients it is >> blocking until all results are received while when invoked from peers it is >> non-blocking and the ResultCollector::getResult() method is used to wait to >> get all the results. >> >> This is not explicit in the documentation of the interface and it has >> already been captured in the following ticket: >> https://issues.apache.org/jira/browse/GEODE-3817 >> >> Anti-Goals >> >> - >> >> Solution >> >> In order to make the API more coherent two actions are proposed: >> >> 1. Read timeout in function execution Java client API >> >> Add two new Execution::execute() methods in the Java client to offer the >> possibility to set the timeout for the socket just as it is done in the C++ >> and C# clients. >> >> This action is simple to implement and there are no adverse effects >> attached to it. >> >> 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. >> >> 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. >> >> 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
Re: [DISCUSS] Improvements on client function execution API
Just to be clear between java and native-client api: - Read timeout in function execution Java client API - This is to change the java client behavior And following are the native client problems and solution applies to native-client? - Timeout in ResultCollector::getResult() and Execution::execute() blocking -Anil. On Wed, Aug 21, 2019 at 8:49 AM Alberto Gomez wrote: > Hi, > > I have just added the following proposal in the wiki for discussion and > would like to get feedback from the community. > > > https://cwiki.apache.org/confluence/display/GEODE/%5BDiscussion%5D+Improvements+on+client+Function+execution+API > > Problem > > The client API for function execution is inconsistent in the following > aspects: > > 1.Read timeout in function execution client API > > The client API for Function execution allows to set a timeout to wait for > the execution of a function. > > Setting this timeout has the effect of setting a read timeout in the > socket. If the timeout expires during the execution of a function before > data has been received, the connection is closed and if the number of > retries is reached, the execute method throws an exception. > > Nevertheless, how this timeout is set is not uniform across the different > clients: > > * In the native C++ and C# clients, the timeout can be set on a per > function execution basis by passing the timeout to the Execution::execute() > method. > * In the Java API, the timeout can only be set globally by a system > property when starting the client process. > > 2. Timeout in ResultCollector::getResult() > > Apart from the timeout on the function execution, the client API offers > the possibility of setting a timeout on the getResult() method of the > ResultCollector object returned by the Execution::execute() method. > > Given that this method is blocking when invoked from a client (until all > results have been received) then the setting of this timeout has no effect > at all. In fact, the DefaultResultCollector in the Java API just ignores > the value of the timeout. > > Note that this timeout in the ResultCollector::getResult() method is > useful when used inside a peer as function invocations are not blocking. > > 3. Blocking Execution::execute() > > As mentioned above the Execution::execute() method behavior is different > when invoked from clients than from peers. When invoked from clients it is > blocking until all results are received while when invoked from peers it is > non-blocking and the ResultCollector::getResult() method is used to wait to > get all the results. > > This is not explicit in the documentation of the interface and it has > already been captured in the following ticket: > https://issues.apache.org/jira/browse/GEODE-3817 > > Anti-Goals > > - > > Solution > > In order to make the API more coherent two actions are proposed: > > 1. Read timeout in function execution Java client API > > Add two new Execution::execute() methods in the Java client to offer the > possibility to set the timeout for the socket just as it is done in the C++ > and C# clients. > > This action is simple to implement and there are no adverse effects > attached to it. > > 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. > > 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. > > 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. > * Create a new class (ProxyResultCollector) that will hold a Future > for a ResultCollector and whose getResult() methods implementation would be > something like: > > return this.future.get().getResult(); > > * After creating the future that invokes > ServerRegionFunction::executeFunction() create an
[DISCUSS] Improvements on client function execution API
Hi, I have just added the following proposal in the wiki for discussion and would like to get feedback from the community. https://cwiki.apache.org/confluence/display/GEODE/%5BDiscussion%5D+Improvements+on+client+Function+execution+API Problem The client API for function execution is inconsistent in the following aspects: 1.Read timeout in function execution client API The client API for Function execution allows to set a timeout to wait for the execution of a function. Setting this timeout has the effect of setting a read timeout in the socket. If the timeout expires during the execution of a function before data has been received, the connection is closed and if the number of retries is reached, the execute method throws an exception. Nevertheless, how this timeout is set is not uniform across the different clients: * In the native C++ and C# clients, the timeout can be set on a per function execution basis by passing the timeout to the Execution::execute() method. * In the Java API, the timeout can only be set globally by a system property when starting the client process. 2. Timeout in ResultCollector::getResult() Apart from the timeout on the function execution, the client API offers the possibility of setting a timeout on the getResult() method of the ResultCollector object returned by the Execution::execute() method. Given that this method is blocking when invoked from a client (until all results have been received) then the setting of this timeout has no effect at all. In fact, the DefaultResultCollector in the Java API just ignores the value of the timeout. Note that this timeout in the ResultCollector::getResult() method is useful when used inside a peer as function invocations are not blocking. 3. Blocking Execution::execute() As mentioned above the Execution::execute() method behavior is different when invoked from clients than from peers. When invoked from clients it is blocking until all results are received while when invoked from peers it is non-blocking and the ResultCollector::getResult() method is used to wait to get all the results. This is not explicit in the documentation of the interface and it has already been captured in the following ticket: https://issues.apache.org/jira/browse/GEODE-3817 Anti-Goals - Solution In order to make the API more coherent two actions are proposed: 1. Read timeout in function execution Java client API Add two new Execution::execute() methods in the Java client to offer the possibility to set the timeout for the socket just as it is done in the C++ and C# clients. This action is simple to implement and there are no adverse effects attached to it. 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. 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. 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. * Create a new class (ProxyResultCollector) that will hold a Future for a ResultCollector and whose getResult() methods implementation would be something like: return this.future.get().getResult(); * After creating the future that invokes ServerRegionFunction::executeFunction() create an instance of ProxyResultCollector and call the setFuture() method on it passing the just created future and return it. Changes and Additions to Public Interfaces Regarding the first action, the Java client API would grow by adding the following two methods: * ResultCollector execute(String functionId, long timeout, TimeUnit unit) throws FunctionException; * ResultCollector execute(Function function, long timeout, TimeUnit unit) throws FunctionException; Depending on the option selected on the second action: *
