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)
