GEODE-5032: Update cluster configuration objects for more intuitive consumption #1848

PurelyApplied · 2018-04-23T23:31:13Z

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.

PurelyApplied · 2018-04-23T23:36:29Z

I meant to put this in the commit message, but it must've gotten lost when I rebased:

Changes include:

Those getters that return a List have been renamed to indicate a plural.
Much of the "massaging" has been documented in the bindings.xjb file. Comments document what other adjustments have been made, at time of writing.
A more-restrictive xsd has been included in configurationService.xsd that removes deprecated items that should not be supported by this new feature.
Configuration classes have been generated as Serializable, as this will be required when the creation commands et al pass configuration objects to instantiate the services they support.

I have included the xjc tasks I used to generate these objects in their respective gradle files, as well as the bindings file each task would produce. These could have future value if this process is repeated, but I could also see the value in keeping our build files from becoming cluttered.

This PR does not update or adjust any of the configuration objects in geode-connectors or geode-lucene. Those can be addressed in another ticket.

jinmeiliao · 2018-04-23T23:54:35Z

...e/src/main/resources/META-INF/schemas/geode.apache.org/schema/cache/configurationService.xsd

+  ~ See the License for the specific language governing permissions and
+  ~ limitations under the License.
+  -->
+<xsd:schema


So we need to check both the cluster-config.xsd, the *.xjb and the generated POJOs into the source tree? It would be ideal if we can do either:

keeping the xsd, and xjb and have the POJOs generated by the build process. Which gives us a nice one place to see all the changes we've made to the schema.

keeping the POJOs only. It would be easier for the developer to add/modify the methods instead of trying to find the way to transform it using a binding file (.xjb).

A third option is:
3. keeping the massaged xsd and the POJOs. the XSD is just a reference to keep all the changes in one place and for future reference. it's not used in the build process.

Not sure which way we would like to go.

I'd love to get to the point where we could just generate the POJOs as part of the build process, even if it has to be based on an "internal only" XSD.

During iteration, I found there is some trouble with getting CacheElement to selectively apply to some but not all classes. There are some 3rd party libraries that could help on that front and with gradle integration. I was hesitant to introduce a new dependency for this, though, and don't remember which licenses are compatible with the Apache 2.0.

I don't like the idea that the config classes are all treated like first-class citizens. We're already pushing the line where some config classes are special cases. Every special case is a pitfall for a future developer.

If the massaged XSD isn't actually consumed (and won't be in the future), then I should just remove it from this PR. Future developers won't remember to update the XSD or bindings file with every update to the POJOs, and once it's out of date, its existence will just add confusion.

pdxrunner · 2018-04-24T18:28:59Z

...test/java/org/apache/geode/distributed/internal/InternalClusterConfigurationServiceTest.java

+      configProperty.setType("test");
+      configProperty.setValue("test");
+      jndiBinding.getConfigProperties().add(configProperty);
+      cacheConfig.getJndiBindings().getJndiBindings().add(jndiBinding);


