-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59736/
-----------------------------------------------------------
Review request for geode.
Repository: geode
Description
-------
I'm having trouble finding a way to test the actual product changes for
GEODE-2933 that doesn't feel hacky. I don't intend to commit this review
request in the current state. Rather, I wanted to start a discussion to
validate the general strategy and consider alternative suggestions before I get
too far into the work.
There is a StartLocatorCommandArguments class introduced that makes easier
passing around all of the arguments to startLocator(). It also allows setting
one or two arguments for tests without needing to put the other 20 nulls in the
correct positions. Thoughts/questions I still have:
1) Does the improved testability justify the added boilerplate of
StartLocatorCommandArguments?
2) Should the setting of "default values" (lines ~221-246 in
LauncherLifecycleCommands) also be moved into this class?
3) It seems like there some overlap between the intent of
StartLocatorCommandArguments and LocatorLauncher.Builder - not sure where the
boundary between these two should be.
4) There are more places that `args` could replace telescoped methods. E.g.
`createStartLocatorCommandLine` could take in `(locatorLauncher, args)` rather
than ```(locatorLauncher,
gemfirePropertiesPathname, gemfireSecurityPropertiesPathname,
gemfireProperties,
classpath, includeSystemClasspath, jvmArgsOpts, initialHeap,
maxHeap)```
Diffs
-----
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java
4c668b6813de71aae9e3e46c82a5c231a41c1f6a
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/lifecycle/StartLocatorCommandArguments.java
PRE-CREATION
geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java
9f68d3a5eaadbe8f1bd95ec8df85ed1f65aa67ce
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/lifecycle/StartLocatorCommandArgumentsTest.java
PRE-CREATION
Diff: https://reviews.apache.org/r/59736/diff/1/
Testing
-------
Thanks,
Jared Stewart
