> On April 5, 2016, 10 p.m., John Blum wrote: > > geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/commands/LauncherLifecycleCommands.java, > > line 1146 > > <https://reviews.apache.org/r/45509/diff/2/?file=1326856#file1326856line1146> > > > > So remind me again... we are requiring the user to explicility specify > > all (required) Spring JARS on the classpath (using the --classpath option) > > when using the --spring-xml-location option on 'start server'? > > > > I guess this is consistent with the user having to explicility include > > the SDG JAR on the CP when starting a GemFire Server configured with Spring > > using Gfsh. > > Dan Smith wrote: > Yeah, this means the user will need to add the spring jars using > --classpath. > > I could have left this code in, but then there would be no tests and the > user would get this magic behavior if they dropped the right jars int the lib > directory. I figured it was better to make them be more explicit and follow > the public API for adding to the classpath.
+1 > On April 5, 2016, 10 p.m., John Blum wrote: > > geode-assembly/src/test/resources/expected_jars.txt, line 64 > > <https://reviews.apache.org/r/45509/diff/2/?file=1326854#file1326854line64> > > > > This might actually be required by the Developer and Management REST > > APIs, an in particular the Management REST API on a Manager when the server > > serves as a Manager. > > > > Maybe this exclusion/removal does not matter. Not even sure what this > > file is, but appears to be used to validate required JARs for testing > > purposes. > > Dan Smith wrote: > This expected_jars file is validating the list of jars we actually ship > with the binary distribution, including jars bundled inside wars. This is to > prevent us from shipping dependencies that don't meet our licensing > requirements. > > It looks like spring-context-support was only being shipped because it > was a dependency of spring-shell. BUT, we never actually added to the > classpath of the gemfire server unless --spring-xml-location was specified, > which is why I changed geode-core/build.gradle to exclude that jar since it's > seems we weren't using it. > > Are there tests I should run that might indicate if we need this > dependency in the REST apis? I'm running precheckin on these changes. Well, before Apache Geode, I used to run the Management Test Suite using HTTP by setting the (Ant) build System property -DuseHTTP=true. I am not sure how it is accomplished now with the new Gradle setup. By default, the Management (CLI command) Test Suite uses JMX between Gfsh and the Manager. Since the Management REST API enables Gfsh CLI commands to be tunneled to the Manager using HTTP, it just uses the same Management (CLI) tests to test the interface since it is exercising the same logic, just via a different transport. All the behavior as perceived by the user from Gfsh should be the same. Hmmmm.... Thinking about this further, even though the Manager runs in the same VM as the GemFire Server, and technically so does the Management REST API, it is still self-contained in WAR file, which is loaded by Jetty (err, Tomcat?) in a separate ClassLoader. Perhaps spring-context-<version>.jar is not longer needed in the CLASSPATH and was only used by the --spring-xml-location option to 'start server' when bootstrapping/configuring a GemFire Server with Spring? > On April 5, 2016, 10 p.m., John Blum wrote: > > geode-assembly/src/test/resources/expected_jars.txt, line 69 > > <https://reviews.apache.org/r/45509/diff/2/?file=1326854#file1326854line69> > > > > Not sure why Spring HATEOAS is here. I don't think we use it at all > > and was purely for experimental purposes when developing the Developer REST > > API, FYI. > > Dan Smith wrote: > It looks like spring-hateoas still has some usage in the code in > geode-web-api. See the JsonWriter and JSONUtils class. Should those > references be removed? > > ./gradlew geode-web-api:findUsage -Djar.name=hateoas Hmmm, probably not. However, it looks like the only class Nilkanth used was org.springframework.hateoas.Link, which seems a bit extreme to pull in an entirely new dependency (i.e. spring-hateoas) for. Ideally, longer term, Geode/GemFire's (Developer) REST API/Interface becomes fully REST compliant/mature (level 3) with support for full hypermedial controls using (Spring) HATEOAS. So, now that it has been introduced, it is possible users are now using "Links" as a data type in their applications; so removing this (especially in GemFire's case) is a bit more problematic. I say leave it alone. I have never heard of users complaining about conflicting versions of Spring HATEOAS in their GemFire/Geode applications because Geode/GemFire pulls in the dependency. - John ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45509/#review127225 ----------------------------------------------------------- On April 5, 2016, 9:32 p.m., Dan Smith wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45509/ > ----------------------------------------------------------- > > (Updated April 5, 2016, 9:32 p.m.) > > > Review request for geode, John Blum and Mark Bretl. > > > Repository: geode > > > Description > ------- > > Spring Data Gemfire (SDG) now implements the service provider required > to start the cache server with spring. Switching to point at the latest > SDG snapshot and renabling the test for starting with SDG. > > > Diffs > ----- > > build.gradle 85e5c3564e0a347f4442d064f800f45822d53302 > geode-assembly/build.gradle 5930f13c5307e0cd70396b019263176a377d415a > > geode-assembly/src/test/java/com/gemstone/gemfire/management/internal/cli/commands/LauncherLifecycleCommandsDUnitTest.java > afb2770487c5ea7d53b79946b18ac2c40ec8830d > > geode-assembly/src/test/java/com/gemstone/gemfire/management/internal/cli/commands/LauncherLifecycleCommandsJUnitTest.java > d7e7970aa2e989069cf7752f452237dd3feb3e4b > geode-assembly/src/test/resources/expected_jars.txt PRE-CREATION > geode-core/build.gradle 041dc07c860c008f117d37969ee688375c2a348d > > geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/commands/LauncherLifecycleCommands.java > f540e19645bd65c7a44d492d2f073d95c389adc1 > geode-rebalancer/build.gradle PRE-CREATION > geode-web/build.gradle 596590b1c96e016e2dd927a21dc50d2e373b62b2 > gradle/dependency-versions.properties > 901bdd5631af0a30ec7ea5f2ce296c6db4152d5d > > Diff: https://reviews.apache.org/r/45509/diff/ > > > Testing > ------- > > > Thanks, > > Dan Smith > >
