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
