RE: resource manager requirements & recommendations
Thanks for the answer and the links Anthony, the discussion is interesting. I think the differences between using CMS and G1 should be documented, I will to contribute in this topic. For example, we have found these comments in a GemFire support ticket (https://community.pivotal.io/s/question/0D50e5q9JT0CAM/please-refer-to-the-pivotal-ticket-210727): "First, we are not completely compatible with G1GC yet in GemFire, meaning that some features, percentages, etc., in GemFire need to be rethought out if changing from CMS to G1. For example, if using eviction or critical thresholds, with CMS, these percentages would be a % of "Tenured" heap size. For G1GC, they would be a % of "Total" heap size, because as you may realize, G1GC doesn't have a max Eden space or max Tenured space." De: Anthony Baker Enviado: miércoles, 11 de septiembre de 2019 18:58 Para: dev@geode.apache.org Asunto: Re: resource manager requirements & recommendations The challenge with designing a good approach for managing heap use in Java is that we *can’t* know how much of the current heap use is really garbage. That means that it can be really easy to evict too much or too little data. With the CMS engine there are tuning parameters like occupancy fraction that you can set to match the eviction threshold. This leads to a fairly predictable approach to managing heap memory. With G!GC, the challenge is harder since the entire heap might fill up with garbage before any collections occur. Despite CMS being deprecated, I think it’s currently the best choice to control heap use in Geode. As noted in JEP 291 [1] and subsequent discussion [2]: "For some applications CMS is a very good fit and might always outperform G1”. I also think we need to do more work in this area to make G1 perform as well as CMS. Anthony [1] http://openjdk.java.net/jeps/291 [2] http://mail.openjdk.java.net/pipermail/jdk9-dev/2017-April/thread.html#start > On Sep 11, 2019, at 9:14 AM, Alberto Bustamante Reyes > wrote: > > Hi all, > > Im interested on using the resource manager with G1 garbage collector. To > check if it is possible, I have been reading documentation about heap memory > management and I came up with some questions because there are some points in > the documentation where it is not clear for me if they are describing > requirements or recommendations. > > As far as I understood, the requirements for using the Resource Manager are > only two: > > * set the critical heap percentage > * configure your GC properly in order to work before the eviction > procedure starts. > > Am I right? There are three points in the documentation that makes me > question if I'm correct: > > > 1. The first chapter in > https://geode.apache.org/docs/guide/19/managing/heap_use/heap_management.html > states how to configure your GC for improving performance, but it only talks > about CMS, there is no info about other GCs. > 2. In the steps of how to configure ResourceManager, when talking about > tuning GC parameters, it talks again only about CMS. > 3. In the documentation of ResourceManager class, setCriticalHeapPercentage > method, it is stated the following: > > Many virtual machine implementations have additional VM switches to control > the behavior of the garbage collector. We suggest that you investigate tuning > the garbage collector when using this type of eviction controller. A > collector that frequently collects is needed to keep our heap usage up to > date. In particular, on the Sun HotSpot VM, the -XX:+UseConcMarkSweepGC flag > needs to be set, [...] > > So it seems that CMS is a requirement, but I have not found in the code any > limitation about using only CMS. > > If my previous statement about the requirements is fine, then I suppose the > documentation needs a review to distinguish between generic requirements and > the CMS specific use case. > > Other question that come to my mind is about the lack of info about G1. As > CMS is deprecated since Java 9, are there any plans to test and document G1 > configuration? > > Thanks in advance for your comments! > > Alberto B. > > > > > >
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 abou
Re: [VOTE] Adding new AEQ feature to release/1.10.0
+1 On Fri, Sep 13, 2019 at 11:59 PM Ryan McMahon wrote: > +1 > > On Fri, Sep 13, 2019 at 3:58 PM Donal Evans wrote: > > > +1 > > > > On Fri, Sep 13, 2019 at 3:37 PM Benjamin Ross wrote: > > > > > +1 > > > > > > On Fri, Sep 13, 2019 at 3:25 PM Anilkumar Gingade > > > > wrote: > > > > > > > +1. This is needed for spring data-geode; whose upcoming release is > > based > > > > on older geode version. > > > > > > > > -Anil. > > > > > > > > > > > > On Fri, Sep 13, 2019 at 3:23 PM Nabarun Nag wrote: > > > > > > > > > Hi Geode Community , > > > > > > > > > > [GEODE-7121] > > > > > > > > > > I would like to include the new feature of creating AEQs with a > > paused > > > > > event processor to the release 1.10 branch. This also includes the > > > > feature > > > > > to resume the AEQ at a later point in time. > > > > > This feature includes addition of new/modified APIs and gfsh > > commands. > > > > > > > > > > [All details about this feature has been discussed in a previous > > > discuss > > > > > thread] > > > > > > > > > > These are the commits that needs to be in release 1.10.0 branch. > > > > > f6e11084daa30791f7bbf9a8187f6d1bc9c4b91a > > > > > 615d3399d24810126a6d57b5163f7afcd06366f7 > > > > > 1440a95e266e671679a623f93865c5e7e683244f > > > > > 42e07dc9054794657acb40c292f3af74b79a1ea6 > > > > > e1f200e2f9e77e986d250fde3848dc004b26a7c2 > > > > > 5f70160fba08a06c7e1fc48c7099e63dd1a0502b > > > > > 0645446ec626bc351a2c881e4df6a4ae2e75fbfc > > > > > 575c6bac115112df1e84455b052566c75764b0be > > > > > 3d9627ff16443f4aa513a67bcc284e68953aff8a > > > > > ea22e72916f8e34455800d347690e483727f9bf5 > > > > > 8d26d595f5fb94ff703116eb91bb747e9ba7f536 > > > > > > > > > > Will create a PR ASAP. > > > > > > > > > > Regards > > > > > Nabarun Nag > > > > > > > > > > > > > > > -- Juan José Ramos Cassella Senior Software Engineer Email: jra...@pivotal.io
Re: [VOTE] Adding new AEQ feature to release/1.10.0
+1 On Mon, Sep 16, 2019 at 3:08 AM Juan José Ramos wrote: > +1 > > On Fri, Sep 13, 2019 at 11:59 PM Ryan McMahon > wrote: > > > +1 > > > > On Fri, Sep 13, 2019 at 3:58 PM Donal Evans wrote: > > > > > +1 > > > > > > On Fri, Sep 13, 2019 at 3:37 PM Benjamin Ross > wrote: > > > > > > > +1 > > > > > > > > On Fri, Sep 13, 2019 at 3:25 PM Anilkumar Gingade < > aging...@pivotal.io > > > > > > > wrote: > > > > > > > > > +1. This is needed for spring data-geode; whose upcoming release is > > > based > > > > > on older geode version. > > > > > > > > > > -Anil. > > > > > > > > > > > > > > > On Fri, Sep 13, 2019 at 3:23 PM Nabarun Nag > wrote: > > > > > > > > > > > Hi Geode Community , > > > > > > > > > > > > [GEODE-7121] > > > > > > > > > > > > I would like to include the new feature of creating AEQs with a > > > paused > > > > > > event processor to the release 1.10 branch. This also includes > the > > > > > feature > > > > > > to resume the AEQ at a later point in time. > > > > > > This feature includes addition of new/modified APIs and gfsh > > > commands. > > > > > > > > > > > > [All details about this feature has been discussed in a previous > > > > discuss > > > > > > thread] > > > > > > > > > > > > These are the commits that needs to be in release 1.10.0 branch. > > > > > > f6e11084daa30791f7bbf9a8187f6d1bc9c4b91a > > > > > > 615d3399d24810126a6d57b5163f7afcd06366f7 > > > > > > 1440a95e266e671679a623f93865c5e7e683244f > > > > > > 42e07dc9054794657acb40c292f3af74b79a1ea6 > > > > > > e1f200e2f9e77e986d250fde3848dc004b26a7c2 > > > > > > 5f70160fba08a06c7e1fc48c7099e63dd1a0502b > > > > > > 0645446ec626bc351a2c881e4df6a4ae2e75fbfc > > > > > > 575c6bac115112df1e84455b052566c75764b0be > > > > > > 3d9627ff16443f4aa513a67bcc284e68953aff8a > > > > > > ea22e72916f8e34455800d347690e483727f9bf5 > > > > > > 8d26d595f5fb94ff703116eb91bb747e9ba7f536 > > > > > > > > > > > > Will create a PR ASAP. > > > > > > > > > > > > Regards > > > > > > Nabarun Nag > > > > > > > > > > > > > > > > > > > > > > > -- > Juan José Ramos Cassella > Senior Software Engineer > Email: jra...@pivotal.io >
Re: [VOTE] Adding new AEQ feature to release/1.10.0
Naba- Do you have this PR ready to merge to release/1.10.0 since we seem to have the votes for this to happen? On Mon, Sep 16, 2019 at 7:08 AM Jens Deppe wrote: > +1 > > On Mon, Sep 16, 2019 at 3:08 AM Juan José Ramos wrote: > > > +1 > > > > On Fri, Sep 13, 2019 at 11:59 PM Ryan McMahon > > wrote: > > > > > +1 > > > > > > On Fri, Sep 13, 2019 at 3:58 PM Donal Evans > wrote: > > > > > > > +1 > > > > > > > > On Fri, Sep 13, 2019 at 3:37 PM Benjamin Ross > > wrote: > > > > > > > > > +1 > > > > > > > > > > On Fri, Sep 13, 2019 at 3:25 PM Anilkumar Gingade < > > aging...@pivotal.io > > > > > > > > > wrote: > > > > > > > > > > > +1. This is needed for spring data-geode; whose upcoming release > is > > > > based > > > > > > on older geode version. > > > > > > > > > > > > -Anil. > > > > > > > > > > > > > > > > > > On Fri, Sep 13, 2019 at 3:23 PM Nabarun Nag > > wrote: > > > > > > > > > > > > > Hi Geode Community , > > > > > > > > > > > > > > [GEODE-7121] > > > > > > > > > > > > > > I would like to include the new feature of creating AEQs with a > > > > paused > > > > > > > event processor to the release 1.10 branch. This also includes > > the > > > > > > feature > > > > > > > to resume the AEQ at a later point in time. > > > > > > > This feature includes addition of new/modified APIs and gfsh > > > > commands. > > > > > > > > > > > > > > [All details about this feature has been discussed in a > previous > > > > > discuss > > > > > > > thread] > > > > > > > > > > > > > > These are the commits that needs to be in release 1.10.0 > branch. > > > > > > > f6e11084daa30791f7bbf9a8187f6d1bc9c4b91a > > > > > > > 615d3399d24810126a6d57b5163f7afcd06366f7 > > > > > > > 1440a95e266e671679a623f93865c5e7e683244f > > > > > > > 42e07dc9054794657acb40c292f3af74b79a1ea6 > > > > > > > e1f200e2f9e77e986d250fde3848dc004b26a7c2 > > > > > > > 5f70160fba08a06c7e1fc48c7099e63dd1a0502b > > > > > > > 0645446ec626bc351a2c881e4df6a4ae2e75fbfc > > > > > > > 575c6bac115112df1e84455b052566c75764b0be > > > > > > > 3d9627ff16443f4aa513a67bcc284e68953aff8a > > > > > > > ea22e72916f8e34455800d347690e483727f9bf5 > > > > > > > 8d26d595f5fb94ff703116eb91bb747e9ba7f536 > > > > > > > > > > > > > > Will create a PR ASAP. > > > > > > > > > > > > > > Regards > > > > > > > Nabarun Nag > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > Juan José Ramos Cassella > > Senior Software Engineer > > Email: jra...@pivotal.io > > >
Re: [VOTE] Adding new AEQ feature to release/1.10.0
No not yet. The documentation tickets got resolved Friday afternoon and hence the PR will be created today. Regards Naba On Mon, Sep 16, 2019 at 8:52 AM Dick Cavender wrote: > Naba- > > Do you have this PR ready to merge to release/1.10.0 since we seem to have > the votes for this to happen? > > > > On Mon, Sep 16, 2019 at 7:08 AM Jens Deppe wrote: > > > +1 > > > > On Mon, Sep 16, 2019 at 3:08 AM Juan José Ramos > wrote: > > > > > +1 > > > > > > On Fri, Sep 13, 2019 at 11:59 PM Ryan McMahon > > > wrote: > > > > > > > +1 > > > > > > > > On Fri, Sep 13, 2019 at 3:58 PM Donal Evans > > wrote: > > > > > > > > > +1 > > > > > > > > > > On Fri, Sep 13, 2019 at 3:37 PM Benjamin Ross > > > wrote: > > > > > > > > > > > +1 > > > > > > > > > > > > On Fri, Sep 13, 2019 at 3:25 PM Anilkumar Gingade < > > > aging...@pivotal.io > > > > > > > > > > > wrote: > > > > > > > > > > > > > +1. This is needed for spring data-geode; whose upcoming > release > > is > > > > > based > > > > > > > on older geode version. > > > > > > > > > > > > > > -Anil. > > > > > > > > > > > > > > > > > > > > > On Fri, Sep 13, 2019 at 3:23 PM Nabarun Nag > > > wrote: > > > > > > > > > > > > > > > Hi Geode Community , > > > > > > > > > > > > > > > > [GEODE-7121] > > > > > > > > > > > > > > > > I would like to include the new feature of creating AEQs > with a > > > > > paused > > > > > > > > event processor to the release 1.10 branch. This also > includes > > > the > > > > > > > feature > > > > > > > > to resume the AEQ at a later point in time. > > > > > > > > This feature includes addition of new/modified APIs and gfsh > > > > > commands. > > > > > > > > > > > > > > > > [All details about this feature has been discussed in a > > previous > > > > > > discuss > > > > > > > > thread] > > > > > > > > > > > > > > > > These are the commits that needs to be in release 1.10.0 > > branch. > > > > > > > > f6e11084daa30791f7bbf9a8187f6d1bc9c4b91a > > > > > > > > 615d3399d24810126a6d57b5163f7afcd06366f7 > > > > > > > > 1440a95e266e671679a623f93865c5e7e683244f > > > > > > > > 42e07dc9054794657acb40c292f3af74b79a1ea6 > > > > > > > > e1f200e2f9e77e986d250fde3848dc004b26a7c2 > > > > > > > > 5f70160fba08a06c7e1fc48c7099e63dd1a0502b > > > > > > > > 0645446ec626bc351a2c881e4df6a4ae2e75fbfc > > > > > > > > 575c6bac115112df1e84455b052566c75764b0be > > > > > > > > 3d9627ff16443f4aa513a67bcc284e68953aff8a > > > > > > > > ea22e72916f8e34455800d347690e483727f9bf5 > > > > > > > > 8d26d595f5fb94ff703116eb91bb747e9ba7f536 > > > > > > > > > > > > > > > > Will create a PR ASAP. > > > > > > > > > > > > > > > > Regards > > > > > > > > Nabarun Nag > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > Juan José Ramos Cassella > > > Senior Software Engineer > > > Email: jra...@pivotal.io > > > > > >
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
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 functi
Re: Question about excluding serialized classes
Sorry I am entering this late in the game but why do we need to decorate the
function at all for metrics. The current implementation has statistics without
decoration. All the concerns raised in this thread concern me as well as the
cost of constructing yet another object every time a function is invoked by
serialized function, Execution.execute(Function). Each allocation puts more
pressure on the GC, decreases our throughput and increases our latency.
-Jake
> On Sep 11, 2019, at 2:29 PM, Dale Emery wrote:
>
> The stats use the ID of the function, and each TimingFunction reports the
> same ID as the function it wraps. So I think the stats would look like they
> always did.
>
> Dale
>
> —
> Dale Emery
> dem...@pivotal.io
>
>
>
>> On Sep 11, 2019, at 12:14 PM, Anthony Baker wrote:
>>
>> I think the Decorator approach you outlined could have other impacts as
>> well. Would I still be able to see specific function executions in
>> statistics or would they all become “TImingFunction”?
>>
>> Anthony
>>
>>
>>> On Sep 11, 2019, at 12:00 PM, Aaron Lindsey wrote:
>>>
>>> Thanks for your response, Dan.
>>>
>>> The second scenario you mentioned (i.e. users calling
>>> FunctionService.getFunction(String)) worries me because our PR changes the
>>> FunctionService so they would now get back an instance of the decorator
>>> function instead of the specific instance they registered by calling
>>> FunctionService.registerFunction(Function). Therefore, any explicit casts
>>> to a Function implementation like (MyFunction)
>>> FunctionService.getFunction("MyFunction") would fail. Does that mean this
>>> be a breaking change? The FunctionService class does not specify that
>>> getFunction must return the same type function as the one passed to
>>> registerFunction, but I could see how users might be relying on that
>>> behavior since there is no other way to get a specific function type out of
>>> the FunctionService without doing a cast.
>>>
>>> - Aaron
>>>
>>>
>>> On Wed, Sep 11, 2019 at 10:52 AM Dan Smith wrote:
>>>
Functions are serialized when you call Execution.execute(Function) instead
of Execution.execute(String). Invoking execute on a function object
serializes the function and executes it on the remote side. Functions
executed this way don't have be registered.
Users can also get registered function objects directly from the function
service using FunctionService.getFunction(String) and do whatever they want
with them, which I guess could include serializing them.
Hope that helps!
-Dan
On Wed, Sep 11, 2019 at 10:27 AM Aaron Lindsey
wrote:
> As part of a PR to add Micrometer timers for function executions
> , we implemented a decorator
> Function that wraps all registered non-internal functions and adds
> instrumentation. This PR is
> failing AnalyzeSerializablesJUnitTest.testSerializables because the
> decorator class is a new Serializable.
>
> I'm not sure if it would be OK to add this class to excludedClasses.txt
> because I don't know whether this function will ever be serialized. If it
> will be serialized, then I'm concerned that this might break backwards
> compatibility because we're changing the serialized form of registered
> functions. If this is the case, then we could implement custom logic for
> serializing the decorator class which would replace its serialized form
> with the serialized form of the inner class. Again, I'm not sure if that
> would be necessary because I don't know the conditions under which a
> function would be serialized.
>
> Could someone help me understand when functions would be persisted or
sent
> over the wire so I can determine if this change would break
compatibility?
>
> Thanks,
> Aaron
>
>>
>
