+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)