You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@geode.apache.org by "aditya87 (GitHub)" <gi...@apache.org> on 2019/01/09 00:03:03 UTC

[GitHub] [geode] aditya87 opened pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

This internal API will be eventually used by the proposed cluster management REST API (https://cwiki.apache.org/confluence/display/GEODE/Cluster+Management+Service) in order to create a region.

Signed-off-by: Jinmei Liao <ji...@pivotal.io>

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

- [X] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?

- [X] Is your initial contribution a single, squashed commit?

- [X] Does `gradlew build` run cleanly?

- [X] Have you written or updated unit tests to verify your changes?

- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?

### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.


[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

not sure that I understand the logic of `isSuccessful` if the `result== NOT_APPLICABLE`. Does this mean that it has not yet been completed? Maybe some JavaDoc explaining the logic would help.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

will do 

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

Have made the test more logically meaningful.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Bill commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "Bill (GitHub)" <gi...@apache.org>.

+1 on the rename to `ClusterConfigElement`. That's more accurate.

As for my comments about `Operation`: if various subtypes of `CacheElement` do not need their own redundant operation (verbs) then feel free to disregard my suggestion.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

Sure, I think once we start adding more mutators/realizers we can refactor it this way. Don't think it's necessary now.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] ukohlmeyer commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "ukohlmeyer (GitHub)" <gi...@apache.org>.

interface and generics <T,C> please

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

Yes, and maybe this could an enum type to make it more descriptive

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

Yes, you could pass in the class, but I'd rather keep the interface simpler.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] ukohlmeyer commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "ukohlmeyer (GitHub)" <gi...@apache.org>.

Also could we use generics `ConfigurationPersister<T,Cache>`

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pdxrunner commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "pdxrunner (GitHub)" <gi...@apache.org>.

Fix the javadoc, should be `
> Created with an object of type CacheElement, which represents the configuration change.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

With "dumb" I meant, it is still ok to have the Exception, but with a usable message... maybe this message could even by localized. 

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

`private` vs `package private`?

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jdeppe-pivotal commented on issue #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "jdeppe-pivotal (GitHub)" <gi...@apache.org>.

+100

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

While it would solve the problem of using the RegionConfig instance twice, I don't think it's "simpler", since you're not creating the RegionConfig instance, you're just using the one that's passed in.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

Used enums to represent this better

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

Sure, this can be private

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

since the mapping is class to the mutator, should the signature be:
`public ConfigurationMutator get(Class configClass)` instead?

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

Awesome.. thank you

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

Sure, will fix.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Bill commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "Bill (GitHub)" <gi...@apache.org>.

I see `ClusterCacheElement` being sent to CRUD methods on `ClusterManagementService` interface. But I don't understand why the data structure itself needs to carry its own operation?

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

`private` rather `package private`?

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

if value is `parent/child`, then this would fail to catch this. Could change it to:

```
if (value == null) {
      throw new IllegalArgumentException("Region name cannot be null");
    }

    if (value.startsWith("/")) {
      value = value.substring(1);
    }

    if (value.split("/").length > 1) {
      throw new IllegalArgumentException("Region name is invalid -- cannot have multiple slashes");
    }
    
    this.name = value;
```

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

`cc`? As in?

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

It means that cluster config is disabled, so the the result is successful on the members (and it didn't have to persist in the cluster config service). 

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

I thought there was a decision made to try and avoid accessing/looking up of Locators/Caches using the static singleton approach. Is there not another way the locator could be accessed?

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

could these fields possibly be made private or protected, rather than package protected?

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Bill commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "Bill (GitHub)" <gi...@apache.org>.

What is a `ClusterCacheElement`?

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

I was thinking the exact same thing this morning. Can we please change the `NoMembersException` to not allow the default constructor, but at the very least override the `message` variant.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

it probably would be better to have this service hold a reference to this factory instance as a member variable, then it will only need initialize all the mutator once in every VM. I would like to see this service holds a list of all the mutators, and that generic function holds a list of all the realizors.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

Because the `ClusterCacheElement`'s implementation will be used on both locator/server and it holds all the configuration attributes of a CacheElement. 

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] ukohlmeyer commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "ukohlmeyer (GitHub)" <gi...@apache.org>.

why would this not be an interface? I don't see the need to store the `CacheElement` OR `CacheConfig` on the Persister

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

So we are not concerned about concurrency? *COULD* there be any potential that multiple threads could change the same  CacheConfig and instance and modify-in-place? How would the system react if two threads were to amend the same CacheConfig object and ask it to persist it... If concurrency is not a concern, this question is mute

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pdxrunner commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "pdxrunner (GitHub)" <gi...@apache.org>.

Let's annotate all of these new interfaces as `@Experimental`

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

I might be barking up the wrong tree, but it smacks of bad OO-design... Example... If you design a `Kennel`, and you say that there are `KennelInhabitants`, which represent the animals that live in the `Kennel`... You cannot make a `KennelInhabitant` of type `Canine` AND `Feline` just because it can be any one of the two... This is begging for failure if you have a `Canine` but you cast it to a `Feline` and try to invoke the operations on that...
Just bad OO-design... `implements` and `extends` should be replaced with "IS A..." and not with "COULD BE ONE OF THESE"

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

Depending on which type of ClusterCacheElement it is (i.e. a new region, a new async event queue, a new index, etc), it will go in a different place in the CacheConfig. Therefore, the add is not the same -- and each element must be able to define how to add itself.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

CacheConfig, I believe. Will change that.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

I'm not sure that I agree with the design. How can a `ClusterCacheElement` be a `CacheElement` AND a `ServerCacheElement` AND a `LocatorCacheElement`?

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

I think this comment is a holdover from before. Having said that, we'd be adding some basic validations in the setters of the config object (like we did for subregion paths) -- and eventually when we things we need to validate post generating the config, we would add a separate function to the ClusterCacheElement interface which each config object can override. Bear in mind some validations will be need to run on the locator and some on the server -- but currently this is out of scope for this JIRA.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] ukohlmeyer commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "ukohlmeyer (GitHub)" <gi...@apache.org>.

maybe store the `ClusterRegionConfigRealizer` instance

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Petahhh commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "Petahhh (GitHub)" <gi...@apache.org>.

Can we be consistent with the order of params for status related methods? For example the constructor is boolean first and string message second.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

Fair point :) Will simplify this

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

could we improve this? Since you are in this code... can we not address or try to address this?

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

The mutators are used technically outside the scope of the methods that actually operate on the common persisted config on the locator. We "atomically" get a CacheConfig object representation of the persisted config (essentially a copy), mutate it, and then it gets written back -- if you look at the way this is used in LocatorClusterManagementService it would make this clear, since the underlying method that does this acquires a lock. So in this case it's thread safe whether you modify in-place or not. 

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

Sure

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

For `GfshCommand`, we haven't gone through the effort of removing these static lookups yet. In fact, `GfshCommand` came into existence because we want to hide these static lookups in one place. In here, not only `locator`, `gfsh`, and `managementService` are also accessed through static methods.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

No, you can have a region without persistence enabled, apparently.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

configuration, I believe. Will change that.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Petahhh commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "Petahhh (GitHub)" <gi...@apache.org>.

Good idea @kohlmu-pivotal =D

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Petahhh commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "Petahhh (GitHub)" <gi...@apache.org>.

Regarding
> clusterConfigStatus == null 

It's kind of weird to imply that uninitialized status are successful

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao closed pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

[ pull request closed by jinmeiliao ]

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

Done!

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

Same as above

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

In the case that `getRefID` is not one of the selected types, what would the `getDataPolicy()` return? Any chance that this could ever be `null` or throw an NPE in this fairly long command chain.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

I feel it might be useful to have the ability to throw different HTTP error codes based on the exception that happened. In this case, maybe it's just a 500 anyway, but in general if we make the REST API completely dumb, then it will be difficult to throw the right error code (outside of string-parsing the message).

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

It would be a good practice not to modify in-place, but don't think it's a bad practice to modify in-place either. If you think about how this could be used, it might even be cleaner to modify in place, so you don't have to declare more variables.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

Sure. this is dragging out long as it is now.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

all of these initializations can be done on the field definition. Don't see why this is special.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

this is because of this line in cache-1.0.xsd:
`attribute name="version" use="required" type="{http://geode.apache.org/schema/cache}versionType" fixed="1.0"`
We could make that version value a static field.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

I'm going to answer all your comments in one go.

1) Interface vs abstract class: We had an interface and generics before. If we were to use that, it would become something like this:
```
interface ConfigurationPersister {
void add(CacheElement config, CacheConfig existing)
...
}
```
and then in the `LocatorClusterManagementService`:
```
public void createCacheElement(CacheElement config) {
  CacheConfig existingConfig = persistenceService.getConfig("cluster", cache);
  ConfigurationPersister persister = (new ConfigurationPersisterFactory()).generate(config)
  persister.add(config)
}
```

See how we are using the config twice? Once to create the persister (since we need to know what instance of the persister we need), and then passing it in to persist it? That is clunky and weird in my opinion.

We could, of course, do this:
```
interface ConfigurationPersister {
void add()
...
}
```
and then in the `LocatorClusterManagementService`:
```
public void createCacheElement(CacheElement config) {
  CacheConfig existingConfig = persistenceService.getConfig("cluster", cache);
  ConfigurationPersister persister = (new ConfigurationPersisterFactory()).generate(config)
  persister.add()
}
```
This implicitly tells you that any ConfigurationPersister instance must store the config it is initialized with -- and if you are doing that, since you can't have variables on an interface, might as well use an abstract class. 

2). Generics -- yes, we could add a generic to the `ConfigurationPersister` class, and that would eliminate the need for some explicit downcasting we are doing.

3). Not using newInstance()/storing the realizer instance -- could not figure out a good way of using a Map<Class, Class> and not using newInstance(). Storing the realizer instance would not work nicely the way the code is written right now, because the persister/realizer are instantiated with a given existing config object/cache element -- if we were to store instances in the map, we'd be potentially using stale state in our code. Of course, if the method signatures took in the config itself, as described in point 1), this wouldn't be an issue since you wouldn't have to store the config, but as described in 1), that is awkward.

Hope I've answered your questions. For further discussions, would love to connect again on Slack!

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

same as above

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

See above

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pdxrunner commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "pdxrunner (GitHub)" <gi...@apache.org>.

The message `cannot have multiple slashes` could be misleading to a user. Using the logic @jinmeiliao shows, a region was specified as `"parent/child"` is invalid even though only one `'/'` was specified.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

Fair point!

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

With the serviceInterface of `subject.generate(Class)` would be simpler than having to create `RegionConfig` instance

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

Could we split this test?

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

hard coding a version number? Does not seem correct....

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

Could we have a `PartitionRegionConfigRealizer` and `ReplicateRegionConfigRealizer` and `NormalRegionConfigRealizer` and `ReplicateProxyRegionConfigRealizer`.... etc.. Maybe pre-optimization?

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Petahhh commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "Petahhh (GitHub)" <gi...@apache.org>.

If persistentService is null, should we also be setting the cluster config status so the user knows cluster config was not updated?

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

No, because the status only indicates success/failure -- in the case of no persistence, it's a third case which the nullability represents -- again, we can use enums to represent that I guess. Will look into it.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

I disagree... The REST API should be "dumb" and only pass on the error that was given to it.. not make one up. :)

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] ukohlmeyer commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "ukohlmeyer (GitHub)" <gi...@apache.org>.

why would you not just instantiate the Persister? Better (and cheaper) than having to do a `Class.newInstance()` every time you want to use it

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on issue #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

+1

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

Sorry, that's what I meant -- cluster config enabled versus not.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

ok.. could we then at least have it as a final static field, rather than an arbitrary string in the middle of the code somewhere..

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

Sure, I think once we start adding more mutators/realizers we can refactor it this way.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

it probably would be better to have this service hold a reference to this factory instance or the map reference directly as a member variable, then it will only need initialize all the mutator once in every VM. Something like:
```
public class LocatorClusterManagementService extends ....{
....
private Map<Class, Mutators> mutators = new HashMap(); // this map can be a instance variable since the we only have one service instance per cache.
public LocatorClusterManagementService(...){
  // initialize the map
}
}

public Class UpdateCacheFunction extends ... {
...
private static Map<Class, Realizers> realizers = new HashMap(); // this needs to be a static variable, since the we would generate one function instance per function call.
// some static initializers to initialize the map...
}
```
We should also have some test visible methods to add to these maps in unit tests.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Bill commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "Bill (GitHub)" <gi...@apache.org>.

Hmm I wonder if the operation could be completely separated from the subtype (of `CacheElement`). So e.g. `ADD` would be polymorphic across various kinds of `CacheElement`

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

You still need to have the persistenceService available here in order to grab the latest cluster config object. Figure if you need it, you might as well use its nullability to check if persistence is enabled.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

cluster configuration. But we will rename that.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Petahhh commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "Petahhh (GitHub)" <gi...@apache.org>.

cloud cache ;)

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

Could this then maybe just have a pass-through persistence mechanism? Makes more sense than having to null check

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

Can use enums

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

Sure, yeah we can add an error message, that can be bubbled down by the REST service.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Petahhh commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "Petahhh (GitHub)" <gi...@apache.org>.

Where is the config validated? How do we guarantee that the config is validated before it's passed to this method?

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

Why would any mutation operation not return the "new" `CacheConfig`? I think it would be good practice NOT to modify-in-place

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

We are currently not handling other ref ID cases in this API -- eventually will. Won't get an NPE but will get an IllegalArgumentError -- although you're right, maybe it's better to handle that upstream.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

@Bill Not sure I fully understand your comment? We have ClusterCacheElement (called ClusterConfigElement now), which defines a method called addTo(CacheConfig existing) (that is now wrapped by a ConfigurationPersister instance). It is up to the individual subclass of ClusterConfigElement (like ClusterRegionConfig) to return a ConfigurationPersister that implements this method. The implementation contains the logic to persist the specific instance of ClusterConfigElement into the cluster configuration on the locator. Essentially giving us the polymorphism you are referring to. 

So I'm not sure I understand how what you're saying is different to what's happening? Would be great to sync up tomorrow to discuss this.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

`c` being what?

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

Unfortunately because `LocatorClusterManagementService` from a different package needs to access this, it can't be package private.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

surely this does not need to be public or?

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

when clusterConfigstatus is null, this denotes the locator is started without cluster configuration service (`enabled-cluster-configuration` is false). When clusterConfigStatus is false, which means the ccService is there, but there is some problem saving the configuration. Will better document that.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

but right now IT IS package private... maybe protected then?

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

Pre-optimization, especially since we don't have disparate ReplicateRegionConfig and PartitionedRegionConfig classes.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Petahhh commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "Petahhh (GitHub)" <gi...@apache.org>.

If we're throwing a `NoMembersException` found, is this enough information for the user to understand why their create failed?

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Petahhh commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "Petahhh (GitHub)" <gi...@apache.org>.

When the `persistenceService` is null, is it implied that the region definitely does not exist?

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

I thought this persistence service was relating to the cluster configuration mechanism rather than "persistence" in the sense of a region writing to disk. I thought ClusterConfig, when enabled, would persist locally by default.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

comcast?

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

Unfortunately because the LocatorClusterManagementService in a different package needs to access this, it can't be package private.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

@Bill Not sure I fully understand your comment? We have `ClusterCacheElement` (called `ClusterConfigElement` now), which defines a method called `addTo(CacheConfig existing` (that is now wrapped by a `ConfigurationPersister` instance). It is up to the individual subclass of `ClusterConfigElement` (like `ClusterRegionConfig`) to return a `ConfigurationPersister` that implements this method. The implementation contains the logic to persist the specific instance of `ClusterConfigElement` into the cluster configuration on the locator. Essentially giving us the polymorphism you are referring to. 

So I'm not sure I understand how what you're saying is different to what's happening? Would be great to sync up tomorrow to discuss this.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Bill commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "Bill (GitHub)" <gi...@apache.org>.

I see `ClusterCacheElement` being sent to CRUD methods on `ClusterManagementService` interface. But I don't understand why the data structure itself needs to define its own operations? Is adding one of these a fundamentally different operation than adding something else? Isn't adding always adding?

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] ukohlmeyer commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "ukohlmeyer (GitHub)" <gi...@apache.org>.

I imagine passing in the `CacheElement` and `Cache` as parameters would be "easier"

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

I'm gonna answer all your comments in one go.

1) Interface vs abstract class: We had an interface and generics before. If we were to use that, it would become something like this:
```
interface ConfigurationPersister {
void add(CacheElement config, CacheConfig existing)
...
}
```
and then in the `LocatorClusterManagementService`:
```
public void createCacheElement(CacheElement config) {
  CacheConfig existingConfig = persistenceService.getConfig("cluster", cache);
  ConfigurationPersister persister = (new ConfigurationPersisterFactory()).generate(config)
  persister.add(config)
}
```

See how we are using the config twice? Once to create the persister (since we need to know what instance of the persister we need), and then passing it in to persist it? That is clunky and weird in my opinion.

We could, of course, do this:
```
interface ConfigurationPersister {
void add()
...
}
```
and then in the `LocatorClusterManagementService`:
```
public void createCacheElement(CacheElement config) {
  CacheConfig existingConfig = persistenceService.getConfig("cluster", cache);
  ConfigurationPersister persister = (new ConfigurationPersisterFactory()).generate(config)
  persister.add()
}
```
This implicitly tells you that any ConfigurationPersister instance must store the config it is initialized with -- and if you are doing that, since you can't have variables on an interface, might as well use an abstract class. 

2). Generics -- yes, we could add a generic to the `ConfigurationPersister` class, and that would eliminate the need for some explicit downcasting we are doing.

3). Not using newInstance()/storing the realizer instance -- could not figure out a good way of using a Map<Class, Class> and not using newInstance(). Storing the realizer instance would not work nicely the way the code is written right now, because the persister/realizer are instantiated with a given existing config object/cache element -- if we were to store instances in the map, we'd be potentially using stale state in our code. Of course, if the method signatures took in the config itself, as described in point 1), this wouldn't be an issue since you wouldn't have to store the config, but as described in 1), that is awkward.

Hope I've answered your questions. For further discussions, would love to connect again on Slack!

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "aditya87 (GitHub)" <gi...@apache.org>.

This will eventually be handled by the REST API -- when it catches a NoMembersException, it knows exactly what error message to give. No need to introduce a special message.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on pull request #3059: GEODE-6185 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "kohlmu-pivotal (GitHub)" <gi...@apache.org>.

Why could clusterConfigStatus not be initialized with `ClusterConfigStatus.success=false`. Then one extra null check is removed.

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] ukohlmeyer commented on pull request #3059: GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

Posted by "ukohlmeyer (GitHub)" <gi...@apache.org>.

with generics and the `CacheElement` and `Cache` passed in, you could have `add(T config, C cache)`

[ Full content available at: https://github.com/apache/geode/pull/3059 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org