should be `cacheConfig.getJndiBindings().add(jndiBinding)

Whoops! Looks like my build alias skips compiling on the test side. That's something I need to fix!

pdxrunner · 2018-04-24T20:29:27Z

.../test/java/org/apache/geode/management/internal/cli/commands/ListJndiBindingCommandTest.java

@@ -65,7 +65,7 @@ public void noServiceNoMember() {
  @Test
  public void hasServiceNoBindingNoMember() {
    doReturn(ccService).when(command).getConfigurationService();
-    when(cacheConfig.getJndiBindings()).thenReturn(Collections.emptyList());
+    when(cacheConfig.getJndiBindings().getJndiBindings()).thenReturn(Collections.emptyList());


... another one of these getJndiBindings().getJndiBindings()

…his branch's history for future reference.

jinmeiliao · 2018-04-27T20:10:13Z

geode-core/src/main/java/org/apache/geode/cache/configuration/JndiBindingsType.java

      @XmlElement(name = "config-property-name", namespace = "http://geode.apache.org/schema/cache",
          required = true)
-      protected String configPropertyName;
+      protected String name;


I think we changed the name to be "configPropertyName" for some json serialization which requires this name. Do all tests pass?

jinmeiliao · 2018-04-27T20:11:50Z

...e-core/src/main/java/org/apache/geode/cache/configuration/SerializationRegistrationType.java

@@ -174,8 +174,9 @@
   */
  @XmlAccessorType(XmlAccessType.FIELD)
  @XmlType(name = "", propOrder = {"className"})
-  public static class Instantiator {
+  public static class Instantiator implements Serializable {


do we need to add the class to geode-core-sanctioned-serializable.txt?

jinmeiliao

It would be nice that for every change we made to the POJO, we would have a junit test to make sure the resulting xml still pass the xsd validation.

This reverts commit 1b78b62.

This reverts commit 4a97a2e.

PurelyApplied · 2018-05-09T23:23:43Z

Oops, I messed up my branches and accidentally pushed the restoration of the build files.

Considering the drift between this and the extant iteration, I think I'll just close this PR for now and open a new one if it comes to that.

PurelyApplied added 2 commits April 23, 2018 16:27

Build framework.

32df7cb

Updated configuration classes

71de6d6

PurelyApplied requested review from jdeppe-pivotal, sboorlagadda, jinmeiliao and pdxrunner April 23, 2018 23:36

jinmeiliao reviewed Apr 23, 2018

View reviewed changes

Restore port to String input.

bd0ae85

pdxrunner requested changes Apr 24, 2018

View reviewed changes

Correct scoping of JNDI binding config object in tests.

703a790

pdxrunner reviewed Apr 24, 2018

View reviewed changes

PurelyApplied added 4 commits April 26, 2018 16:02

I will be removing these immediately, but wanted them to persist in t…

9b6c5e3

…his branch's history for future reference.

Remove some build-related files.

1b78b62

Revert */build.gradle changes.

4a97a2e

Remove custom xsd and xjb files.

1c2530b

jinmeiliao reviewed Apr 27, 2018

View reviewed changes

jinmeiliao requested changes Apr 27, 2018

View reviewed changes

PurelyApplied added 2 commits May 9, 2018 15:53

Revert "Remove some build-related files."

cc2dab0

This reverts commit 1b78b62.

Revert "Revert */build.gradle changes."

9f508a5

This reverts commit 4a97a2e.

PurelyApplied closed this May 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GEODE-5032: Update cluster configuration objects for more intuitive consumption #1848

GEODE-5032: Update cluster configuration objects for more intuitive consumption #1848

PurelyApplied commented Apr 23, 2018 •

edited

PurelyApplied commented Apr 23, 2018

jinmeiliao Apr 23, 2018 •

edited

PurelyApplied Apr 24, 2018

pdxrunner Apr 24, 2018

PurelyApplied Apr 24, 2018

pdxrunner Apr 24, 2018

jinmeiliao Apr 27, 2018

jinmeiliao Apr 27, 2018

jinmeiliao left a comment

PurelyApplied commented May 9, 2018

GEODE-5032: Update cluster configuration objects for more intuitive consumption #1848

GEODE-5032: Update cluster configuration objects for more intuitive consumption #1848

Conversation

PurelyApplied commented Apr 23, 2018 • edited

For all changes:

Note:

PurelyApplied commented Apr 23, 2018

jinmeiliao Apr 23, 2018 • edited

Choose a reason for hiding this comment

PurelyApplied Apr 24, 2018

Choose a reason for hiding this comment

pdxrunner Apr 24, 2018

Choose a reason for hiding this comment

PurelyApplied Apr 24, 2018

Choose a reason for hiding this comment

pdxrunner Apr 24, 2018

Choose a reason for hiding this comment

jinmeiliao Apr 27, 2018

Choose a reason for hiding this comment

jinmeiliao Apr 27, 2018

Choose a reason for hiding this comment

jinmeiliao left a comment

Choose a reason for hiding this comment

PurelyApplied commented May 9, 2018

PurelyApplied commented Apr 23, 2018 •

edited

jinmeiliao Apr 23, 2018 •

edited