GEODE-6184 Added internal implementation of ClusterManagementService Java API to create a region

[aditya87]

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 jiliao@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:

• Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

• Has your PR been rebased against the latest commit within the target branch (typically develop)?

• Is your initial contribution a single, squashed commit?

• Does gradlew build run cleanly?

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

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.

[kohlmu-pivotal]

Imo, there are a few general areas that need improvement:

• All API's and service methods require JavaDoc and proper documentation
• All public facing interfaces and API's need extensive JavaDoc
• single variable or abbreviated variables are not allowed. Please refrain from using 'c' and 'fr' as variable names, have little meaning
• Rather than have "gaps" in implementation with '//TODO: to be implemented', could you possibly have an exception thrown that would indicate missing implementation. That way if this method used, it would at least fail with a meaningful error
• Tests should be more granular. There are some tests that test more than 1 scenario. The smell with this approach is that it could fail on the first scenario. One spends time to fix that scenario, just to re-run the test and have the next scenario fail. With finer grained tests, it is more visible the amount of scenarios that fail and estimate the extent of regression.
• Not sure I understand what deleteFrom on the *CacheElements is supposed to do. Is it deleting the *CacheElement from the passed in CacheConfig or are you deleting the CacheConfig from the *CacheElement?

[jinmeiliao]

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

[aditya87]

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

[jinmeiliao]

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.

[aditya87]

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

[jinmeiliao]

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

[kohlmu-pivotal]

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

[aditya87]

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.

[kohlmu-pivotal]

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

[aditya87]

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.

[kohlmu-pivotal]

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

[aditya87]

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

[kohlmu-pivotal]

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

[aditya87]

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.

[jinmeiliao]

As long as all the tests are passing.

[jinmeiliao]

+1

[jdeppe-pivotal]

+100