You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "pivotal-jbarrett (GitHub)" <gi...@apache.org> on 2018/11/29 04:38:07 UTC
[GitHub] [geode] pivotal-jbarrett opened pull request #2915: NO
REVIEW: Makes modules out of geode-core and geode-cq
- 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 <jb...@pivotal.io>
Co-authored-by: Owen Nichols <on...@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](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/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on issue #2915: GEODE-6117: Makes
modules out of geode-core and geode-cq
Posted by "kirklund (GitHub)" <gi...@apache.org>.
I forgot about geode-cq as a module. It still seems like cq is part of query(ing) though. We can't use org.apache.geode.query.cq inside geode-cq? Or is that just convention? Would we ever create geode-query to contain org.apache.geode.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.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.
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett closed pull request #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
[ pull request closed by pivotal-jbarrett ]
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
The modules "plugin" expects it project. It is consistent with the experimental plugin from Gradle. Eventually they will have first level support for this.
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on issue #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
@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?
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Not sure this comment is very helpful
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on issue #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
@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.
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.
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on issue #2915: GEODE-6117: Makes
modules out of geode-core and geode-cq
Posted by "kirklund (GitHub)" <gi...@apache.org>.
Thumbs up! Already approved.
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on issue #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
@kirklund
> We can't use org.apache.geode.cache.query.cq inside geode-cq? Or is that just convention?
Since package`org.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?
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] PurelyApplied commented on pull request #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "PurelyApplied (GitHub)" <gi...@apache.org>.
Ditto `ext` comment in core.
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
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`.
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on issue #2915: GEODE-6117: Makes
modules out of geode-core and geode-cq
Posted by "kirklund (GitHub)" <gi...@apache.org>.
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.
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] PurelyApplied commented on pull request #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "PurelyApplied (GitHub)" <gi...@apache.org>.
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?
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on issue #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
@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?
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
👍
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on issue #2915: GEODE-6117: Makes
modules out of geode-core and geode-cq
Posted by "kirklund (GitHub)" <gi...@apache.org>.
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.query.cq inside geode-cq? Or is that just convention? Would we ever create geode-query to contain org.apache.geode.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.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.
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
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.
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on issue #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
@mcmellawatt can we get a re-review and blessing?
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] PurelyApplied commented on pull request #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "PurelyApplied (GitHub)" <gi...@apache.org>.
Always love to see these simple sorts of cleanups.
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] PurelyApplied commented on pull request #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "PurelyApplied (GitHub)" <gi...@apache.org>.
I don't really know what this file is documenting, but every other version here is wildly out of date
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Should also mention that it goes away completely with the upcoming `module-info.java` changes.
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
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!
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
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.
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
This string and the one below it are used twice in this class. Maybe extract them into `static final` fields?
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
It isn't, got left on accident. Thanks!
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Sure could. Was staying consistent with the other swizzle happening here.
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] PurelyApplied commented on pull request #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "PurelyApplied (GitHub)" <gi...@apache.org>.
Ah, makes sense and good to hear.
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on issue #2915: GEODE-6117: Makes
modules out of geode-core and geode-cq
Posted by "kirklund (GitHub)" <gi...@apache.org>.
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."]
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on issue #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
@kirklund
> We can't use org.apache.geode.cache.query.cq inside geode-cq? Or is that just convention?
Since package`org.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?
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] PurelyApplied commented on pull request #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "PurelyApplied (GitHub)" <gi...@apache.org>.
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).
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] metatype commented on pull request #2915:
GEODE-6117: Makes modules out of geode-core and geode-cq
Posted by "metatype (GitHub)" <gi...@apache.org>.
No changes needed, but it would be nice if the build didn't assume that every subdirectory is a `java` project.
[ Full content available at: https://github.com/apache/geode/pull/2915 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org