I think you could still use your decorator approach if you also unwrap the
Functions when returning them from the public APIs like getFunction etc. In
that case your TimingFunction could probably safely by added to
excludedClasses.txt.
You will miss collecting metrics about functions that aren't registered and
are invoked using Execution.execute(Function) but maybe that's intentional?
-Dan
On Wed, Sep 11, 2019 at 1:24 PM Mark Hanson <mhan...@pivotal.io> wrote:
> They would be the specific functions, but this doesn’t get us around they
> other problem. I think this approach is not going to work and we are about
> to start looking into other ways.
>
> Thanks,
> Mark
>
> > On Sep 11, 2019, at 12:14 PM, Anthony Baker <aba...@pivotal.io> 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 <alind...@pivotal.io>
> 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 <dsm...@pivotal.io> 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 <alind...@pivotal.io>
> >>> wrote:
> >>>
> >>>> As part of a PR to add Micrometer timers for function executions
> >>>> <https://github.com/apache/geode/pull/4038>, 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
> >>>>
> >>>
> >
>
>
