You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Jared Stewart <js...@pivotal.io> on 2017/06/01 23:10:06 UTC
Review Request 59736: GEODE-2933: jmx-manager-hostname-for-clients
should be a gfsh option
-----------------------------------------------------------
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
Re: Review Request 59736: GEODE-2933: jmx-manager-hostname-for-clients
should be a gfsh option
Posted by Patrick Rhomberg <pr...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59736/#review176794
-----------------------------------------------------------
1) `¯_(?)_/¯`
2) I think the setting of defaults would belong to the CommandArguments builder. I think it would read nicely for that to be wrapped in a `try` with the error returns in lines 225 and 235 as caught exceptions.
3) I think this overlap could be resolved cleanly with a `LocatorLauncher.Builder.buildFromArgs(argsObject)` method. We already have a `*.parseArguments`, which could be removed/depricated to help cleanly define that line if we want.
4) I like the `(method, args)` as a paradigm, but I don't know how extensive an effort would be needed to standardize this across the codebase.
- Patrick Rhomberg
On June 1, 2017, 11:10 p.m., Jared Stewart wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59736/
> -----------------------------------------------------------
>
> (Updated June 1, 2017, 11:10 p.m.)
>
>
> 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
>
>
Re: Review Request 59736: GEODE-2933: jmx-manager-hostname-for-clients
should be a gfsh option
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59736/#review176899
-----------------------------------------------------------
It does seem lots of added boilerplate code. And we need to repeat that for start server as well unless we put the common code in a parent class. the complexity seems to be big.
To unit test a single command without having to pass in all these null values, I think we might be able to use GfshParseResult to do this. I cooked up a GfshParserRule in this PR: https://github.com/apache/geode/pull/557. Perhaps you can see if you can use it. See LauncherLifeCycleCommandTest for example.
- Jinmei Liao
On June 1, 2017, 11:10 p.m., Jared Stewart wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59736/
> -----------------------------------------------------------
>
> (Updated June 1, 2017, 11:10 p.m.)
>
>
> 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
>
>