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