Re: Propose including GEODE-7178 in 1.10
My understanding is that this portion of the protocol is determined by instanceof checks, not the ordinal version. The messages from the java client went through a different code path than messages from the native client. So java clients using ordinal 45 still work (that’s why our backwards compatibility tests don’t fail). Anthony > On Sep 12, 2019, at 2:34 PM, Dan Smith wrote: > > +1 for getting this in 1.10. > > I am curious though - is the native client behaving like an older versions > of the java client, or is this totally unique behavior for the native > client? Is there some integration test that we are missing here? > > -Dan > > On Thu, Sep 12, 2019 at 11:52 AM Michael Oleske wrote: > >> Here is the Pull Request for the cherry pick as requested >> https://github.com/apache/geode/pull/4049 >> >> -michael >> >> On Thu, Sep 12, 2019 at 10:28 AM Dick Cavender >> wrote: >> >>> Hi Michael, thank you for bringing your concern and fixing this issue. >>> >>> Geode's release process dictates a time-based schedule < >>> https://cwiki.apache.org/confluence/display/GEODE/Release+Schedule> to >> cut >>> release branches. The “critical fixes” rule does allow critical fixes to >>> be brought to the release branch by proposal on the dev list, as you have >>> done here. >>> >>> If there is consensus from the Geode community that your proposed change >>> satisfies the “critical fixes” rule, I will be happy to bring it to the >>> 1.10.0 release branch. >>> >>> Due to the complexity of this change, could please open a PR against >>> release/1.10.0 containing the exact changes you want to merge? >>> >>> Regards >>> >>> -Dick >>> >>> >>> On Thu, Sep 12, 2019 at 10:23 AM Anthony Baker >> wrote: >>> +1 yes please! > On Sep 12, 2019, at 10:11 AM, Michael Oleske >>> wrote: > > Hi Geode Devs! > > I'd like to propose including the fix for GEODE-7178. This resolves >> an > issue that Ivan (https://markmail.org/message/dwwac42xmpo4xb2e) ran into in > 1.10 RC1. > > SHA: 91176d61df64bf1390cdba7b1cdc2b40cdfaba3a > Link to GitHub: > >>> >> https://github.com/apache/geode/commit/91176d61df64bf1390cdba7b1cdc2b40cdfaba3a > > Thanks! > -michael >>> >>
Re: Propose including GEODE-7178 in 1.10
The native client does behave as an old Java client (ordinal 45). I have written a story (https://issues.apache.org/jira/browse/GEODE-7190) to have Native Client updated. -michael On Thu, Sep 12, 2019 at 2:35 PM Dan Smith wrote: > +1 for getting this in 1.10. > > I am curious though - is the native client behaving like an older versions > of the java client, or is this totally unique behavior for the native > client? Is there some integration test that we are missing here? > > -Dan > > On Thu, Sep 12, 2019 at 11:52 AM Michael Oleske > wrote: > > > Here is the Pull Request for the cherry pick as requested > > https://github.com/apache/geode/pull/4049 > > > > -michael > > > > On Thu, Sep 12, 2019 at 10:28 AM Dick Cavender > > wrote: > > > > > Hi Michael, thank you for bringing your concern and fixing this issue. > > > > > > Geode's release process dictates a time-based schedule < > > > https://cwiki.apache.org/confluence/display/GEODE/Release+Schedule> to > > cut > > > release branches. The “critical fixes” rule does allow critical fixes > to > > > be brought to the release branch by proposal on the dev list, as you > have > > > done here. > > > > > > If there is consensus from the Geode community that your proposed > change > > > satisfies the “critical fixes” rule, I will be happy to bring it to the > > > 1.10.0 release branch. > > > > > > Due to the complexity of this change, could please open a PR against > > > release/1.10.0 containing the exact changes you want to merge? > > > > > > Regards > > > > > > -Dick > > > > > > > > > On Thu, Sep 12, 2019 at 10:23 AM Anthony Baker > > wrote: > > > > > > > +1 yes please! > > > > > > > > > On Sep 12, 2019, at 10:11 AM, Michael Oleske > > > wrote: > > > > > > > > > > Hi Geode Devs! > > > > > > > > > > I'd like to propose including the fix for GEODE-7178. This > resolves > > an > > > > > issue that Ivan (https://markmail.org/message/dwwac42xmpo4xb2e) > ran > > > > into in > > > > > 1.10 RC1. > > > > > > > > > > SHA: 91176d61df64bf1390cdba7b1cdc2b40cdfaba3a > > > > > Link to GitHub: > > > > > > > > > > > > > > > https://github.com/apache/geode/commit/91176d61df64bf1390cdba7b1cdc2b40cdfaba3a > > > > > > > > > > Thanks! > > > > > -michael > > > > > > > > > > > > > >
Re: Propose including GEODE-7178 in 1.10
+1 for getting this in 1.10. I am curious though - is the native client behaving like an older versions of the java client, or is this totally unique behavior for the native client? Is there some integration test that we are missing here? -Dan On Thu, Sep 12, 2019 at 11:52 AM Michael Oleske wrote: > Here is the Pull Request for the cherry pick as requested > https://github.com/apache/geode/pull/4049 > > -michael > > On Thu, Sep 12, 2019 at 10:28 AM Dick Cavender > wrote: > > > Hi Michael, thank you for bringing your concern and fixing this issue. > > > > Geode's release process dictates a time-based schedule < > > https://cwiki.apache.org/confluence/display/GEODE/Release+Schedule> to > cut > > release branches. The “critical fixes” rule does allow critical fixes to > > be brought to the release branch by proposal on the dev list, as you have > > done here. > > > > If there is consensus from the Geode community that your proposed change > > satisfies the “critical fixes” rule, I will be happy to bring it to the > > 1.10.0 release branch. > > > > Due to the complexity of this change, could please open a PR against > > release/1.10.0 containing the exact changes you want to merge? > > > > Regards > > > > -Dick > > > > > > On Thu, Sep 12, 2019 at 10:23 AM Anthony Baker > wrote: > > > > > +1 yes please! > > > > > > > On Sep 12, 2019, at 10:11 AM, Michael Oleske > > wrote: > > > > > > > > Hi Geode Devs! > > > > > > > > I'd like to propose including the fix for GEODE-7178. This resolves > an > > > > issue that Ivan (https://markmail.org/message/dwwac42xmpo4xb2e) ran > > > into in > > > > 1.10 RC1. > > > > > > > > SHA: 91176d61df64bf1390cdba7b1cdc2b40cdfaba3a > > > > Link to GitHub: > > > > > > > > > > https://github.com/apache/geode/commit/91176d61df64bf1390cdba7b1cdc2b40cdfaba3a > > > > > > > > Thanks! > > > > -michael > > > > > > > > >
Re: Propose including GEODE-7178 in 1.10
Here is the Pull Request for the cherry pick as requested https://github.com/apache/geode/pull/4049 -michael On Thu, Sep 12, 2019 at 10:28 AM Dick Cavender wrote: > Hi Michael, thank you for bringing your concern and fixing this issue. > > Geode's release process dictates a time-based schedule < > https://cwiki.apache.org/confluence/display/GEODE/Release+Schedule> to cut > release branches. The “critical fixes” rule does allow critical fixes to > be brought to the release branch by proposal on the dev list, as you have > done here. > > If there is consensus from the Geode community that your proposed change > satisfies the “critical fixes” rule, I will be happy to bring it to the > 1.10.0 release branch. > > Due to the complexity of this change, could please open a PR against > release/1.10.0 containing the exact changes you want to merge? > > Regards > > -Dick > > > On Thu, Sep 12, 2019 at 10:23 AM Anthony Baker wrote: > > > +1 yes please! > > > > > On Sep 12, 2019, at 10:11 AM, Michael Oleske > wrote: > > > > > > Hi Geode Devs! > > > > > > I'd like to propose including the fix for GEODE-7178. This resolves an > > > issue that Ivan (https://markmail.org/message/dwwac42xmpo4xb2e) ran > > into in > > > 1.10 RC1. > > > > > > SHA: 91176d61df64bf1390cdba7b1cdc2b40cdfaba3a > > > Link to GitHub: > > > > > > https://github.com/apache/geode/commit/91176d61df64bf1390cdba7b1cdc2b40cdfaba3a > > > > > > Thanks! > > > -michael > > > > >
Re: Propose including GEODE-7178 in 1.10
Hi Michael, thank you for bringing your concern and fixing this issue. Geode's release process dictates a time-based schedule < https://cwiki.apache.org/confluence/display/GEODE/Release+Schedule> to cut release branches. The “critical fixes” rule does allow critical fixes to be brought to the release branch by proposal on the dev list, as you have done here. If there is consensus from the Geode community that your proposed change satisfies the “critical fixes” rule, I will be happy to bring it to the 1.10.0 release branch. Due to the complexity of this change, could please open a PR against release/1.10.0 containing the exact changes you want to merge? Regards -Dick On Thu, Sep 12, 2019 at 10:23 AM Anthony Baker wrote: > +1 yes please! > > > On Sep 12, 2019, at 10:11 AM, Michael Oleske wrote: > > > > Hi Geode Devs! > > > > I'd like to propose including the fix for GEODE-7178. This resolves an > > issue that Ivan (https://markmail.org/message/dwwac42xmpo4xb2e) ran > into in > > 1.10 RC1. > > > > SHA: 91176d61df64bf1390cdba7b1cdc2b40cdfaba3a > > Link to GitHub: > > > https://github.com/apache/geode/commit/91176d61df64bf1390cdba7b1cdc2b40cdfaba3a > > > > Thanks! > > -michael > >
Re: Propose including GEODE-7178 in 1.10
+1 yes please! > On Sep 12, 2019, at 10:11 AM, Michael Oleske wrote: > > Hi Geode Devs! > > I'd like to propose including the fix for GEODE-7178. This resolves an > issue that Ivan (https://markmail.org/message/dwwac42xmpo4xb2e) ran into in > 1.10 RC1. > > SHA: 91176d61df64bf1390cdba7b1cdc2b40cdfaba3a > Link to GitHub: > https://github.com/apache/geode/commit/91176d61df64bf1390cdba7b1cdc2b40cdfaba3a > > Thanks! > -michael
Propose including GEODE-7178 in 1.10
Hi Geode Devs! I'd like to propose including the fix for GEODE-7178. This resolves an issue that Ivan (https://markmail.org/message/dwwac42xmpo4xb2e) ran into in 1.10 RC1. SHA: 91176d61df64bf1390cdba7b1cdc2b40cdfaba3a Link to GitHub: https://github.com/apache/geode/commit/91176d61df64bf1390cdba7b1cdc2b40cdfaba3a Thanks! -michael
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
