The only concern I would have is that this might cause gfsh to load other things that implement the spring CommandMarker that have nothing to do with gfsh simply because they are in one of the jars. I really don't know how likely that would be to happen but it seems more likely since the scan happens on both sides.
Since the default for this property is the empty string you could just change the default behavior to scan everything (instead of nothing) but still allow them to limit where it scans be specifying a non-default value. On Thu, May 25, 2017 at 2:30 PM, Jinmei Liao <jil...@pivotal.io> wrote: > both sides. > > On Thu, May 25, 2017 at 2:16 PM, Darrel Schneider <dschnei...@pivotal.io> > wrote: > > > What JVM does this scanning? Is it just the top-level shell JVM or does > it > > happen on the jmx-manager side? > > > > On Thu, May 25, 2017 at 1:01 PM, Jared Stewart <jstew...@pivotal.io> > > wrote: > > > > > It sounds like we're on the same page. > > > > > > The current behavior requires two pieces from a user who wants to add > > > commands: > > > 1) implement CommandMarker > > > 2) Add a system property pointing to the package of your class, or > > provide > > > a META-INF.services file pointing to your class > > > > > > The proposed behavior is to drop 2) and rely on 1) alone. > > > > > > On May 25, 2017 12:55 PM, "Udo Kohlmeyer" <ukohlme...@pivotal.io> > wrote: > > > > > > What I meant was, given our hammer of choice for GFSH is Spring Shell, > > all > > > command definitions should be of type CommandMarker. > > > > > > Not sure how it loads the correct CommandMarker classes (for different > > > commands). But my gut feel is, have single way of doing something... > KISS > > > > > > > > > > > > On 5/25/17 12:22, Jared Stewart wrote: > > > > > > > Can you clarify - I'm proposing we use *implements CommandMarker* > alone > > > as > > > > the way to specify commands. What do we gain be *also* requiring a > > > > META-INF.services file? > > > > > > > > On May 25, 2017 12:18 PM, "Udo Kohlmeyer" <ukohlme...@pivotal.io> > > wrote: > > > > > > > > imo, I think that GFSH is Spring Shell, it should really only be > > commands > > > > that are registered inside of META-INF.services .. aka implements > > > > CommandMarker. > > > > > > > > This way we has standard simple way to specify commands. > > > > > > > > --Udo > > > > > > > > > > > > > > > > On 5/25/17 11:52, Jared Stewart wrote: > > > > > > > > I would like to propose that we eliminate the “user-command-packages” > > > >> system property, in favor of scanning the entire classpath to find > > > >> commands. > > > >> > > > >> To give more detail, Geode currently supports three ways to load > GFSH > > > >> commands: > > > >> Scan the classpath for commands in "org.apache.geode.management.i > > > >> nternal.cli.commands” > > > >> Scan the classpath for commands in a package specified by a user via > > the > > > >> “user-command-packages” system property. > > > >> Scan the classpath for commands registered in files inside > > > >> META-INF.services (e.g. "geode-core/src/test/resources > > > >> /META-INF/services/org.springframework.shell.core.CommandMarker”) > > > >> > > > >> We have some bespoke code responsible for scanning the classpath > > > >> (ClasspathScanLoadHelper) that I am in the process of replacing with > > > >> FastClasspathScanner (GEODE-2989). Since FastClasspathScanner will > no > > > >> longer need to eagerly load all of the classes in the target > package, > > I > > > >> don’t see any reason why a user should need to specify a > > > >> “user-command-package” property or a “META-INF/services” file. > > Instead, > > > >> we > > > >> should just scan the whole classpath and register any commands that > we > > > >> find. > > > >> > > > >> Thoughts? > > > >> > > > >> > > > > > > > > > -- > Cheers > > Jinmei >
