John, great points. I updated the wiki page with some bullet points that might address your concerns.
Goals - The API is intended to ONLY update the cluster or server-group's configuration saved by the locator(s). For example, when developer uses the api to add a region definition to the cache configuration, it should NOT be expected that the api will also create the regions for you on the existing servers. - The API should not be tied with XML. We should be able to change how we save cluster configuration internally without changing the api. - Expose a clean Java API for retrieval and modification of the cluster configuration. Anti-Goals - This API is not to be confused with an API that app-developers can use from client to, say, create a region and update the cluster configuration at the same time. That API would entail a remote invocation of this lower-level api, but it's not the same. On Tue, 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) > -- Cheers Jinmei