[PROPOSAL] use default value for validate-serializable-objects in dunit
I want to propose using the default value for validate-serializable-object in dunit tests instead of forcing it on for all dunit tests. I'm sympathetic to the reason why this was done: ensure that all existing code and future code will function properly with this feature enabled. Unfortunately running all dunit tests with it turned on is not a good way to achieve this. Here are my reasons: 1) All tests should start out with the same defaults that Users have. If we don't do this, we are going to goof up sometime and release something that only works with this feature turned on or worsen the User experience of Geode in some way. 2) All tests should have sovereign control over their own configuration. We should strive towards being able to look at a test and determine its config at a glance without having to dig through the framework or other classes for hidden configuration. We continue to improve dunit in this area but I think adding to the problem is going in the wrong direction. 3) It sets a bad precedent. Do we follow this approach once or keep adding additional non-default features when we need to test them too? Next one is GEODE-4769 "Serialize region entry before putting in local cache" which will be disabled by default in the next Geode release and yet by turning it on by default for all of precheckin we were able to find lots of problems in both the product code and test code. 4) This is already starting to cause confusion for developers thinking its actually a product default or expecting it to be enabled in other (non-dunit) tests. Alternatives for test coverage: There really are no reasonable shortcuts for end-to-end test coverage of any feature. We need to write new tests or identify existing tests to subclass with the feature enabled. 1) Subclass specific tests to turn validate-serializable-object on for that test case. Examples of this include a) dunit tests that execute Region tests with OffHeap enabled, b) dunit tests that flip on HTTP over GFSH, c) dunit tests that run with SSL or additional security enabled. 2) Write new tests that cover all features with validate-serializable-object enabled. 3) Rerun all of dunit with and without the option. This doesn't sound very reasonable to me, but it's the closest to what we really want or need. Any other ideas or suggestions other than forcing all dunit tests to run with a non-default value? Thanks, Kirk
Re: [PROPOSAL] Public API for Cluster Configuration
All, I have added a section "Additional Concerns" addressing security and concurrent use of the proposed interface. Imagination is Change. ~Patrick
Re: [PROPOSAL] Public API for Cluster Configuration
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
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: FunctionService Proposal
+1 Udo. A very clear and descriptive spec; Nice work!
In short, these would be welcomed changes and would have very little impact
to *Spring Data for Apache Geode*, which nicely abstracts this API even
further using its POJO-based, Annotation configuration model for Function
Implementations as well as Executions.
I too would prefer that there be a single method to obtain an instance of
the FunctionService. However, as we know this is not currently possible
without a clear and proper separation of the client and peer/server cache.
As Dan points out, while there are 2 separate interfaces for client and
peer/server caches, namely o.a.g.cache.client.ClientCache and
o.a.g.cache.Cache, the problem remains, there is only a single
implementation of both, o.a.g.internal.cache.GemFireCacheImpl, and
therefore, it is not possible to have a 2 exactly named methods that only
vary by return type in the same class (this is different than a subclass
having a different, compatible return type, which was only allowed as of
Java 8?).
I propose the following...
interface FunctionService {
onRegion(:Region);
}
interface ClientFunctionService extends FunctionService {
onServer(:Pool);
onServers(:Pool);
onServer(:RegionService);
onServers(:RegionService);
}
interface ServerFunctionService extends FunctionService {
onMember(:String...)
onMembers(:String...)
onMember(:DistributedMember)
onMembers(:DistributedMember[])
}
Then on the GemFireCache interface, you can declare...
interface GemFire extends RegionService {
T getFunctionService();
...
}
Then it is a simple matter for users to get reference to either, for
example from a client:
ClientFunctionService clientFunctionService =
clientCache.getFunctionService();
Or, from a server (peer):
ServerFunctionService serverFunctionService = cache.getFunctionService();
A (crude) implementation of getFunctionService() in
o.a.g.internal.cache.GemFireCacheImpl, would be...
@SuppressWarnings("unchecked")
public T getFunctionService() {
return (T) (isClient() ? new DefaultClientFunctionService() : new
DefaultServerFunctionService());
}
Of course, the implementation could be much more sophisticate than this.
The point being, I don't see the separate of the client and server logic
for cache functionality being split anytime soon and this would a good
interim solution that would not require any changes in the future.
I might even make the instantiation of the Client or Server based
FunctionServices accessible from a protected method for extension purpose
(i.e. subclassing), thereby making Apache Geode more extensible, so...
@SuppressWarnings("unchecked")
public T getFunctionService() {
return (T) (isClient() ? newClientFunctionService() :
newServerFunctionService());
}
protected ClientFunctionService newClientFunctionService() {
return new DefaultClientFunctionService();
}
protected ServerFunctionService newServerFunctionService() {
return new DefaultServerFunctionService();
}
It wouldn't be too difficult to imagine this being accomplished using
Java's SPI as well. Having a customizable/extensible API for Function
execution would be useful where users wanted to plugin different execution
handlers, for streaming or reactive purposes, for instance, and so on. If
the user is not satisfied with the defaults, regardless of concern, they
can plugin their own removing us from the equation. It does not preclude
us from providing OOTB implementations for these concerns, but they would
just be another provider like any other.
Food for thought.
Cheers,
John
On Thu, Mar 8, 2018 at 5:02 PM, Galen O'Sullivan
wrote:
> +1 for making the function service not static, and splitting servers from
> clients.
>
> Also +1 for Dan's suggestion.
>
> On Wed, Mar 7, 2018 at 2:51 PM, Patrick Rhomberg
> wrote:
>
> > I did not know that! And then, yes, onRegion is much better.
> >
> > On Wed, Mar 7, 2018 at 2:43 PM, Dan Smith wrote:
> >
> > > > If we're not opposed to descriptive verbosity, I might prefer
> > > "onServersHostingRegion" more than "onRegion".
> > >
> > > onRegion does not really mean "on the servers hosting region XXX". It
> > only
> > > executes on a subset of the servers, potentially with retries, until it
> > has
> > > covered that entire dataset once. So I think onRegion is more
> > appropriate.
> > >
> > > -Dan
> > >
> > > On Wed, Mar 7, 2018 at 2:38 PM, Patrick Rhomberg >
> > > wrote:
> > >
> > > > +1 for iteration towards better single responsibility design and more
> > > > easily-digestible classes.
> > > >
> > > > Regarding method names, I think that there would be some good utility
> > in
> > > > having "onGroup" methods, as well.
> > > > If we're not opposed to descriptive verbosity, I might prefer
> > > > "onServersHostingRegion" more than "onRegion".
> > > >
> > > > On Wed, Mar 7, 2018 at 1:45 PM, Dan Smith wrote:
> > > >
> > > > > Hi Udo,
> > > > >
> > > > > +1 for making the function service not static and spitting it into
Re: [PROPOSAL] Public API for Cluster Configuration
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
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
Need permissions to Geode JIRA
Hello, I need to be able to update tickets in Geode JIRA, can someone please set me up with appropriate permissions? Thanks, Blake
Re: Need permissions to Geode JIRA
Hi Blake, What's I added you, you should have permission now. -Dan On Tue, Mar 13, 2018 at 1:12 PM, Blake Bender wrote: > Hello, > > I need to be able to update tickets in Geode JIRA, can someone please set > me up with appropriate permissions? > > Thanks, > > Blake >
Geode unit tests completed in 'develop/IntegrationTest' with non-zero exit code
Pipeline results can be found at: Concourse: https://concourse.apachegeode-ci.info/teams/main/pipelines/develop/jobs/IntegrationTest/builds/274
Re: [PROPOSAL] Public API for Cluster Configuration
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 a
[Spring CI] Spring Data GemFire > Nightly-ApacheGeode > #855 was SUCCESSFUL (with 2379 tests)
--- Spring Data GemFire > Nightly-ApacheGeode > #855 was successful. --- Scheduled 2381 tests in total. https://build.spring.io/browse/SGF-NAG-855/ -- This message is automatically generated by Atlassian Bamboo
Geode unit tests completed in 'develop/IntegrationTest' with non-zero exit code
Pipeline results can be found at: Concourse: https://concourse.apachegeode-ci.info/teams/main/pipelines/develop/jobs/IntegrationTest/builds/275
Geode unit tests completed in 'develop/FlakyTest' with non-zero exit code
Pipeline results can be found at: Concourse: https://concourse.apachegeode-ci.info/teams/main/pipelines/develop/jobs/FlakyTest/builds/308
Broken Integration Tests due to putIfAbsent
Hi all, I committed some code recently that looks like it broke the CI. However, based on the failure, I think it will still fail if I revert the commit. I'm currently testing a small hotfix and will apply it if it passes tests. Sorry for the inconvenience. Thanks, Galen GEODE-4836 https://github.com/apache/geode/pull/1612
Re: [PROPOSAL] Public API for Cluster Configuration
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 be, or better
> ye
Geode unit tests completed in 'develop/IntegrationTest' with non-zero exit code
Pipeline results can be found at: Concourse: https://concourse.apachegeode-ci.info/teams/main/pipelines/develop/jobs/IntegrationTest/builds/277
