GEODE-6117: Makes modules out of geode-core and geode-cq #2915

jake-at-work · 2018-11-29T04:38:07Z

Moves all o.a.g.internal.cq(ish) classes to o.a.g.cq.internal.
Fixes dependencies
Temporarily removes runtime dependency on Shiro to fix client modules.

Co-authored-by: Jacob Barrett jbarrett@pivotal.io
Co-authored-by: Owen Nichols onichols@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.

- Moves all o.a.g.internal.cq(ish) classes to o.a.g.cq.internal. - Fixes dependencies - Temporarily removes runtime dependency on Shiro to fix client modules. Co-authored-by: Jacob Barrett <jbarrett@pivotal.io> Co-authored-by: Owen Nichols <onichols@pivotal.io>

Authored-by: Jacob Barrett <jbarrett@pivotal.io>

Co-authored-by: Jacob Barrett <jbarrett@pivotal.io> Co-authored-by: Owen Nichols <onichols@pivotal.io>

Authored-by: Jacob Barrett <jbarrett@pivotal.io>

mcmellawatt · 2018-12-05T16:28:18Z

geode-core/src/main/java/org/apache/geode/cache/client/internal/QueryOp.java

@@ -66,7 +66,7 @@ private QueryOp() {
  /**
   * Note: this class is extended by CreateCQWithIROpImpl.
   */
-  protected static class QueryOpImpl extends AbstractOp {
+  public static class QueryOpImpl extends AbstractOp {


Why does this need to be public now? I suppose it is part of an internal package, so does that mean we don't need to worry about this being a public API change? Mainly I'm just curious why this became public, I'm not seeing the corresponding usage that requires it to be public in this PR.

There is a class in the geode-cq that inherits from this inner class. Tt was able to before because it was in the same package but to support modules we can't split packages across jars anymore. In order for the geode-cq class to access this class it must now be public. Since it is "internal" it should not be considered public API. In fact it isn't exported in a way that any module other than geode-cq can access it in the module-info.java.

mcmellawatt · 2018-12-05T16:38:40Z

geode-core/src/main/java/org/apache/geode/internal/InternalDataSerializer.java

@@ -287,6 +287,9 @@ public static String processIncomingClassName(String name) {
    if (name.startsWith(oldPackage)) {
      return newPackage + name.substring(oldPackage.length());
    }
+    if (name.equals("org.apache.geode.cache.query.internal.cq.ServerCQImpl")) {


This string and the one below it are used twice in this class. Maybe extract them into static final fields?

Sure could. Was staying consistent with the other swizzle happening here.

mcmellawatt · 2018-12-05T16:42:13Z

geode-dunit/src/main/java/org/apache/geode/test/dunit/internal/ProcessManager.java

+  }
+
+  private String removeModuleFromGradlePath(String classpath, String module) {
+    // geode-cq/build/classes/java/main


Not sure this comment is very helpful

It isn't, got left on accident. Thanks!

jake-at-work · 2018-12-05T17:49:55Z

@mcmellawatt can we get a re-review and blessing?

metatype · 2018-12-06T17:00:17Z

boms/build.gradle

+ * limitations under the License.
+ */
+
+jar.enabled = false


No changes needed, but it would be nice if the build didn't assume that every subdirectory is a java project.

I know right? This is a result of us putting java in a subproject block. Gradle treats all directories as projects so all directories are Java!

kirklund

I don't think org.apache.geode.cq is a good package name. We're moving these classes from org.apache.geode.cache.query.internal.cq and cq (Continuous Query) is a sub-feature of query. I think it should be:

org.apache.geode.cache.query.cq

jake-at-work · 2018-12-06T18:18:43Z

@kirklund I tried to keep the package names, module names and project names consistent. I am up for any naming really but we need consistency that we don't have today. The rational for package org.apache.geode.cq is that fits with the Maven coordinate of org.apache.geode:geode-cq and the module name of org.apache.geode.cq.

So you are asking for classes in org.apache.geode.cache.query.internal.cq to move to org.apache.geode.cache.query.cq.internal?

I think over time what we will see then is that stuff in gemfire-core at org.apache.geode.cache.query.internal.cq should migrate to org.apache.geode.cache.query.cq.spi and things in geode-cq that implement those SPIs will move to org.apache.geode.cache.query.cq.impl or something like that.

Feels a little verbose but works for me I guess. What module name would you suggest?

PurelyApplied

Looks solid. My only real complaint is with the ext. in the gradles. Great cleanups throughout in addition to the moves.

PurelyApplied · 2018-12-06T18:18:22Z

geode-docs/tools_modules/http_session_mgmt/weblogic_setting_up_the_module.html.md.erb

@@ -91,7 +91,7 @@ If you are deploying an ear file:
    lib/fastutil-7.0.2.jar
    lib/geode-core-1.0.0.jar
    lib/geode-json-1.0.0.jar
-    lib/javax.transaction-api-1.2.jar
+    lib/javax.transaction-api-1.3.jar


I don't really know what this file is documenting, but every other version here is wildly out of date

These are the minimum changes to get modules working from a client. Yes other deps may be old but out of scope for this work.

PurelyApplied · 2018-12-06T18:19:05Z

geode-core/src/main/java/org/apache/geode/internal/InternalDataSerializer.java

+      "org.apache.geode.distributed.internal.tcpserver";
+  private static final String POST_GEODE_190_SERVER_CQIMPL =
+      "org.apache.geode.cq.internal.cache.query.ServerCQImpl";
+  private static final String PRE_GEODE_190_SERVER_CQIMPL =


Always love to see these simple sorts of cleanups.

PurelyApplied · 2018-12-06T18:39:46Z

geode-dunit/src/main/java/org/apache/geode/test/dunit/internal/ProcessManager.java

  private String[] buildJavaCommand(int vmNum, int namingPort, String version) {
-    String cmd = System.getProperty("java.home") + File.separator + "bin" + File.separator + "java";
+    String cmd = System.getProperty("java.home") + File.separator
+        + "bin" + File.separator + "java";


Just a reminder that, by popular request, spotless will no longer remove extra whitespace within a line, even if the result will no longer exceed the 80-width limit. For instance, this line's diff isn't necessary for spotless (but I assume it was at some point during your iteration).

PurelyApplied · 2018-12-06T18:40:51Z

geode-core/build.gradle

@@ -104,6 +104,8 @@ task createVersionPropertiesFile(dependsOn: ':writeBuildInfo') {
  }
 }

+ext.moduleName = group + '.core'


I'm really not a fan of bloating out the ext in our project / subprojects. Is there a reason this shouldn't just be in-lined below?

The modules "plugin" expects it project. It is consistent with the experimental plugin from Gradle. Eventually they will have first level support for this.

Should also mention that it goes away completely with the upcoming module-info.java changes.

Ah, makes sense and good to hear.

PurelyApplied · 2018-12-06T18:41:10Z

geode-cq/build.gradle

@@ -60,3 +60,12 @@ dependencies {

  upgradeTestRuntime(project(':geode-old-versions'))
 }
+
+ext.moduleName = group + '.cq'


Ditto ext comment in core.

kirklund · 2018-12-06T21:34:28Z

I forgot geode-cq is a module but geode-query is not [edited]. It still seems like cq is part of query(ing) though. We can't use org.apache.geode.cache.query.cq inside geode-cq? Or is that just convention? Would we ever create geode-query to contain org.apache.geode.cache.query and then geode-cq adds cq onto that?

I'm thinking that geode-core will eventually contain org.apache.geode.logging to contain our LogService and SPI for the LogWriter and Alert appenders (or even move that to geode-logging). Then geode-log4j would contain our default impls of that SPI which are the LogWriter and Alert appenders that are written with log4j-core. Then geode-core uses geode-logging and geode-log4j is an optional provider to the SPI in geode-logging. Or is that too many sub-modules (I'm new to modules!)?

So, my input on cq is based mainly on cq being something that builds on query. If org.apache.geode.cache.query.cq is ugly because of geode-cq or if we would never split out query to geode-query then my suggestion probably isn't very relevant. Either way I'll mark my request as resolved and let you decide which way to go.

[I edited the package names to include ".cache."]

kirklund

See my latest comment on the PR itself.

jake-at-work · 2018-12-06T23:57:26Z

@kirklund

We can't use org.apache.geode.cache.query.cq inside geode-cq? Or is that just convention?

Since packageorg.apache.geode.cache.query.cq is not in the geode-core module we could use that package in geode-cq module. It is convention to keep package, module and artifact names similar. There are certainly good reasons to deviate and plenty of examples in the wild where people have. I would be most inclined to keep the module name and artifact name in sync since that is what module automatic naming would do anyway. So artifact geode-cq.jar would be automatic module geode.cq. So for consistency I would just append the group name, minus redundancy, to get module org.apache.geode.cq. In doing that I lean towards keeping the root package name the same, but eh... I do feel like the cache part of o.a.geode.cache.cq is redundant though. What's he argument for cache?

Would we ever create geode-query to contain org.apache.geode.cache.query and then geode-cq adds cq onto that?

Maybe... We can alway refactor later if we do and it feels better to have cq someone logically grouped under query at that point. Having two modules that contain the same bits with different names would be fine as long as one and only one is on the module path at any time. You would simply stop including the deprecated on in the BOM.

I'm thinking that geode-core will eventually contain org.apache.geode.logging to contain our LogService and SPI for the LogWriter and Alert appenders (or even move that to geode-logging). Then geode-log4j would contain our default impls of that SPI which are the LogWriter and Alert appenders that are written with log4j-core. Then geode-core uses geode-logging and geode-log4j is an optional provider to the SPI in geode-logging. Or is that too many sub-modules (I'm new to modules!)?

No, that sounds perfect.

So, my input on cq is based mainly on cq being something that builds on query. If org.apache.geode.cache.query.cq is ugly because of geode-cq or if we would never split out query to geode-query then my suggestion probably isn't very relevant. Either way I'll mark my request as resolved and let you decide which way to go.

All valid thoughts. It would be good for us to get it right and not have to deprecate and refactor a module later.

I guess I could fall sort of in the middle. I wouldn't have the package name to deviate too far from the module name unless there is a really strong argument. To me the "cache" part feels redundant since Geode is the "cache". So I could see module and package name of o.a.geode.query.cq and deviating only in the artifact name not match at this point and remaining geode-cq. But in my mind adding the "query" part is redundant to the "cq" part and removing it makes all 3 names consistent. Hmmm...

Any additional thoughts before I flip a coin?

kirklund · 2018-12-07T00:15:04Z

The difference between org.apache.geode.cache.query and org.apache.geode.query is fairly small but they are non-internal User APIs. So for fixing modularization I would lean away from changing the User packages -- or do so as little as possible. The impression I got on the dev-list was that we were just shuffling around internal packages without reshaping our non-internal User packages. You should probably be more explicit about exposing or altering User packages (especially on the dev-list). That's also why I was asking for org.apache.geode.cache.query.cq instead of org.apache.geode.cq -- because I feel like we're making a pretty permanent decision that impacts the User API/package for CQ. It'll be next to impossible to change that after Geode 1.9.0 releases.

jake-at-work · 2018-12-07T00:24:16Z

@kirklund
No user facing public API packages are being altered in this PR. I think if I had to put my sticking point somewhere it would be on keeping module and artifact names consistent. I am less sticky on the package names inside the modules being consistent with the module name. There is precedent for that even in Java where they have packages javax.foo in modules named java.foo.

So lets do this:
Artifact: org.apache.geode:geode-cq (no change from previous releases)
Module: org.apache.geode.cq
Root package: org.apache.geode.cache.query.cq.internal

Yes?

kirklund · 2018-12-07T02:00:58Z

Thumbs up! Already approved.

jake-at-work and others added 13 commits November 28, 2018 20:35

Adds geode-client BOM.

8f85cf1

Authored-by: Jacob Barrett <jbarrett@pivotal.io>

Fixes tests

813c5d9

Fixes tests

7c4114f

Authored-by: Jacob Barrett <jbarrett@pivotal.io>

Fixes tests

8105010

Co-authored-by: Jacob Barrett <jbarrett@pivotal.io> Co-authored-by: Owen Nichols <onichols@pivotal.io>

Fixes tests

d487a86

Co-authored-by: Jacob Barrett <jbarrett@pivotal.io> Co-authored-by: Owen Nichols <onichols@pivotal.io>

Fixes tests

4e44c08

Co-authored-by: Jacob Barrett <jbarrett@pivotal.io> Co-authored-by: Owen Nichols <onichols@pivotal.io>

Fixes classname after refactor

03b6da4

bump

b21883c

Authored-by: Jacob Barrett <jbarrett@pivotal.io>

Fixes version mixing issues with upgrade tests.

654492c

Fixes version checking.

e17f31b

Updates transaction-api version.

91cdf32

Authored-by: Jacob Barrett <jbarrett@pivotal.io>

Merge remote-tracking branch 'origin/develop' into wip/modules-client

8a7ebdd

jake-at-work requested a review from metatype November 30, 2018 19:00

jake-at-work changed the title ~~NO REVIEW: Makes modules out of geode-core and geode-cq~~ Makes modules out of geode-core and geode-cq Nov 30, 2018

jake-at-work changed the title ~~Makes modules out of geode-core and geode-cq~~ GEODE-6117: Makes modules out of geode-core and geode-cq Nov 30, 2018

jake-at-work requested a review from upthewaterspout November 30, 2018 19:05

Use Spring BOM plugin.

4bd0ab4

jake-at-work mentioned this pull request Dec 1, 2018

GEODE-6101: Builds module for consumption by Java 9+ modules project. #2911

Closed

6 tasks

mcmellawatt reviewed Dec 5, 2018

View reviewed changes

PR feedback

c0204eb

metatype reviewed Dec 6, 2018

View reviewed changes

Updates expected pom checks.

30c66be

kirklund requested changes Dec 6, 2018

View reviewed changes

PurelyApplied requested changes Dec 6, 2018

View reviewed changes

PurelyApplied approved these changes Dec 6, 2018

View reviewed changes

jake-at-work mentioned this pull request Dec 6, 2018

GEODE-6154: Remove optional from geode-core spring-shell dependency #2961

Merged

kirklund approved these changes Dec 6, 2018

View reviewed changes

jake-at-work added 2 commits December 6, 2018 22:40

PR feedback. Use org.apache.geode.cache.query.cq.internal.

116d3a4

Fixes serializable tests

3ef8b49

jake-at-work merged commit 22a5017 into apache:develop Dec 7, 2018

jake-at-work deleted the wip/modules-client branch December 7, 2018 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GEODE-6117: Makes modules out of geode-core and geode-cq #2915

GEODE-6117: Makes modules out of geode-core and geode-cq #2915

jake-at-work commented Nov 29, 2018

mcmellawatt Dec 5, 2018

jake-at-work Dec 5, 2018

mcmellawatt Dec 5, 2018

jake-at-work Dec 5, 2018

mcmellawatt Dec 5, 2018

jake-at-work Dec 5, 2018

jake-at-work commented Dec 5, 2018

metatype Dec 6, 2018

jake-at-work Dec 6, 2018

kirklund left a comment

jake-at-work commented Dec 6, 2018 •

edited

PurelyApplied left a comment

PurelyApplied Dec 6, 2018

jake-at-work Dec 6, 2018

PurelyApplied Dec 6, 2018

jake-at-work Dec 6, 2018

PurelyApplied Dec 6, 2018

PurelyApplied Dec 6, 2018

jake-at-work Dec 6, 2018

jake-at-work Dec 6, 2018

PurelyApplied Dec 6, 2018

PurelyApplied Dec 6, 2018

kirklund commented Dec 6, 2018 •

edited

kirklund left a comment

jake-at-work commented Dec 6, 2018 •

edited

kirklund commented Dec 7, 2018

jake-at-work commented Dec 7, 2018

kirklund commented Dec 7, 2018

GEODE-6117: Makes modules out of geode-core and geode-cq #2915

GEODE-6117: Makes modules out of geode-core and geode-cq #2915

Conversation

jake-at-work commented Nov 29, 2018

For all changes:

Note:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jake-at-work commented Dec 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirklund left a comment

Choose a reason for hiding this comment

jake-at-work commented Dec 6, 2018 • edited

PurelyApplied left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirklund commented Dec 6, 2018 • edited

kirklund left a comment

Choose a reason for hiding this comment

jake-at-work commented Dec 6, 2018 • edited

kirklund commented Dec 7, 2018

jake-at-work commented Dec 7, 2018

kirklund commented Dec 7, 2018

jake-at-work commented Dec 6, 2018 •

edited

kirklund commented Dec 6, 2018 •

edited

jake-at-work commented Dec 6, 2018 •

edited