Re: [DISCUSS] enable GitHub PR blocking on API breaking changes
INFRA has added the status to the required list for merging PRs to geode/develop. Thanks everyone. On Fri, May 15, 2020 at 3:03 PM Robert Houghton wrote: > Please refer to https://issues.apache.org/jira/browse/INFRA-20270 > > On Fri, May 15, 2020 at 2:58 PM Robert Houghton > wrote: > >> Support seems to be unanimously good, and folks voted in this thread. I'm >> going to make the ASF-INFRA request. >> >> On Fri, May 15, 2020 at 11:47 AM Anthony Baker wrote: >> >>> Barry and I tossed up a draft PR to fix a problem in session state >>> replication with Tomcat. If we can get this completed I’d like to include >>> it with v1.13.0. I believe our tests will fail with any version of Tomcat >>> after 9.0.21. >>> >>> Anthony >>> >>> >>> > On May 15, 2020, at 1:27 AM, Ju@N wrote: >>> > >>> > +1 >>> > >>> > On Thu, 14 May 2020 at 22:19, Mark Hanson wrote: >>> > >>> >> +1. The more we can automate this types of checks the better. >>> >> >>> >> Thanks, >>> >> Mark >>> >> >>> >> >>> > >>> > -- >>> > Ju@N >>> >>>
Re: [PROPOSAL] Move definition of Region separator character to geode-common
I vote that this separator is not moved into a common module. We should try to stop leaking things out of “modules”. The only reason why we have a requirement on the “/“ is to symbolize contextualization. I assume this came about because of sub-regions. I think we should rather aim to improve this behavior, by assuming that Regions are on “root” level. That means “/RegionName” should be expressed as “RegionName”. I think that the dependency on “/“ in the Region name is something that we should remove. But for now, not to make this worse, let's not add this constant into places where it does not belong. As @Anil stated, it should not be visible, so let’s not make it visible. Let’s rather fix the API’s to not require this contextual, syntactical sugar that we have forced onto a name. But in the meantime, we can have a private constant in the management module. —Udo On May 18, 2020, 12:25 PM -0700, Jacob Barrett , wrote: On May 18, 2020, at 10:15 AM, Udo Kohlmeyer wrote: I was wondering. Why do we require to add this Region.SEPERATOR to be anywhere outside of Region. Geode-management was purposefully designed NOT to have a dependency on core. Creating a new dependency on a donor module, just means that management module will now start knowing about geode. The proposal was updated to move the separator to common not core. Module geode-management already has a dependency on geode-common.
Re: [PROPOSAL] Move definition of Region separator character to geode-common
> On May 18, 2020, at 10:15 AM, Udo Kohlmeyer wrote: > > I was wondering. Why do we require to add this Region.SEPERATOR to be > anywhere outside of Region. > > Geode-management was purposefully designed NOT to have a dependency on core. > Creating a new dependency on a donor module, just means that management > module will now start knowing about geode. The proposal was updated to move the separator to common not core. Module geode-management already has a dependency on geode-common.
Re: [PROPOSAL] Move definition of Region separator character to geode-common
I'm fine with leaving the geode-management module out of this refactor and leaving the SEPARATOR definition inside the Region interface in geode-code if that's what's best to maintain proper modularity. However, looking at the geode-management module, there is already a Region class which has methods to strip "/" from the start of region paths, an Index class which does the same, and RegionType and IndexType Enums which are basically reimplementations of the RegionShortcut and IndexType classes in geode-core, so it looks like if we want to have it so that the only module that knows about Region internals is geode-core, then there's potentially some amount of work to be done to make that happen. On Mon, May 18, 2020 at 10:16 AM Udo Kohlmeyer wrote: > I was wondering. Why do we require to add this Region.SEPERATOR to be > anywhere outside of Region. > > Geode-management was purposefully designed NOT to have a dependency on > core. Creating a new dependency on a donor module, just means that > management module will now start knowing about geode. > > I suggest if you want to make sure that management also uses a common > Region.SEPARATOR, then maybe create a class inside of management for now OR > we have to look at management to better understand WHY it requires this > knowledge and if there could not be a different implementation to avoid > creating a new donor project. > > The whole idea behind modularity is that modules don't expose their > internals. The Region Separator is REGION specific. That knowledge should > be kept there. It should not be proliferated around or moved into a > "common" module, just because there is a leak. > > @Donal you are closest to the code, but would it maybe not make more sense > to just define that constant and maybe raise a JIRA so that we can address > this "leakage" at a later stage? > > --Udo > > From: Jacob Barrett > Sent: Saturday, May 16, 2020 11:02 PM > To: dev@geode.apache.org > Subject: Re: [PROPOSAL] Move definition of Region separator character to > geode-common > > Probably. Unfortunately we haven’t been very good and cleaning these up > and moving forward with a Java modules plan. It’s gunna bite us. > > > On May 16, 2020, at 8:08 PM, Donal Evans wrote: > > > > In that case, would it also make sense to move the existing > GeodeGlossary > > class to org.apache.geode.common.internal, from its current location in > > org.apache.geode.util.internal? > > > >> On Sat, May 16, 2020 at 8:02 PM Jacob Barrett > wrote: > >> > >> I am fine as long as you make sure you use a package name that is going > to > >> be Java 9 modules safe. Two modules cannot export the same package. So > if > >> geode-commons is going to export org.apache.geode.util I think we will > have > >> collisions. I suggest org.apache.geode.common. > >> > >> -Jake > >> > >> > On May 16, 2020, at 1:23 PM, Donal Evans wrote: > >>> > >>> I've recently been working on a little side project to replace every > use > >> of > >>> a hardcoded "/" character in region names/paths with a reference to the > >>> Region.SEPARATOR constant. I ran into some problems though, since the > >>> geode-management module needs to know about the separator character (in > >> the > >>> Region and Index classes) but does not have a dependency on geode-core, > >>> where the character is currently defined. > >>> > >>> Since the whole point of the exercise is to attempt to provide a single > >>> place where the region separator character is defined, pulling the > >>> definition down into a module upon which both geode-core and > >>> geode-management depend seems like the sensible choice, so I'm > proposing > >> to > >>> create a GeodePublicGlossary class (name entirely up for change) in the > >>> geode-common/src/main/java/org/apache/geode/util/ package, moving the > >>> definition there, then deprecating the definitions in the Region > >> interface > >>> in geode-core. > >>> > >>> To preempt a possible question, there already exists a GeodeGlossary > >> class > >>> (which defines the GEMFIRE_PREFIX constant), but it's in an internal > >>> package, so isn't a suitable place to move the definition of the > >> currently > >>> user-visible region separator character. > >>> > >>> Any feedback or suggestions on this idea would be very welcome. > >> > >> >
Re: [PROPOSAL] Move definition of Region separator character to geode-common
The Region separator should not be user visible. In the past, we had tried to remove needing this from the end-user or any other place. If we look into its usage, it is mostly for sub-regions and we don't recommend much use of this. I was also wondering, its use by external or management modules have to know about it; they just have to pass in what the user has provided or typed in. -Anil. On Mon, May 18, 2020 at 10:16 AM Udo Kohlmeyer wrote: > I was wondering. Why do we require to add this Region.SEPERATOR to be > anywhere outside of Region. > > Geode-management was purposefully designed NOT to have a dependency on > core. Creating a new dependency on a donor module, just means that > management module will now start knowing about geode. > > I suggest if you want to make sure that management also uses a common > Region.SEPARATOR, then maybe create a class inside of management for now OR > we have to look at management to better understand WHY it requires this > knowledge and if there could not be a different implementation to avoid > creating a new donor project. > > The whole idea behind modularity is that modules don't expose their > internals. The Region Separator is REGION specific. That knowledge should > be kept there. It should not be proliferated around or moved into a > "common" module, just because there is a leak. > > @Donal you are closest to the code, but would it maybe not make more sense > to just define that constant and maybe raise a JIRA so that we can address > this "leakage" at a later stage? > > --Udo > > From: Jacob Barrett > Sent: Saturday, May 16, 2020 11:02 PM > To: dev@geode.apache.org > Subject: Re: [PROPOSAL] Move definition of Region separator character to > geode-common > > Probably. Unfortunately we haven’t been very good and cleaning these up > and moving forward with a Java modules plan. It’s gunna bite us. > > > On May 16, 2020, at 8:08 PM, Donal Evans wrote: > > > > In that case, would it also make sense to move the existing > GeodeGlossary > > class to org.apache.geode.common.internal, from its current location in > > org.apache.geode.util.internal? > > > >> On Sat, May 16, 2020 at 8:02 PM Jacob Barrett > wrote: > >> > >> I am fine as long as you make sure you use a package name that is going > to > >> be Java 9 modules safe. Two modules cannot export the same package. So > if > >> geode-commons is going to export org.apache.geode.util I think we will > have > >> collisions. I suggest org.apache.geode.common. > >> > >> -Jake > >> > >> > On May 16, 2020, at 1:23 PM, Donal Evans wrote: > >>> > >>> I've recently been working on a little side project to replace every > use > >> of > >>> a hardcoded "/" character in region names/paths with a reference to the > >>> Region.SEPARATOR constant. I ran into some problems though, since the > >>> geode-management module needs to know about the separator character (in > >> the > >>> Region and Index classes) but does not have a dependency on geode-core, > >>> where the character is currently defined. > >>> > >>> Since the whole point of the exercise is to attempt to provide a single > >>> place where the region separator character is defined, pulling the > >>> definition down into a module upon which both geode-core and > >>> geode-management depend seems like the sensible choice, so I'm > proposing > >> to > >>> create a GeodePublicGlossary class (name entirely up for change) in the > >>> geode-common/src/main/java/org/apache/geode/util/ package, moving the > >>> definition there, then deprecating the definitions in the Region > >> interface > >>> in geode-core. > >>> > >>> To preempt a possible question, there already exists a GeodeGlossary > >> class > >>> (which defines the GEMFIRE_PREFIX constant), but it's in an internal > >>> package, so isn't a suitable place to move the definition of the > >> currently > >>> user-visible region separator character. > >>> > >>> Any feedback or suggestions on this idea would be very welcome. > >> > >> >
Re: [PROPOSAL] Move definition of Region separator character to geode-common
I was wondering. Why do we require to add this Region.SEPERATOR to be anywhere outside of Region. Geode-management was purposefully designed NOT to have a dependency on core. Creating a new dependency on a donor module, just means that management module will now start knowing about geode. I suggest if you want to make sure that management also uses a common Region.SEPARATOR, then maybe create a class inside of management for now OR we have to look at management to better understand WHY it requires this knowledge and if there could not be a different implementation to avoid creating a new donor project. The whole idea behind modularity is that modules don't expose their internals. The Region Separator is REGION specific. That knowledge should be kept there. It should not be proliferated around or moved into a "common" module, just because there is a leak. @Donal you are closest to the code, but would it maybe not make more sense to just define that constant and maybe raise a JIRA so that we can address this "leakage" at a later stage? --Udo From: Jacob Barrett Sent: Saturday, May 16, 2020 11:02 PM To: dev@geode.apache.org Subject: Re: [PROPOSAL] Move definition of Region separator character to geode-common Probably. Unfortunately we haven’t been very good and cleaning these up and moving forward with a Java modules plan. It’s gunna bite us. > On May 16, 2020, at 8:08 PM, Donal Evans wrote: > > In that case, would it also make sense to move the existing GeodeGlossary > class to org.apache.geode.common.internal, from its current location in > org.apache.geode.util.internal? > >> On Sat, May 16, 2020 at 8:02 PM Jacob Barrett wrote: >> >> I am fine as long as you make sure you use a package name that is going to >> be Java 9 modules safe. Two modules cannot export the same package. So if >> geode-commons is going to export org.apache.geode.util I think we will have >> collisions. I suggest org.apache.geode.common. >> >> -Jake >> >> On May 16, 2020, at 1:23 PM, Donal Evans wrote: >>> >>> I've recently been working on a little side project to replace every use >> of >>> a hardcoded "/" character in region names/paths with a reference to the >>> Region.SEPARATOR constant. I ran into some problems though, since the >>> geode-management module needs to know about the separator character (in >> the >>> Region and Index classes) but does not have a dependency on geode-core, >>> where the character is currently defined. >>> >>> Since the whole point of the exercise is to attempt to provide a single >>> place where the region separator character is defined, pulling the >>> definition down into a module upon which both geode-core and >>> geode-management depend seems like the sensible choice, so I'm proposing >> to >>> create a GeodePublicGlossary class (name entirely up for change) in the >>> geode-common/src/main/java/org/apache/geode/util/ package, moving the >>> definition there, then deprecating the definitions in the Region >> interface >>> in geode-core. >>> >>> To preempt a possible question, there already exists a GeodeGlossary >> class >>> (which defines the GEMFIRE_PREFIX constant), but it's in an internal >>> package, so isn't a suitable place to move the definition of the >> currently >>> user-visible region separator character. >>> >>> Any feedback or suggestions on this idea would be very welcome. >> >>
