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 executio
Re: Question about excluding serialized classes
Jake — We're not constructing a new object each time a function is invoked
because the "decorated" functions are only created when a function is
registered, but we'll keep your concern in mind.
Thanks for all of the feedback from everyone. I think we have the
information we need to move forward. In summary, we'll have to change our
implementation such that the methods in FunctionService still return the
same objects they did before our changes.
- Aaron
On Mon, Sep 16, 2019 at 7:54 PM Jacob Barrett wrote:
> 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
> >
>
> >>
> >
>
>
Re: Question about excluding serialized classes
Not all functions are registered. I can invoke a function with
Execution.execute(Function) from the client, the Function is serialized and
executed on the server, the class need only exist on the server for
deserialization. Must a function be registered now to get metrics?
> On Sep 17, 2019, at 8:44 AM, Aaron Lindsey wrote:
>
> Jake — We're not constructing a new object each time a function is invoked
> because the "decorated" functions are only created when a function is
> registered, but we'll keep your concern in mind.
>
> Thanks for all of the feedback from everyone. I think we have the
> information we need to move forward. In summary, we'll have to change our
> implementation such that the methods in FunctionService still return the
> same objects they did before our changes.
>
> - Aaron
>
>
> On Mon, Sep 16, 2019 at 7:54 PM Jacob Barrett wrote:
>
>> 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
>> co
Re: Question about excluding serialized classes
> On Sep 16, 2019, at 7:54 PM, Jacob Barrett wrote: > > The current implementation has statistics without decoration. For our meters, we want to make a distinction that the current stats implementation does not: We want to measure only non-internal functions. It isn’t clear (yet) how we can inform the function stats constructor so that it knows whether to create the meters. Cheers, Dale — Dale Emery dem...@pivotal.io
Re: Question about excluding serialized classes
> > Not all functions are registered. I can invoke a function with > Execution.execute(Function) from the client, the Function is serialized and > executed on the server, the class need only exist on the server for > deserialization. Must a function be registered now to get metrics? > For this meter we're only interested in timing registered functions. - Aaron On Tue, Sep 17, 2019 at 9:18 AM Dale Emery wrote: > > > On Sep 16, 2019, at 7:54 PM, Jacob Barrett wrote: > > > > The current implementation has statistics without decoration. > > For our meters, we want to make a distinction that the current stats > implementation does not: We want to measure only non-internal functions. It > isn’t clear (yet) how we can inform the function stats constructor so that > it knows whether to create the meters. > > Cheers, > Dale > > — > Dale Emery > dem...@pivotal.io > >
Re: Question about excluding serialized classes
> On Sep 17, 2019, at 9:36 AM, Aaron Lindsey wrote: > >> >> Not all functions are registered. I can invoke a function with >> Execution.execute(Function) from the client, the Function is serialized and >> executed on the server, the class need only exist on the server for >> deserialization. Must a function be registered now to get metrics? >> > > For this meter we're only interested in timing registered functions. Can you explain why ya’ll aren’t interested in the more common use case? Also, are you aware this will likely result in a mismatch in meters for the same function between the client and the server. If the client has the function registered and the function is executed by name then the internals fetch the registered function object and effectively executes as Execution.execute(Function). This means that the client side will update meters for this function but when deserialized on the server to execute it would not update meters on the server. Only the case where the client doesn't have the Function registered and executes by name but the server does have it registered and looks up the instance would you end up with metrics updated on both sides. -Jake
Re: Question about excluding serialized classes
Hi Jake, I think this thread has moved a bit off-topic, but I appreciate you pointing out that scenario. We'll have to consider that when re-working this PR. - Aaron On Tue, Sep 17, 2019 at 9:42 AM Jacob Barrett wrote: > > > On Sep 17, 2019, at 9:36 AM, Aaron Lindsey wrote: > > > >> > >> Not all functions are registered. I can invoke a function with > >> Execution.execute(Function) from the client, the Function is serialized > and > >> executed on the server, the class need only exist on the server for > >> deserialization. Must a function be registered now to get metrics? > >> > > > > For this meter we're only interested in timing registered functions. > > Can you explain why ya’ll aren’t interested in the more common use case? > > Also, are you aware this will likely result in a mismatch in meters for > the same function between the client and the server. If the client has the > function registered and the function is executed by name then the internals > fetch the registered function object and effectively executes as > Execution.execute(Function). This means that the client side will update > meters for this function but when deserialized on the server to execute it > would not update meters on the server. Only the case where the client > doesn't have the Function registered and executes by name but the server > does have it registered and looks up the instance would you end up with > metrics updated on both sides. > > -Jake > >
