+1 Udo. A very clear and descriptive spec; Nice work!
In short, these would be welcomed changes and would have very little impact
to *Spring Data for Apache Geode*, which nicely abstracts this API even
further using its POJO-based, Annotation configuration model for Function
Implementations as well as Executions.
I too would prefer that there be a single method to obtain an instance of
the FunctionService. However, as we know this is not currently possible
without a clear and proper separation of the client and peer/server cache.
As Dan points out, while there are 2 separate interfaces for client and
peer/server caches, namely o.a.g.cache.client.ClientCache and
o.a.g.cache.Cache, the problem remains, there is only a single
implementation of both, o.a.g.internal.cache.GemFireCacheImpl, and
therefore, it is not possible to have a 2 exactly named methods that only
vary by return type in the same class (this is different than a subclass
having a different, compatible return type, which was only allowed as of
Java 8?).
I propose the following...
interface FunctionService {
onRegion(:Region);
}
interface ClientFunctionService extends FunctionService {
onServer(:Pool);
onServers(:Pool);
onServer(:RegionService);
onServers(:RegionService);
}
interface ServerFunctionService extends FunctionService {
onMember(:String...)
onMembers(:String...)
onMember(:DistributedMember)
onMembers(:DistributedMember[])
}
Then on the GemFireCache interface, you can declare...
interface GemFire extends RegionService {
<T extends FunctionService> T getFunctionService();
...
}
Then it is a simple matter for users to get reference to either, for
example from a client:
ClientFunctionService clientFunctionService =
clientCache.getFunctionService();
Or, from a server (peer):
ServerFunctionService serverFunctionService = cache.getFunctionService();
A (crude) implementation of getFunctionService() in
o.a.g.internal.cache.GemFireCacheImpl, would be...
@SuppressWarnings("unchecked")
public <T extends FunctionService> T getFunctionService() {
return (T) (isClient() ? new DefaultClientFunctionService() : new
DefaultServerFunctionService());
}
Of course, the implementation could be much more sophisticate than this.
The point being, I don't see the separate of the client and server logic
for cache functionality being split anytime soon and this would a good
interim solution that would not require any changes in the future.
I might even make the instantiation of the Client or Server based
FunctionServices accessible from a protected method for extension purpose
(i.e. subclassing), thereby making Apache Geode more extensible, so...
@SuppressWarnings("unchecked")
public <T extends FunctionService> T getFunctionService() {
return (T) (isClient() ? newClientFunctionService() :
newServerFunctionService());
}
protected ClientFunctionService newClientFunctionService() {
return new DefaultClientFunctionService();
}
protected ServerFunctionService newServerFunctionService() {
return new DefaultServerFunctionService();
}
It wouldn't be too difficult to imagine this being accomplished using
Java's SPI as well. Having a customizable/extensible API for Function
execution would be useful where users wanted to plugin different execution
handlers, for streaming or reactive purposes, for instance, and so on. If
the user is not satisfied with the defaults, regardless of concern, they
can plugin their own removing us from the equation. It does not preclude
us from providing OOTB implementations for these concerns, but they would
just be another provider like any other.
Food for thought.
Cheers,
John
On Thu, Mar 8, 2018 at 5:02 PM, Galen O'Sullivan <gosulli...@pivotal.io>
wrote:
> +1 for making the function service not static, and splitting servers from
> clients.
>
> Also +1 for Dan's suggestion.
>
> On Wed, Mar 7, 2018 at 2:51 PM, Patrick Rhomberg <prhomb...@pivotal.io>
> wrote:
>
> > I did not know that! And then, yes, onRegion is much better.
> >
> > On Wed, Mar 7, 2018 at 2:43 PM, Dan Smith <dsm...@pivotal.io> wrote:
> >
> > > > If we're not opposed to descriptive verbosity, I might prefer
> > > "onServersHostingRegion" more than "onRegion".
> > >
> > > onRegion does not really mean "on the servers hosting region XXX". It
> > only
> > > executes on a subset of the servers, potentially with retries, until it
> > has
> > > covered that entire dataset once. So I think onRegion is more
> > appropriate.
> > >
> > > -Dan
> > >
> > > On Wed, Mar 7, 2018 at 2:38 PM, Patrick Rhomberg <prhomb...@pivotal.io
> >
> > > wrote:
> > >
> > > > +1 for iteration towards better single responsibility design and more
> > > > easily-digestible classes.
> > > >
> > > > Regarding method names, I think that there would be some good utility
> > in
> > > > having "onGroup" methods, as well.
> > > > If we're not opposed to descriptive verbosity, I might prefer
> > > > "onServersHostingRegion" more than "onRegion".
> > > >
> > > > On Wed, Mar 7, 2018 at 1:45 PM, Dan Smith <dsm...@pivotal.io> wrote:
> > > >
> > > > > Hi Udo,
> > > > >
> > > > > +1 for making the function service not static and spitting it into
> > > client
> > > > > and server FunctionService objects!
> > > > >
> > > > > We do have Cache and ClientCache right now. So I would recommend
> this
> > > API
> > > > > rather than putting two methods on Cache. Cache is already the the
> > > server
> > > > > side API.
> > > > >
> > > > > Cache {
> > > > > ServerFunctionService getFunctionService()
> > > > > }
> > > > >
> > > > > ClientCache {
> > > > > ClientFunctionService getFunctionService()
> > > > > }
> > > > >
> > > > > If at some point we split the client side API into a separate jar
> the
> > > API
> > > > > shouldn't need to change. If you don't like ClientFunctionService,
> > > maybe
> > > > > o.a.g.cache.client.execute.FunctionService? We would never want
> two
> > > > > different versions of org.apache.geode.function.FunctionService.
> > > People
> > > > > wouldn't be able to test a client and server in the same JVM.
> > > > >
> > > > > Also, you might want to check out this (somewhat stalled) proposal
> -
> > > > > https://cwiki.apache.org/confluence/display/GEODE/
> > > > > Function+Service+Usability+Improvements. We had buy in on this on
> > the
> > > > dev
> > > > > list but have not found cycles to actually do it yet. But maybe now
> > is
> > > > the
> > > > > time?
> > > > >
> > > > > -Dan
> > > > >
> > > > > On Wed, Mar 7, 2018 at 11:18 AM, Udo Kohlmeyer <u...@apache.org>
> > wrote:
> > > > >
> > > > > > Hi there Apache Dev's,
> > > > > >
> > > > > > Please look at the proposal to improve the FunctionService and
> > remove
> > > > the
> > > > > > static invocation of it from within the Cache.
> > > > > >
> > > > > > https://cwiki.apache.org/confluence/display/GEODE/Function+
> > > > > > Service+Refactor+-+Removal+of+static-ness+and+splitting+of+
> > > > > > client+and+server-side+FunctionService
> > > > > >
> > > > > > --Udo
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
--
-John
john.blum10101 (skype)
