Thank you for all the comments/suggestions. I've updated the wiki page with more details and scope of this api.
https://cwiki.apache.org/confluence/display/GEODE/Public+API+for+Cluster+Configuration Thanks! On Wed, Mar 14, 2018 at 11:57 AM, John Blum <jb...@pivotal.io> wrote: > *> @Jake, I vaguely remember that we had once spoken about the concept of > nouns and verbs for commands. Maybe this could be a start, understanding > what the nouns are and then understand the verbs that should go with them. > That might actually start flushing out the configuration services. i.e > noun: region verb: create, update, destroy, clear* > > This was exactly the intent behind the Management REST-like API when it was > designed, i.e. proper resource abstractions such as *Regions*, *Indexes*, > *DiskStores*, etc along with actions on the resources (with REST over HTTP, > the verbs being: POST, GET, PUT/PATCH, DELETE) and eventually hypermedia > controls to allow for a discoverable API (by tooling, for instance). > However, on the *Richardson's Maturity Model*, it never quite reached full > maturity since the initial intent was to just provide an HTTP tunnel > between > *Gfsh* and the Apache Geode *Manager*. However, it would not have been so > much work to continue down that path if the Management REST API had not > changed as it has now. It was also language neutral. > > I do have more thoughts on our (ab)use/notion of "internal" API(s), but I > will save it for another thread/time as it does not directly relate to this > topic. In a nutshell, it is concerned with putting interfaces and classes > in "internal" packages and claiming they are for internal use only, when > occasionally, there is no public API to accomplish the task at hand. Also, > you don't need something heavy like OSGi, or even Java 9 modules, to > properly enforce truly internal APIs either. Finally, I would argue that > most things (if not all) should just be in the public API, i.e. with proper > interfaces (APIs and/or SPIs) no one should really have to worry about > accessing "implementation" classes. > > Regards, > John > > > > On Wed, Mar 14, 2018 at 11:40 AM, Dan Smith <dsm...@pivotal.io> wrote: > > > If I understand it correctly, the intended users of this public API are > > people writing extensions to geode that need to save their own > > configuration elements, is that correct? It might be good to spell out > in a > > little more detail how that will work. Does CacheConfig have methods that > > let you add extension elements? > > > > The section on concurrency seems a bit hand-wavy. What specifically is > the > > API guaranteeing? That the cluster configuration will be consistent on > all > > locators and it will be the result of the last write? When do I need to > use > > a distributed lock? > > > > -Dan > > > > > > > > On Wed, Mar 14, 2018 at 9:17 AM, Jacob Barrett <jbarr...@pivotal.io> > > wrote: > > > > > I echo everything John Said. > > > > > > I also want to throw out that we have a “public api” for config in > sorts > > > through gfsh. I am by no means suggesting users programmatically invoke > > > command lines to gfsh but what if we could leverage gfsh as a console > > > terminal app, a Java API, or REST daemon? A common interface/command > set > > to > > > rule them all. It is not fun as a plug-in developer to have to > implement > > > multiple interfaces for multiple configuration systems. > > > > > > The other thing I’ll toss out is rather than rework what we have how > > about > > > replacing it all with something already mature. There are many other > open > > > source projects that have solved the configuration problem. Let’s not > > > reinvent the wheel here. > > > > > > -Jake > > > > > > > > > > > > > On Mar 13, 2018, at 1:59 PM, John Blum <jb...@pivotal.io> wrote: > > > > > > > > Some initial thoughts after looking over the spec... > > > > > > > > > > > > 1. As a developer (application, API/framework, tool or even a module > > > > developer), I really don't care nor should I care what Apache Geode > is > > > > storing the configuration as under-the-hood. > > > > > > > > It could be XML, JSON, YAML, a database or even some configuration > > > server. > > > > It is an implementation detail and I simply don't care; i.e. nothing > in > > > the > > > > interface or types should suggest otherwise. I am not saying this is > > the > > > > case now, just to be mindful of this when refining this API. > > > > > > > > Things like... > > > > > > > > 1.1. "... *as well as injection of elements into the cache and region > > > > levels of the cluster configuration.*" > > > > 1.2. "*Define a no-op interfaces CacheElement and RegionElement to > > > identify > > > > classes that may be saved to the cache and region XML entities*" > > > > 1.3. "*Leverage JAXB to generate an initial set of configuration > > > objects.*" > > > > > > > > .. are dangerously suspicious. > > > > > > > > > > > > 2. "*Expose ClusterConfigurationService via the cache, provided a > > locator > > > > is managing that cache.*" > > > > > > > > I agree with everything but the Locator bit, which brings me back to > > > > something I mentioned/suggested during the inception. The Management > > > > capabilities/functionality should also be a "Service", which is > > > > invokable/runnable from any node (not just Locators). It is not > > > uncommon, > > > > and I would even argue, it (is/ought to be) recommended that clusters > > > > consist of dedicated, standalone Managers. If Locators are > overloaded > > > and > > > > go down, then clients would not be able to discover servers, peers > > > wouldn't > > > > be able to be added in a (automated/managed) scale-up environment > like > > > PCF > > > > on AWS/GCP/Azure, etc. > > > > > > > > As such, the ManagementService is somewhat of a dependency for the > > > > ClusterConfigurationService. Perhaps that is not conducive to the > > > > implementation today given the amount of coupling/dependencies on the > > > > *Locator*, but it is far more logical. > > > > > > > > I see the ClusterConfigurationService as an extension of the > Management > > > > capabilities. > > > > > > > > I would also assume this ClusterConfigurationService is accessible > > from a > > > > o.a.g.cache.client.ClientCache application? > > > > > > > > > > > > 3. It would be nice to provide overloaded variants of > > getCacheConfig(..) > > > > and updateCacheConfig(..) that do not require "Group" if it defaults > > to " > > > > *cluster*" anyway. > > > > > > > > It is a simple matter to provide a default method in the interface of > > the > > > > nature, for example... > > > > > > > > public static final String DEFAULT_GROUP = "cluster"; > > > > > > > > default CacheConfig getCacheConfig(Class... additionalBindClass) { > > > > return getCacheConfig(DEFAULT_GROUP, additionalBindClass); > > > > } > > > > > > > > CacheConfig getCacheConfig(String group, Class... > > additionalBindClass); > > > > > > > > > > > > I would also spell out getCacheConfig(..) as > getCacheConfiguration(..) > > > and > > > > similarly for update. > > > > > > > > > > > > 4. "The ClusterConfigurationService will be made available from the > > > Cache, > > > > provided the Cache is managed by a Locator. The service is > unavailable > > > for > > > > other members." > > > > > > > > See above. Again, 1 of the intents for this API is to be accessible > to > > > > application, framework and tool developers, not just module > developers. > > > > Having 1 API is better than multiple. Less is more. > > > > > > > > > > > > 5. I like the API usage overall, but this is broken (you have the > > making > > > > of a NPE) ... > > > > > > > > RegionConfig regionConfig = > > > > cc.getRegion().stream().filter(x -> > > > > x.getName().equals(regionPath)).findFirst() > > > > .orElse(null); > > > > regionConfig.getIndex().add(index); > > > > > > > > return cc; > > > > > > > > > > > > And should read... > > > > > > > > cc.getRegion().stream().filter(x -> > > > > x.getName().equals(regionPath)).findFirst() > > > > *.ifPresent(regionConfig.getIndex()::add);* > > > > > > > > return cc; > > > > > > > > > > > > > > > > 6. Regarding... > > > > > > > > *> "Common operations, such as the above "find region config", could > > also > > > > be considered for initial inclusion to the public API."* > > > > > > > > Yes, I think these operations should be included. > > > > > > > > In fact the whole ClusterConfigurationService should perhaps be, or > > > better > > > > yet, return an object that functions more like a *Repository* (aka > DAO) > > > to > > > > operate on the "cluster configuration". > > > > > > > > For instance, how do I "remove" part of the cluster configuration? > > > Maybe I > > > > no longer want that Index. I am getting that I would search for the > > > Region > > > > with the Index, and with the reference to the > CacheConfig.RegionConfig, > > > > remove the Index from the array and then call > > > > clusterConfigurationService.updateCacheConfig(..) (??) > > > > > > > > > > > > 7. Regarding... > > > > > > > > > > > > *> "The use of UnaryOperator<CacheConfig> could be replaced by a > direct > > > > assignment, i.e., updateCacheConfig("cluster", > myNewCacheConfig).While > > > this > > > > might present a cleaner API, it would also allow for race conditions > > > > between concurrent modifications of the configuration."* > > > > > > > > If you are referring to modifications of the config object(s) passed > in > > > to > > > > the service after the fact then you could easily resort to > having/using > > > > immutable objects and cloning/copying. > > > > > > > > > > > > 8. Regarding... > > > > > > > > *> "Reduce duplication of effort by replacing "creation" classes > > > > (i.e., CacheCreation, RegionCreation, BindingCreation, et al)" && > ...* > > > > *> "Remove requirement of JAXB / XML annotations on configuration > > element > > > > classes."* > > > > > > > > +1 > > > > > > > > > > > > 9. Regarding Security and ... > > > > > > > > *> "Within Geode itself, these permissions would > > > > be CLUSTER:READ and CLUSTER:WRITE respectively."* > > > > > > > > Isn't it CLUSTER:MANAGE? I get the read and write aspects of this > (wrt > > > the > > > > ClusterConfigurationService interface), but then what is > CLUSTER:MANAGE > > > for > > > > then? For instance, what command in *Gfsh* requires CLUSTER:MANAGE > if > > > not > > > > create Region/Index, for example? > > > > > > > > > > > > More thoughts to follow. > > > > > > > > Regards, > > > > John > > > > > > > > > > > > > > > >> On Tue, Mar 13, 2018 at 1:06 PM, Jinmei Liao <jil...@pivotal.io> > > wrote: > > > >> > > > >> for normal usages, developers will only need to use the > > > >> updateCacheConfig(groupName, mutator) api, like: > > > >> > > > >> service.updateCacheConfig("cluster", cacheConfig->{ > > > >> cacheConfig.getRegion().add(aRegionConfig); > > > >> cacheConfig.getAsyncEventQueue().add(aQueue); > > > >> return cacheConfig; > > > >> }); > > > >> > > > >> Only when module developers who wants to add a custom cache element > > > (with > > > >> its own xsd) or region element, will need to use the other set of > > helper > > > >> apis, example > > > >> ConnectorService connector = new ConnectorService("id"); > > > >> servcie.saveCacheElement("cluster", connector); > > > >> service.deleteCacheElement("cluster", "id", > ConnectorService.class); > > > >> > > > >> or > > > >> service.saveRegionElement("cluster", "regionName", aRegionElement); > > > >> service.deleteRegionElement("cluster", "regionName", "id", > > > Element.class) > > > >> > > > >> On Tue, Mar 13, 2018 at 12:55 PM, Patrick Rhomberg < > > > prhomb...@pivotal.io> > > > >> wrote: > > > >> > > > >>> Hmm... Jinmei has it right -- that the *Element were for declaring > > > custom > > > >>> XML entries -- but they were only really used during exploration. > > With > > > >>> this minimal API, we're not actually using those interface-tags in > a > > > >>> publicly-visible way... > > > >>> > > > >>> Now I'm torn. On the one hand, it seems a bit strange to require a > > > >>> developer to use an interface that doesn't have an obvious use. On > > the > > > >>> other hand, I'm hesitant to allow an arbitrary Object to be passed > to > > > the > > > >>> XML to be saved, just as type-safety and to ensure intent. > > > >>> > > > >>> Thoughts on "Versatility" vs "Declared Intent"? > > > >>> > > > >>> Regarding naming, I could see it either way, too. "cache config" > > makes > > > >>> sense since it is modifying the "cache" portion of the > configuration > > > XML. > > > >>> "cluster configuration" makes sense since it is shared across the > > > >> cluster, > > > >>> although there are separate configurations for each member group as > > > well. > > > >>> > > > >>>> On Tue, Mar 13, 2018 at 11:14 AM, Jinmei Liao <jil...@pivotal.io> > > > wrote: > > > >>>> > > > >>>> 1. CacheElement/RegionElement are used for custom xml element that > > are > > > >> to > > > >>>> be added by other modules. They are not meant for those elements > > > >> defined > > > >>> by > > > >>>> cache.xsd already, like region, gatewayreceiver etc. Maybe we > should > > > >>> rename > > > >>>> it back CustomCacheElement/CustomRegionElement > > > >>>> > > > >>>> 1. the mutator updates the CacheConfig object which holds all the > > > >>> elements > > > >>>> inside cache including regions, indexes and the custom elements. > the > > > >> api > > > >>> is > > > >>>> updateCacheConfig > > > >>>> > > > >>>> On Tue, Mar 13, 2018 at 10:55 AM, Sai Boorlagadda < > > > >>>> sai.boorlaga...@gmail.com > > > >>>>> wrote: > > > >>>> > > > >>>>> I have few comments: > > > >>>>> > > > >>>>> 1, What is ClusterElement/RegionElement interfaces used for? > > > >>>>> > > > >>>>> 2. I see that using unary mutator one can mutate both cache and > > > >> region > > > >>>>> configurations, so the methods in ClusterConfigurationService can > > be > > > >>>>> getClusterConfig and updateClusterConfig? The naming is debatable > > as > > > >>>> region > > > >>>>> configuration is part of CacheConfig, so technically user is > > updating > > > >>>>> CacheConfig even for region changes and changing a group's > > > >>> configuration > > > >>>> is > > > >>>>> not cluster configuration. So I think updateCacheConfig is a > better > > > >>> name > > > >>>>> than updateClusterConfig? > > > >>>>> > > > >>>>> On Mon, Mar 12, 2018 at 4:11 PM, Patrick Rhomberg < > > > >>> prhomb...@pivotal.io> > > > >>>>> wrote: > > > >>>>> > > > >>>>>> Hello all. > > > >>>>>> > > > >>>>>> Please refer to the wiki page [1] for a proposal to expose > > > >>>> modification > > > >>>>>> of cluster configuration. > > > >>>>>> In short, this proposes an endpoint for developers to modify or > > > >>>> extend > > > >>>>>> cluster configuration functionalities. This would eventually > > > >> replace > > > >>>>>> existing configuration classes (e.g., CacheCreation). > > > >>>>>> The proposed intends to use JAXB to generate and translate > > > >> between > > > >>>> Java > > > >>>>>> Objects and the underlying cache configuration's XML. Examples > of > > > >>>> these > > > >>>>>> generated classes are included in the proposal's Resources > > section. > > > >>>>>> Regards. > > > >>>>>> > > > >>>>>> Imagination is Change. > > > >>>>>> ~Patrick > > > >>>>>> > > > >>>>>> [1] > > > >>>>>> https://cwiki.apache.org/confluence/display/GEODE/ > > > >>>>> Public+API+for+Cluster+ > > > >>>>>> Configuration > > > >>>>>> > > > >>>>> > > > >>>> > > > >>>> > > > >>>> > > > >>>> -- > > > >>>> Cheers > > > >>>> > > > >>>> Jinmei > > > >>>> > > > >>> > > > >> > > > >> > > > >> > > > >> -- > > > >> Cheers > > > >> > > > >> Jinmei > > > >> > > > > > > > > > > > > > > > > -- > > > > -John > > > > john.blum10101 (skype) > > > > > > > > > -- > -John > john.blum10101 (skype) > -- Cheers Jinmei
