Re: [PROPOSAL] Public API for Cluster Configuration

[Jinmei Liao]

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

Re: [PROPOSAL] Public API for Cluster Configuration

[John Blum]

*> @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  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 
> 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  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 

Re: [PROPOSAL] Public API for Cluster Configuration

[Dan Smith]

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

Re: [PROPOSAL] Public API for Cluster Configuration

[Udo Kohlmeyer]

+1 to what John said and many are thinking.

@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


From a users perspective, I don't care if the configuration is 
clustered, replicated, centralized or stored on sticky-notes on 
someone's screen. This detail could be implemented using an SPI. The 
point for the API  should be that it is clear, easy to use and simple to 
extend. (I'm thinking that with modularity, each module can expose its 
API to configured itself)


I would like to suggest that we treat GFSH as "just another 
text-completing" shell. I think we should focus on making a 
configuration service that is easy to use and clearly defined. The HOW 
we expose this API with tools like XML, YML, REST, JAVA API, GFSH, *your 
tooling of choice* should be a secondary thought.


In addition to this, I think we should shy away from the notion of 
internal and public configuration API's. All API's should be seen as 
public and the ability to execute should be restricted by authorization.


On 3/14/18 09:17, Jacob Barrett 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  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 

Re: [PROPOSAL] Public API for Cluster Configuration

[Jacob Barrett]

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

Re: [PROPOSAL] Public API for Cluster Configuration

[Jinmei Liao]

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

Re: [PROPOSAL] Public API for Cluster Configuration

[John Blum]

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

Re: [PROPOSAL] Public API for Cluster Configuration

[Jinmei Liao]

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

Re: [PROPOSAL] Public API for Cluster Configuration

[Patrick Rhomberg]

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

Re: [PROPOSAL] Public API for Cluster Configuration

[Jinmei Liao]

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

Re: [PROPOSAL] Public API for Cluster Configuration

[Sai Boorlagadda]

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

Re: [PROPOSAL] Public API for Cluster Configuration

[Patrick Rhomberg]

All,

I have added a section "Additional Concerns" addressing security and
concurrent use of the proposed interface.

Imagination is Change.
~Patrick