geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jinmei Liao <jil...@pivotal.io>
Subject Re: [PROPOSAL] Public API for Cluster Configuration
Date Thu, 15 Mar 2018 20:48:16 GMT
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 <jblum@pivotal.io> 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 <dsmith@pivotal.io> 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 <jbarrett@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 <jblum@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 <jiliao@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 <
> > > prhomberg@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 <jiliao@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.boorlagadda@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 <
> > > >>> prhomberg@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)
> > >
> >
>
>
>
> --
> -John
> john.blum10101 (skype)
>



-- 
Cheers

Jinmei

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message