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 <u...@vmware.com> 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 <jbarr...@pivotal.io> > Sent: Saturday, May 16, 2020 11:02 PM > To: dev@geode.apache.org <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 <doev...@pivotal.io> 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 <jbarr...@pivotal.io> > 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 <doev...@pivotal.io> 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. > >> > >> >
