You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Jinmei Liao <ji...@pivotal.io> on 2017/04/20 20:27:14 UTC
Review Request 58589: GEODE-1597: use Spring shell's parser and
delete our own parsing code
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58589/
-----------------------------------------------------------
Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
Repository: geode
Description
-------
GEODE-1597: use Spring shell's parser and delete our own parsing code
* deleted usage of jopsimple and our own parser code
* reworked help/hint
The biggest change is in GfshParser, instead of directly implement SpringShell's Parser, now it's just a simple wrapper around SpringShell's SimpleParser so that we can use spring's parsing code. The challenge is that SimpleParser expects the user inupt to be in the format of "command option1 value1 option2 value2", while gfsh expects the format to be like "command option1=value1 option2=value2", so most of the code in GfshParser is to turn the input into the SimpleParser input format and then feed it into SimpleParser to get the validation and autocompletion we needed.
So far the difference I noticed with this new implementation are:
1) option validation is what we gain from directly using SpringShell.
2) SpringShell doesn't allow you to specify an option twice, so for multivalued parameters, our old parser can accept command like "change loglevel --member=member1 --member=member2", but now the parser will give you an error, and you should only do "change loglevel --member=member1,member2". The new parser did something speical for --J option though, so for --J, you can specify it multiple times.
3) For value separator, springShell default's to ",", you can only overwrite it with option context "splittingRegex", we do not honor the @CliMetaData(valueSepartor=) anymore. All of our commands uses "," for separators, so this won't break our commands, but if there is any command out there that would define a different @CliMetaData(valueSepartor=) other than ",", SpringShell would not know how to parse it.
4) To specify empty string as parameter value, before you will need to to (put region=A key="''" value="''"), spring shell would think the value you are trying to pass in are two single quotes instead of empty strings, now, you should only do (put region=A key="" value="")
Diffs
-----
geode-core/src/main/java/org/apache/geode/management/cli/CliMetaData.java e69d78a2b42b4a7f21066712b9f763dc1cdb92c8
geode-core/src/main/java/org/apache/geode/management/cli/ConverterHint.java f45abc432dce8af67b71ca29fdfb85e0e9f6f87a
geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java 44004454ef1646bfeca8a7af3cffb109fd83e7d7
geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandResponseBuilder.java bda030de50cca303e2aa6d3d82b17a9111fd25ef
geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParseResult.java d879e2d6c253eb3306fb7558fa0a2b7fbe7d1e40
geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java a1d03e45dd4d6559bd9a0869c7dd95908d1858ca
geode-core/src/main/java/org/apache/geode/management/internal/cli/Launcher.java fc0427e20144a9092d97d05fc3ccbd0e21d5b35e
geode-core/src/main/java/org/apache/geode/management/internal/cli/annotation/CliArgument.java e20e73112c3b6545771c62e58795fdc53fc279c5
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ClientCommands.java 39d0442f2cb4ec4276652169a0cfa962a78af2c9
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ConfigCommands.java 50c9caaee88426f9a8c49d8b1abca9c2fbc66163
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommands.java 28ce0921e0cba412063d3a59566165544dfda3dc
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java 6324b5cfc70c0f02233f73ceab6dc56cb253cbb2
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DeployCommands.java 144408854daf2cd5b33dcdcf995efef77b3e942e
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DiskStoreCommands.java 9ad206043a284144293175baa7144bb835652a46
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/FunctionCommands.java 752ca2a612eb59c137de568f80223d58339e7231
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/GfshHelpCommands.java 1d1b28e568a1e175690fea5cde1a45a318b6db5d
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java 313d1bdfb4d1e610f2353db2a4496449453a0fbe
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommands.java 22981e72f4971f3bb297a53e9ac4d6f8ac7e149e
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/PDXCommands.java 4327decc98e3d0fe637d73857c9eed83f7bc88b9
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueueCommands.java 8c568336aa62c6b6cbf4e1a29584d514e84f1c96
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShellCommands.java b37feabe6ed61c081e9653c94128f092ad189d10
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/WanCommands.java 239db48417cdae0d415830ac411cb611b8433919
geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/BooleanConverter.java 5e9cdb9a642e9cda4934e7934c2fc022e7d9eabe
geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/DirConverter.java c97057bb281e64c96983e38172bcf7953f492c83
geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/EnumConverter.java 354cef9708623d9141caea08fe497deaa75f606f
geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/HelpConverter.java e670274f89ce4423a77715dcee07f119c1568929
geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/HintTopicConverter.java b6f9f81af10a7773df27f77ca5707238e7ef3a3b
geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/StringArrayConverter.java eacf1810ba53fc0c389cff0d5ae30ba5fea6b0c3
geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/StringListConverter.java eab096b9a5c93fd71f9a0b757d80a2d726690ad1
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandException.java 7e5cba05d93a88fa722d98babfdabf89e4e07be0
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandInvalidException.java a14005923ee3343d2e2e8e1818b8fbac16410ca6
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandMultiModeOptionException.java acbc49618504bec9f8eb6f04124e02a47e0a92c3
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandNotAvailableException.java c471df298f10211a812ae0e644c88f81c68b8132
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionException.java a7e56be98afe94690cdc1b61ea5476a72fbd9140
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionHasMultipleValuesException.java 4b365e3f9d611e74f345f50f87c682f272d20ba4
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionInvalidException.java 1db8906024934a024febf447f8d5facc5f1b2ee8
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionMissingException.java f263dce0be966a361b8b4e84ed8360cc1a659e3b
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionNotApplicableException.java 98147787eff00d74ed5e32d2c684867fd1bf605a
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionValueConversionException.java 7dbf869eab0c8331c217aed6eb82b57ea7696be6
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionValueException.java ee02df8423ef48f02ed482ad3e40260799b2e7d7
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionValueMissingException.java 023a8789fe4a0ca2fc11bce7482d0e9ed4aa1b2a
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/ExceptionGenerator.java fda31357a39830b7414b4751966dba2e57526b13
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/ExceptionHandler.java 95afbafe55746bb0159e5067ae4d5aa9c0701b56
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/CliTopic.java 791cdca5ce954153ed5013f3108d738d970b32dd
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/HelpBlock.java PRE-CREATION
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/Helper.java PRE-CREATION
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/Topic.java PRE-CREATION
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/Block.java dde1a20ef0bfa98a7b5dcf57c1775f1769ab3c97
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/DataNode.java 8f5e570cc9761ddedcb7fb16b0a9615f2da87209
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/Help.java 68c10bb211c319bed679e39718660d1a466967a9
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/NewHelp.java 90f9eda49326fa008cc5c1abb5c02f9dcfd94fd6
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/Row.java ac1ca218668aff03d3a774ae2e1ebaebe8215b3c
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/utils/FormatOutput.java 44998a03bed5203200115ce1b2023a436a52fd84
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/utils/HelpUtils.java 11765c54df0df755b7f9b12b9d50986223d6b775
geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java 126eb47e90c2390953a91136e8cfb36687baf2fd
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/Argument.java 9acbc2a82c9cf4d4ccdb629798bdf70e1e48a305
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/AvailabilityTarget.java eff5fd2c2f67280ed9037216c1b15ab8b4a462d1
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/CommandTarget.java 3dfc01ab26d0ad6ee1867ab4014028f5cc0a3649
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/GfshMethodTarget.java 6f28830c314593004dfbb5eda113540d951fb343
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/GfshOptionParser.java a64933c2e9c5e3122a856d292b2ad70949e5017c
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/MethodParameter.java 599fb00d511de87d5a0b439371a9fec62377c027
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/Option.java 4eec1128c6e4646cde7b89e04f567fee2e419ee9
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/OptionSet.java 42270e53d047de566943a59c53969bdae8b32f54
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/Parameter.java dc371b3ba58b2ede0663e3167835926d17a3a418
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/ParserUtils.java 80f12867b7c0144ef9cb5aa5d5e7cc4f5c36b9f1
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/SyntaxConstants.java 52d92128b96e119f3157e5431d579e90218cbaad
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/jopt/JoptOptionParser.java 52224d40c9c47dd730432859033701e9b8faac4d
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/EnclosingCharacters.java a35e6269bd5ba2644fd2fbeb90c6fd8a92a8b971
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/Preprocessor.java 0dd875ad95ae30a6f2ca962da683e9e9ad0b9559
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/PreprocessorUtils.java a1872c92568a65c11f52603575a7814f2eb4c3f9
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/Stack.java ae47723f1ef2ff9af2e92c9991d02205fa044bac
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/TrimmedInput.java 8740f003f5a90a6ac296317ae4efee799e713baf
geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java c346eaf77087102f51952a567ecd7ec036593a13
geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java bcf1b415e9ee85822c470ae6888920f0a90159fd
geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/MultiCommandHelper.java 89f93d5a4f491addc955eeaaa11f0318e00f35de
geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/jline/GfshHistory.java dc4a42fc014c73df16f057155229a51bd309efa3
geode-core/src/main/java/org/apache/geode/management/internal/cli/util/CommandStringBuilder.java 16efda5f772e50b3dea4899733760b0a25513a0e
geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DataCommandsController.java cfe60899503193d4079cff4798422cd4da00cd74
geode-core/src/test/java/org/apache/geode/management/internal/cli/CommandManagerJUnitTest.java 503ffb2929758d617e8539e6aefb3f7b545de9b6
geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserIntegrationTest.java f3e3bd8754e18d7a75faf4b75e3ba75b778fc9d7
geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserJUnitTest.java 44e99f461b9efc5a439e629751011a6d0edd9213
geode-core/src/test/java/org/apache/geode/management/internal/cli/JoptOptionParserTest.java 9815a9cbadae77ea94d49288922f204acf308d61
geode-core/src/test/java/org/apache/geode/management/internal/cli/annotations/CliArgumentJUnitTest.java 161f7c65e62f11aa7b25fb31cd94ad256ea497a6
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CliCommandTestBase.java 165f66482758dadb75ae95d23443973e9de05891
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommandsDUnitTest.java 5f885e136e09541a6d095516fc7cb7db88f659aa
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DeployCommandsDUnitTest.java 9ed5bed8faa6bfceb0849347cd94b7c8effd3191
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DiskStoreCommandsDUnitTest.java 5b07f282b83d1539df6a7ea7bf19888436738723
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/GemfireDataCommandsDUnitTest.java e99a7fbc859243579b75a76742e23fec9b738ef5
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/HelpCommandsIntegrationTest.java b91a1f35efaa00f29a0e858a90e55c1c39c163f9
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/QueueCommandsDUnitTest.java 42a0624546742491e0c8c4d8c406ae8e567c1f85
geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelpBlockUnitTest.java PRE-CREATION
geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelperUnitTest.java PRE-CREATION
geode-core/src/test/java/org/apache/geode/management/internal/cli/parser/ParserUtilsJUnitTest.java 9b22e64d263d6cb8c4453e2eea42fa85e7f1f922
geode-core/src/test/java/org/apache/geode/management/internal/cli/parser/preprocessor/PreprocessorJUnitTest.java 97325cb16b42a112680a793b6581a1fea0e8c621
geode-core/src/test/java/org/apache/geode/management/internal/cli/parser/preprocessor/PreprocessorUtilsJUnitTest.java b56cff2cfb7027999a420ae91f1f983243f865f0
geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategyJUnitTest.java 54c7cf7074435b48232b61916627eed69a201f09
geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshHistoryJUnitTest.java 58453b7c70b552af30ca4e16cd624475a89e3426
geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshJunitTest.java PRE-CREATION
geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigDistributionDUnitTest.java abbc5c0c7e83d259cd13bae61c419f8cf07db1f0
geode-core/src/test/java/org/apache/geode/management/internal/security/TestCommand.java ffe6a28163acf20d13b62347c40d5cceea22c629
geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/cli/LuceneIndexCommands.java 4bfa86842fd81873c132e3bbd7b529ff3e64dbc7
geode-lucene/src/test/java/org/apache/geode/cache/lucene/internal/cli/LuceneIndexCommandsDUnitTest.java 7f203cee7899340e19d589a4618be4ea7632faac
geode-lucene/src/test/java/org/apache/geode/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java 5c1832575ec5c5cccdce670bd9b3477708f79148
Diff: https://reviews.apache.org/r/58589/diff/1/
Testing
-------
precheckin passing except one unittest that always fails in concourse but succeed in local.
Thanks,
Jinmei Liao
Re: Review Request 58589: GEODE-1597: use Spring shell's parser and
delete our own parsing code
Posted by Patrick Rhomberg <pr...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58589/#review172962
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java
Line 127 (original)
<https://reviews.apache.org/r/58589/#comment245992>
Collapse the duplicate `ClassNotFoundException | IOException` catch blocks.
geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java
Line 182 (original)
<https://reviews.apache.org/r/58589/#comment245993>
Collapse duplicate catch blocks.
geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java
Lines 334 (patched)
<https://reviews.apache.org/r/58589/#comment245994>
Delete dead code.
geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java
Lines 348 (patched)
<https://reviews.apache.org/r/58589/#comment245995>
Delete dead code.
geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java
Line 97 (original), 97 (patched)
<https://reviews.apache.org/r/58589/#comment245996>
Huzzah for correcting typos.
- Patrick Rhomberg
On April 25, 2017, 1:08 a.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58589/
> -----------------------------------------------------------
>
> (Updated April 25, 2017, 1:08 a.m.)
>
>
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-1597: use Spring shell's parser and delete our own parsing code
>
> * deleted usage of jopsimple and our own parser code
> * reworked help/hint
>
> The biggest change is in GfshParser, instead of directly implement SpringShell's Parser, now it's just a simple wrapper around SpringShell's SimpleParser so that we can use spring's parsing code. The challenge is that SimpleParser expects the user inupt to be in the format of "command option1 value1 option2 value2", while gfsh expects the format to be like "command option1=value1 option2=value2", so most of the code in GfshParser is to turn the input into the SimpleParser input format and then feed it into SimpleParser to get the validation and autocompletion we needed.
>
> So far the difference I noticed with this new implementation are:
> 1) option validation is what we gain from directly using SpringShell.
> 2) SpringShell doesn't allow you to specify an option twice, so for multivalued parameters, our old parser can accept command like "change loglevel --member=member1 --member=member2", but now the parser will give you an error, and you should only do "change loglevel --member=member1,member2". The new parser did something speical for --J option though, so for --J, you can specify it multiple times.
> 3) For value separator, springShell default's to ",", you can only overwrite it with option context "splittingRegex", we do not honor the @CliMetaData(valueSepartor=) anymore. All of our commands uses "," for separators, so this won't break our commands, but if there is any command out there that would define a different @CliMetaData(valueSepartor=) other than ",", SpringShell would not know how to parse it.
> 4) To specify empty string as parameter value, before you will need to to (put region=A key="''" value="''"), spring shell would think the value you are trying to pass in are two single quotes instead of empty strings, now, you should only do (put region=A key="" value="")
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/management/cli/CliMetaData.java e69d78a2b42b4a7f21066712b9f763dc1cdb92c8
> geode-core/src/main/java/org/apache/geode/management/cli/ConverterHint.java f45abc432dce8af67b71ca29fdfb85e0e9f6f87a
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java 44004454ef1646bfeca8a7af3cffb109fd83e7d7
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandResponseBuilder.java bda030de50cca303e2aa6d3d82b17a9111fd25ef
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParseResult.java d879e2d6c253eb3306fb7558fa0a2b7fbe7d1e40
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java a1d03e45dd4d6559bd9a0869c7dd95908d1858ca
> geode-core/src/main/java/org/apache/geode/management/internal/cli/Launcher.java fc0427e20144a9092d97d05fc3ccbd0e21d5b35e
> geode-core/src/main/java/org/apache/geode/management/internal/cli/annotation/CliArgument.java e20e73112c3b6545771c62e58795fdc53fc279c5
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ClientCommands.java 39d0442f2cb4ec4276652169a0cfa962a78af2c9
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ConfigCommands.java 50c9caaee88426f9a8c49d8b1abca9c2fbc66163
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommands.java 28ce0921e0cba412063d3a59566165544dfda3dc
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java 6324b5cfc70c0f02233f73ceab6dc56cb253cbb2
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DeployCommands.java 144408854daf2cd5b33dcdcf995efef77b3e942e
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DiskStoreCommands.java 9ad206043a284144293175baa7144bb835652a46
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/FunctionCommands.java 752ca2a612eb59c137de568f80223d58339e7231
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/GfshHelpCommands.java 1d1b28e568a1e175690fea5cde1a45a318b6db5d
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java 313d1bdfb4d1e610f2353db2a4496449453a0fbe
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommands.java 22981e72f4971f3bb297a53e9ac4d6f8ac7e149e
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/PDXCommands.java 4327decc98e3d0fe637d73857c9eed83f7bc88b9
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueueCommands.java 8c568336aa62c6b6cbf4e1a29584d514e84f1c96
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShellCommands.java b37feabe6ed61c081e9653c94128f092ad189d10
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/WanCommands.java 239db48417cdae0d415830ac411cb611b8433919
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/BooleanConverter.java 5e9cdb9a642e9cda4934e7934c2fc022e7d9eabe
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/DirConverter.java c97057bb281e64c96983e38172bcf7953f492c83
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/EnumConverter.java 354cef9708623d9141caea08fe497deaa75f606f
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/HelpConverter.java e670274f89ce4423a77715dcee07f119c1568929
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/HintTopicConverter.java b6f9f81af10a7773df27f77ca5707238e7ef3a3b
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/LogLevelConverter.java 3a262405da0d9821a1c9729ca0aad21ccf626a02
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/StringArrayConverter.java eacf1810ba53fc0c389cff0d5ae30ba5fea6b0c3
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/StringListConverter.java eab096b9a5c93fd71f9a0b757d80a2d726690ad1
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandException.java 7e5cba05d93a88fa722d98babfdabf89e4e07be0
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandInvalidException.java a14005923ee3343d2e2e8e1818b8fbac16410ca6
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandMultiModeOptionException.java acbc49618504bec9f8eb6f04124e02a47e0a92c3
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandNotAvailableException.java c471df298f10211a812ae0e644c88f81c68b8132
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionException.java a7e56be98afe94690cdc1b61ea5476a72fbd9140
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionHasMultipleValuesException.java 4b365e3f9d611e74f345f50f87c682f272d20ba4
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionInvalidException.java 1db8906024934a024febf447f8d5facc5f1b2ee8
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionMissingException.java f263dce0be966a361b8b4e84ed8360cc1a659e3b
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionNotApplicableException.java 98147787eff00d74ed5e32d2c684867fd1bf605a
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionValueConversionException.java 7dbf869eab0c8331c217aed6eb82b57ea7696be6
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionValueException.java ee02df8423ef48f02ed482ad3e40260799b2e7d7
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionValueMissingException.java 023a8789fe4a0ca2fc11bce7482d0e9ed4aa1b2a
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/ExceptionGenerator.java fda31357a39830b7414b4751966dba2e57526b13
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/ExceptionHandler.java 95afbafe55746bb0159e5067ae4d5aa9c0701b56
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/CliTopic.java 791cdca5ce954153ed5013f3108d738d970b32dd
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/HelpBlock.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/Helper.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/Block.java dde1a20ef0bfa98a7b5dcf57c1775f1769ab3c97
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/DataNode.java 8f5e570cc9761ddedcb7fb16b0a9615f2da87209
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/Help.java 68c10bb211c319bed679e39718660d1a466967a9
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/NewHelp.java 90f9eda49326fa008cc5c1abb5c02f9dcfd94fd6
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/Row.java ac1ca218668aff03d3a774ae2e1ebaebe8215b3c
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/utils/FormatOutput.java 44998a03bed5203200115ce1b2023a436a52fd84
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/utils/HelpUtils.java 11765c54df0df755b7f9b12b9d50986223d6b775
> geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java 126eb47e90c2390953a91136e8cfb36687baf2fd
> geode-core/src/main/java/org/apache/geode/management/internal/cli/multistep/CLIMultiStepHelper.java 0d215433c548db82b3c37b3e53be42cb0e1765fd
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/Argument.java 9acbc2a82c9cf4d4ccdb629798bdf70e1e48a305
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/AvailabilityTarget.java eff5fd2c2f67280ed9037216c1b15ab8b4a462d1
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/CommandTarget.java 3dfc01ab26d0ad6ee1867ab4014028f5cc0a3649
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/GfshMethodTarget.java 6f28830c314593004dfbb5eda113540d951fb343
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/GfshOptionParser.java a64933c2e9c5e3122a856d292b2ad70949e5017c
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/MethodParameter.java 599fb00d511de87d5a0b439371a9fec62377c027
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/Option.java 4eec1128c6e4646cde7b89e04f567fee2e419ee9
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/OptionSet.java 42270e53d047de566943a59c53969bdae8b32f54
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/Parameter.java dc371b3ba58b2ede0663e3167835926d17a3a418
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/ParserUtils.java 80f12867b7c0144ef9cb5aa5d5e7cc4f5c36b9f1
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/SyntaxConstants.java 52d92128b96e119f3157e5431d579e90218cbaad
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/jopt/JoptOptionParser.java 52224d40c9c47dd730432859033701e9b8faac4d
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/EnclosingCharacters.java a35e6269bd5ba2644fd2fbeb90c6fd8a92a8b971
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/Preprocessor.java 0dd875ad95ae30a6f2ca962da683e9e9ad0b9559
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/PreprocessorUtils.java a1872c92568a65c11f52603575a7814f2eb4c3f9
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/Stack.java ae47723f1ef2ff9af2e92c9991d02205fa044bac
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/TrimmedInput.java 8740f003f5a90a6ac296317ae4efee799e713baf
> geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java c346eaf77087102f51952a567ecd7ec036593a13
> geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java bcf1b415e9ee85822c470ae6888920f0a90159fd
> geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java d74f5d6b4fce9a44d9eeebdfc7dcf716e9b1652b
> geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/MultiCommandHelper.java 89f93d5a4f491addc955eeaaa11f0318e00f35de
> geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/jline/GfshHistory.java dc4a42fc014c73df16f057155229a51bd309efa3
> geode-core/src/main/java/org/apache/geode/management/internal/cli/util/CommandStringBuilder.java 16efda5f772e50b3dea4899733760b0a25513a0e
> geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DataCommandsController.java cfe60899503193d4079cff4798422cd4da00cd74
> geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java 527e059d1dfbe03778141ec0470d7340343f9e56
> geode-core/src/test/java/org/apache/geode/management/internal/cli/CommandManagerJUnitTest.java 503ffb2929758d617e8539e6aefb3f7b545de9b6
> geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserIntegrationTest.java f3e3bd8754e18d7a75faf4b75e3ba75b778fc9d7
> geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserJUnitTest.java 44e99f461b9efc5a439e629751011a6d0edd9213
> geode-core/src/test/java/org/apache/geode/management/internal/cli/JoptOptionParserTest.java 9815a9cbadae77ea94d49288922f204acf308d61
> geode-core/src/test/java/org/apache/geode/management/internal/cli/annotations/CliArgumentJUnitTest.java 161f7c65e62f11aa7b25fb31cd94ad256ea497a6
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CliCommandTestBase.java 165f66482758dadb75ae95d23443973e9de05891
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommandsDUnitTest.java 5f885e136e09541a6d095516fc7cb7db88f659aa
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DeployCommandsDUnitTest.java 9ed5bed8faa6bfceb0849347cd94b7c8effd3191
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DiskStoreCommandsDUnitTest.java 5b07f282b83d1539df6a7ea7bf19888436738723
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/GemfireDataCommandsDUnitTest.java e99a7fbc859243579b75a76742e23fec9b738ef5
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/HelpCommandsIntegrationTest.java b91a1f35efaa00f29a0e858a90e55c1c39c163f9
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/QueueCommandsDUnitTest.java 42a0624546742491e0c8c4d8c406ae8e567c1f85
> geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelpBlockUnitTest.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelperUnitTest.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/management/internal/cli/parser/ParserUtilsJUnitTest.java 9b22e64d263d6cb8c4453e2eea42fa85e7f1f922
> geode-core/src/test/java/org/apache/geode/management/internal/cli/parser/preprocessor/PreprocessorJUnitTest.java 97325cb16b42a112680a793b6581a1fea0e8c621
> geode-core/src/test/java/org/apache/geode/management/internal/cli/parser/preprocessor/PreprocessorUtilsJUnitTest.java b56cff2cfb7027999a420ae91f1f983243f865f0
> geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategyJUnitTest.java 54c7cf7074435b48232b61916627eed69a201f09
> geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshHistoryJUnitTest.java 58453b7c70b552af30ca4e16cd624475a89e3426
> geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshJunitTest.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigDistributionDUnitTest.java abbc5c0c7e83d259cd13bae61c419f8cf07db1f0
> geode-core/src/test/java/org/apache/geode/management/internal/security/TestCommand.java ffe6a28163acf20d13b62347c40d5cceea22c629
> geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/cli/LuceneIndexCommands.java 4bfa86842fd81873c132e3bbd7b529ff3e64dbc7
> geode-lucene/src/test/java/org/apache/geode/cache/lucene/LuceneCommandsSecurityDUnitTest.java ad734e8436822112c1eb8f5c6c0be676884dd009
> geode-lucene/src/test/java/org/apache/geode/cache/lucene/internal/cli/LuceneIndexCommandsDUnitTest.java 7f203cee7899340e19d589a4618be4ea7632faac
> geode-lucene/src/test/java/org/apache/geode/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java 5c1832575ec5c5cccdce670bd9b3477708f79148
>
>
> Diff: https://reviews.apache.org/r/58589/diff/3/
>
>
> Testing
> -------
>
> precheckin passing except one unittest that always fails in concourse but succeed in local.
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 58589: GEODE-1597: use Spring shell's parser and
delete our own parsing code
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58589/#review173100
-----------------------------------------------------------
Ship it!
Ship It!
- Kirk Lund
On April 26, 2017, 7:48 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58589/
> -----------------------------------------------------------
>
> (Updated April 26, 2017, 7:48 p.m.)
>
>
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-1597: use Spring shell's parser and delete our own parsing code
>
> * deleted usage of jopsimple and our own parser code
> * reworked help/hint
>
> The biggest change is in GfshParser, instead of directly implement SpringShell's Parser, now it's just a simple wrapper around SpringShell's SimpleParser so that we can use spring's parsing code. The challenge is that SimpleParser expects the user inupt to be in the format of "command option1 value1 option2 value2", while gfsh expects the format to be like "command option1=value1 option2=value2", so most of the code in GfshParser is to turn the input into the SimpleParser input format and then feed it into SimpleParser to get the validation and autocompletion we needed.
>
> So far the difference I noticed with this new implementation are:
> 1) option validation is what we gain from directly using SpringShell.
> 2) SpringShell doesn't allow you to specify an option twice, so for multivalued parameters, our old parser can accept command like "change loglevel --member=member1 --member=member2", but now the parser will give you an error, and you should only do "change loglevel --member=member1,member2". The new parser did something speical for --J option though, so for --J, you can specify it multiple times.
> 3) For value separator, springShell default's to ",", you can only overwrite it with option context "splittingRegex", we do not honor the @CliMetaData(valueSepartor=) anymore. All of our commands uses "," for separators, so this won't break our commands, but if there is any command out there that would define a different @CliMetaData(valueSepartor=) other than ",", SpringShell would not know how to parse it.
> 4) To specify empty string as parameter value, before you will need to to (put region=A key="''" value="''"), spring shell would think the value you are trying to pass in are two single quotes instead of empty strings, now, you should only do (put region=A key="" value="")
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java 44004454ef1646bfeca8a7af3cffb109fd83e7d7
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java a1d03e45dd4d6559bd9a0869c7dd95908d1858ca
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/GfshHelpCommands.java 1d1b28e568a1e175690fea5cde1a45a318b6db5d
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShellCommands.java b37feabe6ed61c081e9653c94128f092ad189d10
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/Helper.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java c346eaf77087102f51952a567ecd7ec036593a13
> geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java bcf1b415e9ee85822c470ae6888920f0a90159fd
> geode-core/src/test/java/org/apache/geode/management/internal/cli/CommandManagerJUnitTest.java 503ffb2929758d617e8539e6aefb3f7b545de9b6
> geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserIntegrationTest.java f3e3bd8754e18d7a75faf4b75e3ba75b778fc9d7
> geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserJUnitTest.java 44e99f461b9efc5a439e629751011a6d0edd9213
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CliCommandTestBase.java 165f66482758dadb75ae95d23443973e9de05891
> geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelpBlockUnitTest.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelperUnitTest.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategyJUnitTest.java 54c7cf7074435b48232b61916627eed69a201f09
>
>
> Diff: https://reviews.apache.org/r/58589/diff/4/
>
>
> Testing
> -------
>
> precheckin successful with the latest changes.
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 58589: GEODE-1597: use Spring shell's parser and
delete our own parsing code
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58589/
-----------------------------------------------------------
(Updated April 26, 2017, 7:48 p.m.)
Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
Repository: geode
Description
-------
GEODE-1597: use Spring shell's parser and delete our own parsing code
* deleted usage of jopsimple and our own parser code
* reworked help/hint
The biggest change is in GfshParser, instead of directly implement SpringShell's Parser, now it's just a simple wrapper around SpringShell's SimpleParser so that we can use spring's parsing code. The challenge is that SimpleParser expects the user inupt to be in the format of "command option1 value1 option2 value2", while gfsh expects the format to be like "command option1=value1 option2=value2", so most of the code in GfshParser is to turn the input into the SimpleParser input format and then feed it into SimpleParser to get the validation and autocompletion we needed.
So far the difference I noticed with this new implementation are:
1) option validation is what we gain from directly using SpringShell.
2) SpringShell doesn't allow you to specify an option twice, so for multivalued parameters, our old parser can accept command like "change loglevel --member=member1 --member=member2", but now the parser will give you an error, and you should only do "change loglevel --member=member1,member2". The new parser did something speical for --J option though, so for --J, you can specify it multiple times.
3) For value separator, springShell default's to ",", you can only overwrite it with option context "splittingRegex", we do not honor the @CliMetaData(valueSepartor=) anymore. All of our commands uses "," for separators, so this won't break our commands, but if there is any command out there that would define a different @CliMetaData(valueSepartor=) other than ",", SpringShell would not know how to parse it.
4) To specify empty string as parameter value, before you will need to to (put region=A key="''" value="''"), spring shell would think the value you are trying to pass in are two single quotes instead of empty strings, now, you should only do (put region=A key="" value="")
Diffs
-----
geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java 44004454ef1646bfeca8a7af3cffb109fd83e7d7
geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java a1d03e45dd4d6559bd9a0869c7dd95908d1858ca
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/GfshHelpCommands.java 1d1b28e568a1e175690fea5cde1a45a318b6db5d
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShellCommands.java b37feabe6ed61c081e9653c94128f092ad189d10
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/Helper.java PRE-CREATION
geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java c346eaf77087102f51952a567ecd7ec036593a13
geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java bcf1b415e9ee85822c470ae6888920f0a90159fd
geode-core/src/test/java/org/apache/geode/management/internal/cli/CommandManagerJUnitTest.java 503ffb2929758d617e8539e6aefb3f7b545de9b6
geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserIntegrationTest.java f3e3bd8754e18d7a75faf4b75e3ba75b778fc9d7
geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserJUnitTest.java 44e99f461b9efc5a439e629751011a6d0edd9213
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CliCommandTestBase.java 165f66482758dadb75ae95d23443973e9de05891
geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelpBlockUnitTest.java PRE-CREATION
geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelperUnitTest.java PRE-CREATION
geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategyJUnitTest.java 54c7cf7074435b48232b61916627eed69a201f09
Diff: https://reviews.apache.org/r/58589/diff/4/
Testing (updated)
-------
precheckin successful with the latest changes.
Thanks,
Jinmei Liao
Re: Review Request 58589: GEODE-1597: use Spring shell's parser and
delete our own parsing code
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58589/
-----------------------------------------------------------
(Updated April 25, 2017, 9:19 p.m.)
Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
Changes
-------
changes after the reivew: minor fixes, and removing singletons. Precheckin running.
Repository: geode
Description
-------
GEODE-1597: use Spring shell's parser and delete our own parsing code
* deleted usage of jopsimple and our own parser code
* reworked help/hint
The biggest change is in GfshParser, instead of directly implement SpringShell's Parser, now it's just a simple wrapper around SpringShell's SimpleParser so that we can use spring's parsing code. The challenge is that SimpleParser expects the user inupt to be in the format of "command option1 value1 option2 value2", while gfsh expects the format to be like "command option1=value1 option2=value2", so most of the code in GfshParser is to turn the input into the SimpleParser input format and then feed it into SimpleParser to get the validation and autocompletion we needed.
So far the difference I noticed with this new implementation are:
1) option validation is what we gain from directly using SpringShell.
2) SpringShell doesn't allow you to specify an option twice, so for multivalued parameters, our old parser can accept command like "change loglevel --member=member1 --member=member2", but now the parser will give you an error, and you should only do "change loglevel --member=member1,member2". The new parser did something speical for --J option though, so for --J, you can specify it multiple times.
3) For value separator, springShell default's to ",", you can only overwrite it with option context "splittingRegex", we do not honor the @CliMetaData(valueSepartor=) anymore. All of our commands uses "," for separators, so this won't break our commands, but if there is any command out there that would define a different @CliMetaData(valueSepartor=) other than ",", SpringShell would not know how to parse it.
4) To specify empty string as parameter value, before you will need to to (put region=A key="''" value="''"), spring shell would think the value you are trying to pass in are two single quotes instead of empty strings, now, you should only do (put region=A key="" value="")
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java 44004454ef1646bfeca8a7af3cffb109fd83e7d7
geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java a1d03e45dd4d6559bd9a0869c7dd95908d1858ca
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/GfshHelpCommands.java 1d1b28e568a1e175690fea5cde1a45a318b6db5d
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShellCommands.java b37feabe6ed61c081e9653c94128f092ad189d10
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/Helper.java PRE-CREATION
geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java c346eaf77087102f51952a567ecd7ec036593a13
geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java bcf1b415e9ee85822c470ae6888920f0a90159fd
geode-core/src/test/java/org/apache/geode/management/internal/cli/CommandManagerJUnitTest.java 503ffb2929758d617e8539e6aefb3f7b545de9b6
geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserIntegrationTest.java f3e3bd8754e18d7a75faf4b75e3ba75b778fc9d7
geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserJUnitTest.java 44e99f461b9efc5a439e629751011a6d0edd9213
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CliCommandTestBase.java 165f66482758dadb75ae95d23443973e9de05891
geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelpBlockUnitTest.java PRE-CREATION
geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelperUnitTest.java PRE-CREATION
geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategyJUnitTest.java 54c7cf7074435b48232b61916627eed69a201f09
Diff: https://reviews.apache.org/r/58589/diff/4/
Changes: https://reviews.apache.org/r/58589/diff/3-4/
Testing
-------
precheckin passing except one unittest that always fails in concourse but succeed in local.
Thanks,
Jinmei Liao
Re: Review Request 58589: GEODE-1597: use Spring shell's parser and
delete our own parsing code
Posted by Jinmei Liao <ji...@pivotal.io>.
> On April 25, 2017, 5:43 p.m., Kirk Lund wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShellCommands.java
> > Lines 238 (patched)
> > <https://reviews.apache.org/r/58589/diff/3/?file=1698725#file1698725line239>
> >
> > Insert one line above the throw to restore the interrupt bit on the current thread:
> > ```java
> > Thread.currentThread().interrupt();
> > ```
You mean like this?
.....
} catch (final InterruptedException e) {
Thread.currentThread().interrupt();
throw new IllegalStateException(e.getMessage(), e);
}
What would this do?
> On April 25, 2017, 5:43 p.m., Kirk Lund wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java
> > Line 229 (original), 214 (patched)
> > <https://reviews.apache.org/r/58589/diff/3/?file=1698779#file1698779line242>
> >
> > Can we have either Gfsh or CommandProcessor hold an instance to GfshParser instead of both?
> >
> > If we get rid of the singleton, then we don't want both classes constructing an instance of GfshParser. One class could delegate to the other to getGfshParser or even have just one class use GfshParser.
Gfsh's GfshParser is in the Gfsh client VM, used for parsing the command and command completions.
CommandProcess's GfshParser is in the locator's VM, used for parsing the command for server side execution. Each will hold one instance of GfshParser.
> On April 25, 2017, 5:43 p.m., Kirk Lund wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/CommandStringBuilder.java
> > Line 45 (original), 44 (patched)
> > <https://reviews.apache.org/r/58589/diff/3/?file=1698783#file1698783line45>
> >
> > It's probably better to just use the line.separator system property and skip usage of SystemUtils:
> > ```java
> > return System.getProperty("line.separator");
> > ```
> > Maybe even move getLineSeparator() method to SystemUtils.
I think this is due to a bug of some sort, it has to be done this way. There is a comment before this line.
- Jinmei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58589/#review172950
-----------------------------------------------------------
On April 25, 2017, 1:08 a.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58589/
> -----------------------------------------------------------
>
> (Updated April 25, 2017, 1:08 a.m.)
>
>
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-1597: use Spring shell's parser and delete our own parsing code
>
> * deleted usage of jopsimple and our own parser code
> * reworked help/hint
>
> The biggest change is in GfshParser, instead of directly implement SpringShell's Parser, now it's just a simple wrapper around SpringShell's SimpleParser so that we can use spring's parsing code. The challenge is that SimpleParser expects the user inupt to be in the format of "command option1 value1 option2 value2", while gfsh expects the format to be like "command option1=value1 option2=value2", so most of the code in GfshParser is to turn the input into the SimpleParser input format and then feed it into SimpleParser to get the validation and autocompletion we needed.
>
> So far the difference I noticed with this new implementation are:
> 1) option validation is what we gain from directly using SpringShell.
> 2) SpringShell doesn't allow you to specify an option twice, so for multivalued parameters, our old parser can accept command like "change loglevel --member=member1 --member=member2", but now the parser will give you an error, and you should only do "change loglevel --member=member1,member2". The new parser did something speical for --J option though, so for --J, you can specify it multiple times.
> 3) For value separator, springShell default's to ",", you can only overwrite it with option context "splittingRegex", we do not honor the @CliMetaData(valueSepartor=) anymore. All of our commands uses "," for separators, so this won't break our commands, but if there is any command out there that would define a different @CliMetaData(valueSepartor=) other than ",", SpringShell would not know how to parse it.
> 4) To specify empty string as parameter value, before you will need to to (put region=A key="''" value="''"), spring shell would think the value you are trying to pass in are two single quotes instead of empty strings, now, you should only do (put region=A key="" value="")
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/management/cli/CliMetaData.java e69d78a2b42b4a7f21066712b9f763dc1cdb92c8
> geode-core/src/main/java/org/apache/geode/management/cli/ConverterHint.java f45abc432dce8af67b71ca29fdfb85e0e9f6f87a
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java 44004454ef1646bfeca8a7af3cffb109fd83e7d7
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandResponseBuilder.java bda030de50cca303e2aa6d3d82b17a9111fd25ef
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParseResult.java d879e2d6c253eb3306fb7558fa0a2b7fbe7d1e40
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java a1d03e45dd4d6559bd9a0869c7dd95908d1858ca
> geode-core/src/main/java/org/apache/geode/management/internal/cli/Launcher.java fc0427e20144a9092d97d05fc3ccbd0e21d5b35e
> geode-core/src/main/java/org/apache/geode/management/internal/cli/annotation/CliArgument.java e20e73112c3b6545771c62e58795fdc53fc279c5
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ClientCommands.java 39d0442f2cb4ec4276652169a0cfa962a78af2c9
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ConfigCommands.java 50c9caaee88426f9a8c49d8b1abca9c2fbc66163
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommands.java 28ce0921e0cba412063d3a59566165544dfda3dc
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java 6324b5cfc70c0f02233f73ceab6dc56cb253cbb2
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DeployCommands.java 144408854daf2cd5b33dcdcf995efef77b3e942e
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DiskStoreCommands.java 9ad206043a284144293175baa7144bb835652a46
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/FunctionCommands.java 752ca2a612eb59c137de568f80223d58339e7231
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/GfshHelpCommands.java 1d1b28e568a1e175690fea5cde1a45a318b6db5d
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java 313d1bdfb4d1e610f2353db2a4496449453a0fbe
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommands.java 22981e72f4971f3bb297a53e9ac4d6f8ac7e149e
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/PDXCommands.java 4327decc98e3d0fe637d73857c9eed83f7bc88b9
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueueCommands.java 8c568336aa62c6b6cbf4e1a29584d514e84f1c96
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShellCommands.java b37feabe6ed61c081e9653c94128f092ad189d10
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/WanCommands.java 239db48417cdae0d415830ac411cb611b8433919
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/BooleanConverter.java 5e9cdb9a642e9cda4934e7934c2fc022e7d9eabe
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/DirConverter.java c97057bb281e64c96983e38172bcf7953f492c83
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/EnumConverter.java 354cef9708623d9141caea08fe497deaa75f606f
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/HelpConverter.java e670274f89ce4423a77715dcee07f119c1568929
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/HintTopicConverter.java b6f9f81af10a7773df27f77ca5707238e7ef3a3b
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/LogLevelConverter.java 3a262405da0d9821a1c9729ca0aad21ccf626a02
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/StringArrayConverter.java eacf1810ba53fc0c389cff0d5ae30ba5fea6b0c3
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/StringListConverter.java eab096b9a5c93fd71f9a0b757d80a2d726690ad1
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandException.java 7e5cba05d93a88fa722d98babfdabf89e4e07be0
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandInvalidException.java a14005923ee3343d2e2e8e1818b8fbac16410ca6
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandMultiModeOptionException.java acbc49618504bec9f8eb6f04124e02a47e0a92c3
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandNotAvailableException.java c471df298f10211a812ae0e644c88f81c68b8132
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionException.java a7e56be98afe94690cdc1b61ea5476a72fbd9140
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionHasMultipleValuesException.java 4b365e3f9d611e74f345f50f87c682f272d20ba4
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionInvalidException.java 1db8906024934a024febf447f8d5facc5f1b2ee8
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionMissingException.java f263dce0be966a361b8b4e84ed8360cc1a659e3b
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionNotApplicableException.java 98147787eff00d74ed5e32d2c684867fd1bf605a
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionValueConversionException.java 7dbf869eab0c8331c217aed6eb82b57ea7696be6
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionValueException.java ee02df8423ef48f02ed482ad3e40260799b2e7d7
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionValueMissingException.java 023a8789fe4a0ca2fc11bce7482d0e9ed4aa1b2a
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/ExceptionGenerator.java fda31357a39830b7414b4751966dba2e57526b13
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/ExceptionHandler.java 95afbafe55746bb0159e5067ae4d5aa9c0701b56
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/CliTopic.java 791cdca5ce954153ed5013f3108d738d970b32dd
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/HelpBlock.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/Helper.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/Block.java dde1a20ef0bfa98a7b5dcf57c1775f1769ab3c97
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/DataNode.java 8f5e570cc9761ddedcb7fb16b0a9615f2da87209
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/Help.java 68c10bb211c319bed679e39718660d1a466967a9
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/NewHelp.java 90f9eda49326fa008cc5c1abb5c02f9dcfd94fd6
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/Row.java ac1ca218668aff03d3a774ae2e1ebaebe8215b3c
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/utils/FormatOutput.java 44998a03bed5203200115ce1b2023a436a52fd84
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/utils/HelpUtils.java 11765c54df0df755b7f9b12b9d50986223d6b775
> geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java 126eb47e90c2390953a91136e8cfb36687baf2fd
> geode-core/src/main/java/org/apache/geode/management/internal/cli/multistep/CLIMultiStepHelper.java 0d215433c548db82b3c37b3e53be42cb0e1765fd
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/Argument.java 9acbc2a82c9cf4d4ccdb629798bdf70e1e48a305
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/AvailabilityTarget.java eff5fd2c2f67280ed9037216c1b15ab8b4a462d1
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/CommandTarget.java 3dfc01ab26d0ad6ee1867ab4014028f5cc0a3649
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/GfshMethodTarget.java 6f28830c314593004dfbb5eda113540d951fb343
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/GfshOptionParser.java a64933c2e9c5e3122a856d292b2ad70949e5017c
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/MethodParameter.java 599fb00d511de87d5a0b439371a9fec62377c027
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/Option.java 4eec1128c6e4646cde7b89e04f567fee2e419ee9
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/OptionSet.java 42270e53d047de566943a59c53969bdae8b32f54
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/Parameter.java dc371b3ba58b2ede0663e3167835926d17a3a418
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/ParserUtils.java 80f12867b7c0144ef9cb5aa5d5e7cc4f5c36b9f1
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/SyntaxConstants.java 52d92128b96e119f3157e5431d579e90218cbaad
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/jopt/JoptOptionParser.java 52224d40c9c47dd730432859033701e9b8faac4d
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/EnclosingCharacters.java a35e6269bd5ba2644fd2fbeb90c6fd8a92a8b971
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/Preprocessor.java 0dd875ad95ae30a6f2ca962da683e9e9ad0b9559
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/PreprocessorUtils.java a1872c92568a65c11f52603575a7814f2eb4c3f9
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/Stack.java ae47723f1ef2ff9af2e92c9991d02205fa044bac
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/TrimmedInput.java 8740f003f5a90a6ac296317ae4efee799e713baf
> geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java c346eaf77087102f51952a567ecd7ec036593a13
> geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java bcf1b415e9ee85822c470ae6888920f0a90159fd
> geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java d74f5d6b4fce9a44d9eeebdfc7dcf716e9b1652b
> geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/MultiCommandHelper.java 89f93d5a4f491addc955eeaaa11f0318e00f35de
> geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/jline/GfshHistory.java dc4a42fc014c73df16f057155229a51bd309efa3
> geode-core/src/main/java/org/apache/geode/management/internal/cli/util/CommandStringBuilder.java 16efda5f772e50b3dea4899733760b0a25513a0e
> geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DataCommandsController.java cfe60899503193d4079cff4798422cd4da00cd74
> geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java 527e059d1dfbe03778141ec0470d7340343f9e56
> geode-core/src/test/java/org/apache/geode/management/internal/cli/CommandManagerJUnitTest.java 503ffb2929758d617e8539e6aefb3f7b545de9b6
> geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserIntegrationTest.java f3e3bd8754e18d7a75faf4b75e3ba75b778fc9d7
> geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserJUnitTest.java 44e99f461b9efc5a439e629751011a6d0edd9213
> geode-core/src/test/java/org/apache/geode/management/internal/cli/JoptOptionParserTest.java 9815a9cbadae77ea94d49288922f204acf308d61
> geode-core/src/test/java/org/apache/geode/management/internal/cli/annotations/CliArgumentJUnitTest.java 161f7c65e62f11aa7b25fb31cd94ad256ea497a6
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CliCommandTestBase.java 165f66482758dadb75ae95d23443973e9de05891
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommandsDUnitTest.java 5f885e136e09541a6d095516fc7cb7db88f659aa
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DeployCommandsDUnitTest.java 9ed5bed8faa6bfceb0849347cd94b7c8effd3191
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DiskStoreCommandsDUnitTest.java 5b07f282b83d1539df6a7ea7bf19888436738723
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/GemfireDataCommandsDUnitTest.java e99a7fbc859243579b75a76742e23fec9b738ef5
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/HelpCommandsIntegrationTest.java b91a1f35efaa00f29a0e858a90e55c1c39c163f9
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/QueueCommandsDUnitTest.java 42a0624546742491e0c8c4d8c406ae8e567c1f85
> geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelpBlockUnitTest.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelperUnitTest.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/management/internal/cli/parser/ParserUtilsJUnitTest.java 9b22e64d263d6cb8c4453e2eea42fa85e7f1f922
> geode-core/src/test/java/org/apache/geode/management/internal/cli/parser/preprocessor/PreprocessorJUnitTest.java 97325cb16b42a112680a793b6581a1fea0e8c621
> geode-core/src/test/java/org/apache/geode/management/internal/cli/parser/preprocessor/PreprocessorUtilsJUnitTest.java b56cff2cfb7027999a420ae91f1f983243f865f0
> geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategyJUnitTest.java 54c7cf7074435b48232b61916627eed69a201f09
> geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshHistoryJUnitTest.java 58453b7c70b552af30ca4e16cd624475a89e3426
> geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshJunitTest.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigDistributionDUnitTest.java abbc5c0c7e83d259cd13bae61c419f8cf07db1f0
> geode-core/src/test/java/org/apache/geode/management/internal/security/TestCommand.java ffe6a28163acf20d13b62347c40d5cceea22c629
> geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/cli/LuceneIndexCommands.java 4bfa86842fd81873c132e3bbd7b529ff3e64dbc7
> geode-lucene/src/test/java/org/apache/geode/cache/lucene/LuceneCommandsSecurityDUnitTest.java ad734e8436822112c1eb8f5c6c0be676884dd009
> geode-lucene/src/test/java/org/apache/geode/cache/lucene/internal/cli/LuceneIndexCommandsDUnitTest.java 7f203cee7899340e19d589a4618be4ea7632faac
> geode-lucene/src/test/java/org/apache/geode/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java 5c1832575ec5c5cccdce670bd9b3477708f79148
>
>
> Diff: https://reviews.apache.org/r/58589/diff/3/
>
>
> Testing
> -------
>
> precheckin passing except one unittest that always fails in concourse but succeed in local.
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 58589: GEODE-1597: use Spring shell's parser and
delete our own parsing code
Posted by Kirk Lund <ki...@gmail.com>.
> On April 25, 2017, 5:43 p.m., Kirk Lund wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShellCommands.java
> > Lines 238 (patched)
> > <https://reviews.apache.org/r/58589/diff/3/?file=1698725#file1698725line239>
> >
> > Insert one line above the throw to restore the interrupt bit on the current thread:
> > ```java
> > Thread.currentThread().interrupt();
> > ```
>
> Jinmei Liao wrote:
> You mean like this?
> .....
> } catch (final InterruptedException e) {
> Thread.currentThread().interrupt();
> throw new IllegalStateException(e.getMessage(), e);
> }
>
> What would this do?
Yep, that's correct. That just resets the interrupt bit. Anytime you catch InterruptedException you're supposed to reset the interrupt bit like that. I can probably find some more info online. If you own the Thread and the Thread is going to completely terminate (exit its run() method) then you don't need to reset the bit.
If the bit is reset, then the next time that Thread invokes a call that checks the interrupt bit, the Thread will once again throw InterruptedException. This should bring up more examples in Geode:
git grep "Thread.currentThread().interrupt()"
- Kirk
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58589/#review172950
-----------------------------------------------------------
On April 26, 2017, 7:48 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58589/
> -----------------------------------------------------------
>
> (Updated April 26, 2017, 7:48 p.m.)
>
>
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-1597: use Spring shell's parser and delete our own parsing code
>
> * deleted usage of jopsimple and our own parser code
> * reworked help/hint
>
> The biggest change is in GfshParser, instead of directly implement SpringShell's Parser, now it's just a simple wrapper around SpringShell's SimpleParser so that we can use spring's parsing code. The challenge is that SimpleParser expects the user inupt to be in the format of "command option1 value1 option2 value2", while gfsh expects the format to be like "command option1=value1 option2=value2", so most of the code in GfshParser is to turn the input into the SimpleParser input format and then feed it into SimpleParser to get the validation and autocompletion we needed.
>
> So far the difference I noticed with this new implementation are:
> 1) option validation is what we gain from directly using SpringShell.
> 2) SpringShell doesn't allow you to specify an option twice, so for multivalued parameters, our old parser can accept command like "change loglevel --member=member1 --member=member2", but now the parser will give you an error, and you should only do "change loglevel --member=member1,member2". The new parser did something speical for --J option though, so for --J, you can specify it multiple times.
> 3) For value separator, springShell default's to ",", you can only overwrite it with option context "splittingRegex", we do not honor the @CliMetaData(valueSepartor=) anymore. All of our commands uses "," for separators, so this won't break our commands, but if there is any command out there that would define a different @CliMetaData(valueSepartor=) other than ",", SpringShell would not know how to parse it.
> 4) To specify empty string as parameter value, before you will need to to (put region=A key="''" value="''"), spring shell would think the value you are trying to pass in are two single quotes instead of empty strings, now, you should only do (put region=A key="" value="")
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java 44004454ef1646bfeca8a7af3cffb109fd83e7d7
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java a1d03e45dd4d6559bd9a0869c7dd95908d1858ca
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/GfshHelpCommands.java 1d1b28e568a1e175690fea5cde1a45a318b6db5d
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShellCommands.java b37feabe6ed61c081e9653c94128f092ad189d10
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/Helper.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java c346eaf77087102f51952a567ecd7ec036593a13
> geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java bcf1b415e9ee85822c470ae6888920f0a90159fd
> geode-core/src/test/java/org/apache/geode/management/internal/cli/CommandManagerJUnitTest.java 503ffb2929758d617e8539e6aefb3f7b545de9b6
> geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserIntegrationTest.java f3e3bd8754e18d7a75faf4b75e3ba75b778fc9d7
> geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserJUnitTest.java 44e99f461b9efc5a439e629751011a6d0edd9213
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CliCommandTestBase.java 165f66482758dadb75ae95d23443973e9de05891
> geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelpBlockUnitTest.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelperUnitTest.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategyJUnitTest.java 54c7cf7074435b48232b61916627eed69a201f09
>
>
> Diff: https://reviews.apache.org/r/58589/diff/4/
>
>
> Testing
> -------
>
> precheckin successful with the latest changes.
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 58589: GEODE-1597: use Spring shell's parser and
delete our own parsing code
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58589/#review172950
-----------------------------------------------------------
Huge changeset! I'm excited to get this merged into develop. All of my review comments are minor one-liners except the singleton. Overall I'd like to avoid adding more singletons. If Gfsh has to be a singleton, then I'd like to see it be the only singleton in Gfsh -- it could instantiate and own all of the other collaborators for which we only want one instance in a JVM.
geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java
Lines 84 (patched)
<https://reviews.apache.org/r/58589/#comment245955>
Delete empty javadocs
geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java
Lines 96 (patched)
<https://reviews.apache.org/r/58589/#comment245956>
Delete empty javadocs
geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java
Lines 221 (patched)
<https://reviews.apache.org/r/58589/#comment245957>
Delete dead code
geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java
Line 99 (original), 60 (patched)
<https://reviews.apache.org/r/58589/#comment245958>
Can CommandProcessor create its own GfshParser instance instead of introducing another singleton?
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShellCommands.java
Lines 238 (patched)
<https://reviews.apache.org/r/58589/#comment245959>
Insert one line above the throw to restore the interrupt bit on the current thread:
```java
Thread.currentThread().interrupt();
```
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShellCommands.java
Lines 817 (patched)
<https://reviews.apache.org/r/58589/#comment245960>
Delete dead code
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/Helper.java
Lines 231 (patched)
<https://reviews.apache.org/r/58589/#comment245961>
Should probably move these constants to the top of the class
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/Stack.java
Line 21 (original), 22 (patched)
<https://reviews.apache.org/r/58589/#comment245962>
Can these vars be private or are they used in an inner class? It would also be good to make them final
geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java
Line 62 (original), 59 (patched)
<https://reviews.apache.org/r/58589/#comment245963>
Here's where I'd love to see us get rid of the singleton:
```java
this.gfshParser = new GfshParser(cacheProperties);
```
geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java
Line 229 (original), 214 (patched)
<https://reviews.apache.org/r/58589/#comment245966>
Can we have either Gfsh or CommandProcessor hold an instance to GfshParser instead of both?
If we get rid of the singleton, then we don't want both classes constructing an instance of GfshParser. One class could delegate to the other to getGfshParser or even have just one class use GfshParser.
geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java
Line 472 (original), 635 (patched)
<https://reviews.apache.org/r/58589/#comment245967>
Delete dead code
geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java
Lines 988 (patched)
<https://reviews.apache.org/r/58589/#comment245968>
I would just remove all of these comment lines
geode-core/src/main/java/org/apache/geode/management/internal/cli/util/CommandStringBuilder.java
Line 45 (original), 44 (patched)
<https://reviews.apache.org/r/58589/#comment245969>
It's probably better to just use the line.separator system property and skip usage of SystemUtils:
```java
return System.getProperty("line.separator");
```
Maybe even move getLineSeparator() method to SystemUtils.
geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserJUnitTest.java
Line 60 (original), 35 (patched)
<https://reviews.apache.org/r/58589/#comment245970>
Can these vars be private?
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CliCommandTestBase.java
Line 65 (original), 66 (patched)
<https://reviews.apache.org/r/58589/#comment245971>
DistributedRestoreSystemProperties is Serializable so you can remove transient if you want to
geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelpBlockUnitTest.java
Lines 27 (patched)
<https://reviews.apache.org/r/58589/#comment245972>
Can these vars be private?
geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelperUnitTest.java
Lines 36 (patched)
<https://reviews.apache.org/r/58589/#comment245973>
More vars that should probably be private. One reason I keep bringing this up is that I've seen others twist multiple tests together, using vars and methods in each other. Some developers tend to see this as an invitation to "use it" either directly or as an example of what to do (which would be even worse if some develop did this in product code).
geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategyJUnitTest.java
Line 56 (original), 45 (patched)
<https://reviews.apache.org/r/58589/#comment245974>
Private?
geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategyJUnitTest.java
Line 87 (original), 69 (patched)
<https://reviews.apache.org/r/58589/#comment245975>
Wow, that's a long method name :) Remove the "j" from the middle?
- Kirk Lund
On April 25, 2017, 1:08 a.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58589/
> -----------------------------------------------------------
>
> (Updated April 25, 2017, 1:08 a.m.)
>
>
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-1597: use Spring shell's parser and delete our own parsing code
>
> * deleted usage of jopsimple and our own parser code
> * reworked help/hint
>
> The biggest change is in GfshParser, instead of directly implement SpringShell's Parser, now it's just a simple wrapper around SpringShell's SimpleParser so that we can use spring's parsing code. The challenge is that SimpleParser expects the user inupt to be in the format of "command option1 value1 option2 value2", while gfsh expects the format to be like "command option1=value1 option2=value2", so most of the code in GfshParser is to turn the input into the SimpleParser input format and then feed it into SimpleParser to get the validation and autocompletion we needed.
>
> So far the difference I noticed with this new implementation are:
> 1) option validation is what we gain from directly using SpringShell.
> 2) SpringShell doesn't allow you to specify an option twice, so for multivalued parameters, our old parser can accept command like "change loglevel --member=member1 --member=member2", but now the parser will give you an error, and you should only do "change loglevel --member=member1,member2". The new parser did something speical for --J option though, so for --J, you can specify it multiple times.
> 3) For value separator, springShell default's to ",", you can only overwrite it with option context "splittingRegex", we do not honor the @CliMetaData(valueSepartor=) anymore. All of our commands uses "," for separators, so this won't break our commands, but if there is any command out there that would define a different @CliMetaData(valueSepartor=) other than ",", SpringShell would not know how to parse it.
> 4) To specify empty string as parameter value, before you will need to to (put region=A key="''" value="''"), spring shell would think the value you are trying to pass in are two single quotes instead of empty strings, now, you should only do (put region=A key="" value="")
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/management/cli/CliMetaData.java e69d78a2b42b4a7f21066712b9f763dc1cdb92c8
> geode-core/src/main/java/org/apache/geode/management/cli/ConverterHint.java f45abc432dce8af67b71ca29fdfb85e0e9f6f87a
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java 44004454ef1646bfeca8a7af3cffb109fd83e7d7
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandResponseBuilder.java bda030de50cca303e2aa6d3d82b17a9111fd25ef
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParseResult.java d879e2d6c253eb3306fb7558fa0a2b7fbe7d1e40
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java a1d03e45dd4d6559bd9a0869c7dd95908d1858ca
> geode-core/src/main/java/org/apache/geode/management/internal/cli/Launcher.java fc0427e20144a9092d97d05fc3ccbd0e21d5b35e
> geode-core/src/main/java/org/apache/geode/management/internal/cli/annotation/CliArgument.java e20e73112c3b6545771c62e58795fdc53fc279c5
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ClientCommands.java 39d0442f2cb4ec4276652169a0cfa962a78af2c9
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ConfigCommands.java 50c9caaee88426f9a8c49d8b1abca9c2fbc66163
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommands.java 28ce0921e0cba412063d3a59566165544dfda3dc
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java 6324b5cfc70c0f02233f73ceab6dc56cb253cbb2
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DeployCommands.java 144408854daf2cd5b33dcdcf995efef77b3e942e
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DiskStoreCommands.java 9ad206043a284144293175baa7144bb835652a46
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/FunctionCommands.java 752ca2a612eb59c137de568f80223d58339e7231
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/GfshHelpCommands.java 1d1b28e568a1e175690fea5cde1a45a318b6db5d
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java 313d1bdfb4d1e610f2353db2a4496449453a0fbe
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommands.java 22981e72f4971f3bb297a53e9ac4d6f8ac7e149e
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/PDXCommands.java 4327decc98e3d0fe637d73857c9eed83f7bc88b9
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueueCommands.java 8c568336aa62c6b6cbf4e1a29584d514e84f1c96
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShellCommands.java b37feabe6ed61c081e9653c94128f092ad189d10
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/WanCommands.java 239db48417cdae0d415830ac411cb611b8433919
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/BooleanConverter.java 5e9cdb9a642e9cda4934e7934c2fc022e7d9eabe
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/DirConverter.java c97057bb281e64c96983e38172bcf7953f492c83
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/EnumConverter.java 354cef9708623d9141caea08fe497deaa75f606f
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/HelpConverter.java e670274f89ce4423a77715dcee07f119c1568929
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/HintTopicConverter.java b6f9f81af10a7773df27f77ca5707238e7ef3a3b
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/LogLevelConverter.java 3a262405da0d9821a1c9729ca0aad21ccf626a02
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/StringArrayConverter.java eacf1810ba53fc0c389cff0d5ae30ba5fea6b0c3
> geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/StringListConverter.java eab096b9a5c93fd71f9a0b757d80a2d726690ad1
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandException.java 7e5cba05d93a88fa722d98babfdabf89e4e07be0
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandInvalidException.java a14005923ee3343d2e2e8e1818b8fbac16410ca6
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandMultiModeOptionException.java acbc49618504bec9f8eb6f04124e02a47e0a92c3
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandNotAvailableException.java c471df298f10211a812ae0e644c88f81c68b8132
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionException.java a7e56be98afe94690cdc1b61ea5476a72fbd9140
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionHasMultipleValuesException.java 4b365e3f9d611e74f345f50f87c682f272d20ba4
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionInvalidException.java 1db8906024934a024febf447f8d5facc5f1b2ee8
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionMissingException.java f263dce0be966a361b8b4e84ed8360cc1a659e3b
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionNotApplicableException.java 98147787eff00d74ed5e32d2c684867fd1bf605a
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionValueConversionException.java 7dbf869eab0c8331c217aed6eb82b57ea7696be6
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionValueException.java ee02df8423ef48f02ed482ad3e40260799b2e7d7
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionValueMissingException.java 023a8789fe4a0ca2fc11bce7482d0e9ed4aa1b2a
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/ExceptionGenerator.java fda31357a39830b7414b4751966dba2e57526b13
> geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/ExceptionHandler.java 95afbafe55746bb0159e5067ae4d5aa9c0701b56
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/CliTopic.java 791cdca5ce954153ed5013f3108d738d970b32dd
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/HelpBlock.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/Helper.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/Block.java dde1a20ef0bfa98a7b5dcf57c1775f1769ab3c97
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/DataNode.java 8f5e570cc9761ddedcb7fb16b0a9615f2da87209
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/Help.java 68c10bb211c319bed679e39718660d1a466967a9
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/NewHelp.java 90f9eda49326fa008cc5c1abb5c02f9dcfd94fd6
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/Row.java ac1ca218668aff03d3a774ae2e1ebaebe8215b3c
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/utils/FormatOutput.java 44998a03bed5203200115ce1b2023a436a52fd84
> geode-core/src/main/java/org/apache/geode/management/internal/cli/help/utils/HelpUtils.java 11765c54df0df755b7f9b12b9d50986223d6b775
> geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java 126eb47e90c2390953a91136e8cfb36687baf2fd
> geode-core/src/main/java/org/apache/geode/management/internal/cli/multistep/CLIMultiStepHelper.java 0d215433c548db82b3c37b3e53be42cb0e1765fd
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/Argument.java 9acbc2a82c9cf4d4ccdb629798bdf70e1e48a305
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/AvailabilityTarget.java eff5fd2c2f67280ed9037216c1b15ab8b4a462d1
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/CommandTarget.java 3dfc01ab26d0ad6ee1867ab4014028f5cc0a3649
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/GfshMethodTarget.java 6f28830c314593004dfbb5eda113540d951fb343
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/GfshOptionParser.java a64933c2e9c5e3122a856d292b2ad70949e5017c
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/MethodParameter.java 599fb00d511de87d5a0b439371a9fec62377c027
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/Option.java 4eec1128c6e4646cde7b89e04f567fee2e419ee9
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/OptionSet.java 42270e53d047de566943a59c53969bdae8b32f54
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/Parameter.java dc371b3ba58b2ede0663e3167835926d17a3a418
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/ParserUtils.java 80f12867b7c0144ef9cb5aa5d5e7cc4f5c36b9f1
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/SyntaxConstants.java 52d92128b96e119f3157e5431d579e90218cbaad
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/jopt/JoptOptionParser.java 52224d40c9c47dd730432859033701e9b8faac4d
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/EnclosingCharacters.java a35e6269bd5ba2644fd2fbeb90c6fd8a92a8b971
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/Preprocessor.java 0dd875ad95ae30a6f2ca962da683e9e9ad0b9559
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/PreprocessorUtils.java a1872c92568a65c11f52603575a7814f2eb4c3f9
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/Stack.java ae47723f1ef2ff9af2e92c9991d02205fa044bac
> geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/TrimmedInput.java 8740f003f5a90a6ac296317ae4efee799e713baf
> geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java c346eaf77087102f51952a567ecd7ec036593a13
> geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java bcf1b415e9ee85822c470ae6888920f0a90159fd
> geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java d74f5d6b4fce9a44d9eeebdfc7dcf716e9b1652b
> geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/MultiCommandHelper.java 89f93d5a4f491addc955eeaaa11f0318e00f35de
> geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/jline/GfshHistory.java dc4a42fc014c73df16f057155229a51bd309efa3
> geode-core/src/main/java/org/apache/geode/management/internal/cli/util/CommandStringBuilder.java 16efda5f772e50b3dea4899733760b0a25513a0e
> geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DataCommandsController.java cfe60899503193d4079cff4798422cd4da00cd74
> geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java 527e059d1dfbe03778141ec0470d7340343f9e56
> geode-core/src/test/java/org/apache/geode/management/internal/cli/CommandManagerJUnitTest.java 503ffb2929758d617e8539e6aefb3f7b545de9b6
> geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserIntegrationTest.java f3e3bd8754e18d7a75faf4b75e3ba75b778fc9d7
> geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserJUnitTest.java 44e99f461b9efc5a439e629751011a6d0edd9213
> geode-core/src/test/java/org/apache/geode/management/internal/cli/JoptOptionParserTest.java 9815a9cbadae77ea94d49288922f204acf308d61
> geode-core/src/test/java/org/apache/geode/management/internal/cli/annotations/CliArgumentJUnitTest.java 161f7c65e62f11aa7b25fb31cd94ad256ea497a6
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CliCommandTestBase.java 165f66482758dadb75ae95d23443973e9de05891
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommandsDUnitTest.java 5f885e136e09541a6d095516fc7cb7db88f659aa
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DeployCommandsDUnitTest.java 9ed5bed8faa6bfceb0849347cd94b7c8effd3191
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DiskStoreCommandsDUnitTest.java 5b07f282b83d1539df6a7ea7bf19888436738723
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/GemfireDataCommandsDUnitTest.java e99a7fbc859243579b75a76742e23fec9b738ef5
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/HelpCommandsIntegrationTest.java b91a1f35efaa00f29a0e858a90e55c1c39c163f9
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/QueueCommandsDUnitTest.java 42a0624546742491e0c8c4d8c406ae8e567c1f85
> geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelpBlockUnitTest.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelperUnitTest.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/management/internal/cli/parser/ParserUtilsJUnitTest.java 9b22e64d263d6cb8c4453e2eea42fa85e7f1f922
> geode-core/src/test/java/org/apache/geode/management/internal/cli/parser/preprocessor/PreprocessorJUnitTest.java 97325cb16b42a112680a793b6581a1fea0e8c621
> geode-core/src/test/java/org/apache/geode/management/internal/cli/parser/preprocessor/PreprocessorUtilsJUnitTest.java b56cff2cfb7027999a420ae91f1f983243f865f0
> geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategyJUnitTest.java 54c7cf7074435b48232b61916627eed69a201f09
> geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshHistoryJUnitTest.java 58453b7c70b552af30ca4e16cd624475a89e3426
> geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshJunitTest.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigDistributionDUnitTest.java abbc5c0c7e83d259cd13bae61c419f8cf07db1f0
> geode-core/src/test/java/org/apache/geode/management/internal/security/TestCommand.java ffe6a28163acf20d13b62347c40d5cceea22c629
> geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/cli/LuceneIndexCommands.java 4bfa86842fd81873c132e3bbd7b529ff3e64dbc7
> geode-lucene/src/test/java/org/apache/geode/cache/lucene/LuceneCommandsSecurityDUnitTest.java ad734e8436822112c1eb8f5c6c0be676884dd009
> geode-lucene/src/test/java/org/apache/geode/cache/lucene/internal/cli/LuceneIndexCommandsDUnitTest.java 7f203cee7899340e19d589a4618be4ea7632faac
> geode-lucene/src/test/java/org/apache/geode/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java 5c1832575ec5c5cccdce670bd9b3477708f79148
>
>
> Diff: https://reviews.apache.org/r/58589/diff/3/
>
>
> Testing
> -------
>
> precheckin passing except one unittest that always fails in concourse but succeed in local.
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 58589: GEODE-1597: use Spring shell's parser and
delete our own parsing code
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58589/
-----------------------------------------------------------
(Updated April 25, 2017, 1:08 a.m.)
Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
Changes
-------
finally precheckin is green.
Repository: geode
Description
-------
GEODE-1597: use Spring shell's parser and delete our own parsing code
* deleted usage of jopsimple and our own parser code
* reworked help/hint
The biggest change is in GfshParser, instead of directly implement SpringShell's Parser, now it's just a simple wrapper around SpringShell's SimpleParser so that we can use spring's parsing code. The challenge is that SimpleParser expects the user inupt to be in the format of "command option1 value1 option2 value2", while gfsh expects the format to be like "command option1=value1 option2=value2", so most of the code in GfshParser is to turn the input into the SimpleParser input format and then feed it into SimpleParser to get the validation and autocompletion we needed.
So far the difference I noticed with this new implementation are:
1) option validation is what we gain from directly using SpringShell.
2) SpringShell doesn't allow you to specify an option twice, so for multivalued parameters, our old parser can accept command like "change loglevel --member=member1 --member=member2", but now the parser will give you an error, and you should only do "change loglevel --member=member1,member2". The new parser did something speical for --J option though, so for --J, you can specify it multiple times.
3) For value separator, springShell default's to ",", you can only overwrite it with option context "splittingRegex", we do not honor the @CliMetaData(valueSepartor=) anymore. All of our commands uses "," for separators, so this won't break our commands, but if there is any command out there that would define a different @CliMetaData(valueSepartor=) other than ",", SpringShell would not know how to parse it.
4) To specify empty string as parameter value, before you will need to to (put region=A key="''" value="''"), spring shell would think the value you are trying to pass in are two single quotes instead of empty strings, now, you should only do (put region=A key="" value="")
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/management/cli/CliMetaData.java e69d78a2b42b4a7f21066712b9f763dc1cdb92c8
geode-core/src/main/java/org/apache/geode/management/cli/ConverterHint.java f45abc432dce8af67b71ca29fdfb85e0e9f6f87a
geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java 44004454ef1646bfeca8a7af3cffb109fd83e7d7
geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandResponseBuilder.java bda030de50cca303e2aa6d3d82b17a9111fd25ef
geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParseResult.java d879e2d6c253eb3306fb7558fa0a2b7fbe7d1e40
geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java a1d03e45dd4d6559bd9a0869c7dd95908d1858ca
geode-core/src/main/java/org/apache/geode/management/internal/cli/Launcher.java fc0427e20144a9092d97d05fc3ccbd0e21d5b35e
geode-core/src/main/java/org/apache/geode/management/internal/cli/annotation/CliArgument.java e20e73112c3b6545771c62e58795fdc53fc279c5
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ClientCommands.java 39d0442f2cb4ec4276652169a0cfa962a78af2c9
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ConfigCommands.java 50c9caaee88426f9a8c49d8b1abca9c2fbc66163
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommands.java 28ce0921e0cba412063d3a59566165544dfda3dc
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java 6324b5cfc70c0f02233f73ceab6dc56cb253cbb2
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DeployCommands.java 144408854daf2cd5b33dcdcf995efef77b3e942e
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DiskStoreCommands.java 9ad206043a284144293175baa7144bb835652a46
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/FunctionCommands.java 752ca2a612eb59c137de568f80223d58339e7231
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/GfshHelpCommands.java 1d1b28e568a1e175690fea5cde1a45a318b6db5d
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java 313d1bdfb4d1e610f2353db2a4496449453a0fbe
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommands.java 22981e72f4971f3bb297a53e9ac4d6f8ac7e149e
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/PDXCommands.java 4327decc98e3d0fe637d73857c9eed83f7bc88b9
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueueCommands.java 8c568336aa62c6b6cbf4e1a29584d514e84f1c96
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShellCommands.java b37feabe6ed61c081e9653c94128f092ad189d10
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/WanCommands.java 239db48417cdae0d415830ac411cb611b8433919
geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/BooleanConverter.java 5e9cdb9a642e9cda4934e7934c2fc022e7d9eabe
geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/DirConverter.java c97057bb281e64c96983e38172bcf7953f492c83
geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/EnumConverter.java 354cef9708623d9141caea08fe497deaa75f606f
geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/HelpConverter.java e670274f89ce4423a77715dcee07f119c1568929
geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/HintTopicConverter.java b6f9f81af10a7773df27f77ca5707238e7ef3a3b
geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/LogLevelConverter.java 3a262405da0d9821a1c9729ca0aad21ccf626a02
geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/StringArrayConverter.java eacf1810ba53fc0c389cff0d5ae30ba5fea6b0c3
geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/StringListConverter.java eab096b9a5c93fd71f9a0b757d80a2d726690ad1
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandException.java 7e5cba05d93a88fa722d98babfdabf89e4e07be0
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandInvalidException.java a14005923ee3343d2e2e8e1818b8fbac16410ca6
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandMultiModeOptionException.java acbc49618504bec9f8eb6f04124e02a47e0a92c3
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandNotAvailableException.java c471df298f10211a812ae0e644c88f81c68b8132
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionException.java a7e56be98afe94690cdc1b61ea5476a72fbd9140
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionHasMultipleValuesException.java 4b365e3f9d611e74f345f50f87c682f272d20ba4
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionInvalidException.java 1db8906024934a024febf447f8d5facc5f1b2ee8
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionMissingException.java f263dce0be966a361b8b4e84ed8360cc1a659e3b
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionNotApplicableException.java 98147787eff00d74ed5e32d2c684867fd1bf605a
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionValueConversionException.java 7dbf869eab0c8331c217aed6eb82b57ea7696be6
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionValueException.java ee02df8423ef48f02ed482ad3e40260799b2e7d7
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionValueMissingException.java 023a8789fe4a0ca2fc11bce7482d0e9ed4aa1b2a
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/ExceptionGenerator.java fda31357a39830b7414b4751966dba2e57526b13
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/ExceptionHandler.java 95afbafe55746bb0159e5067ae4d5aa9c0701b56
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/CliTopic.java 791cdca5ce954153ed5013f3108d738d970b32dd
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/HelpBlock.java PRE-CREATION
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/Helper.java PRE-CREATION
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/Block.java dde1a20ef0bfa98a7b5dcf57c1775f1769ab3c97
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/DataNode.java 8f5e570cc9761ddedcb7fb16b0a9615f2da87209
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/Help.java 68c10bb211c319bed679e39718660d1a466967a9
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/NewHelp.java 90f9eda49326fa008cc5c1abb5c02f9dcfd94fd6
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/Row.java ac1ca218668aff03d3a774ae2e1ebaebe8215b3c
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/utils/FormatOutput.java 44998a03bed5203200115ce1b2023a436a52fd84
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/utils/HelpUtils.java 11765c54df0df755b7f9b12b9d50986223d6b775
geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java 126eb47e90c2390953a91136e8cfb36687baf2fd
geode-core/src/main/java/org/apache/geode/management/internal/cli/multistep/CLIMultiStepHelper.java 0d215433c548db82b3c37b3e53be42cb0e1765fd
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/Argument.java 9acbc2a82c9cf4d4ccdb629798bdf70e1e48a305
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/AvailabilityTarget.java eff5fd2c2f67280ed9037216c1b15ab8b4a462d1
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/CommandTarget.java 3dfc01ab26d0ad6ee1867ab4014028f5cc0a3649
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/GfshMethodTarget.java 6f28830c314593004dfbb5eda113540d951fb343
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/GfshOptionParser.java a64933c2e9c5e3122a856d292b2ad70949e5017c
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/MethodParameter.java 599fb00d511de87d5a0b439371a9fec62377c027
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/Option.java 4eec1128c6e4646cde7b89e04f567fee2e419ee9
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/OptionSet.java 42270e53d047de566943a59c53969bdae8b32f54
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/Parameter.java dc371b3ba58b2ede0663e3167835926d17a3a418
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/ParserUtils.java 80f12867b7c0144ef9cb5aa5d5e7cc4f5c36b9f1
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/SyntaxConstants.java 52d92128b96e119f3157e5431d579e90218cbaad
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/jopt/JoptOptionParser.java 52224d40c9c47dd730432859033701e9b8faac4d
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/EnclosingCharacters.java a35e6269bd5ba2644fd2fbeb90c6fd8a92a8b971
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/Preprocessor.java 0dd875ad95ae30a6f2ca962da683e9e9ad0b9559
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/PreprocessorUtils.java a1872c92568a65c11f52603575a7814f2eb4c3f9
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/Stack.java ae47723f1ef2ff9af2e92c9991d02205fa044bac
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/TrimmedInput.java 8740f003f5a90a6ac296317ae4efee799e713baf
geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java c346eaf77087102f51952a567ecd7ec036593a13
geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java bcf1b415e9ee85822c470ae6888920f0a90159fd
geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java d74f5d6b4fce9a44d9eeebdfc7dcf716e9b1652b
geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/MultiCommandHelper.java 89f93d5a4f491addc955eeaaa11f0318e00f35de
geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/jline/GfshHistory.java dc4a42fc014c73df16f057155229a51bd309efa3
geode-core/src/main/java/org/apache/geode/management/internal/cli/util/CommandStringBuilder.java 16efda5f772e50b3dea4899733760b0a25513a0e
geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DataCommandsController.java cfe60899503193d4079cff4798422cd4da00cd74
geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java 527e059d1dfbe03778141ec0470d7340343f9e56
geode-core/src/test/java/org/apache/geode/management/internal/cli/CommandManagerJUnitTest.java 503ffb2929758d617e8539e6aefb3f7b545de9b6
geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserIntegrationTest.java f3e3bd8754e18d7a75faf4b75e3ba75b778fc9d7
geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserJUnitTest.java 44e99f461b9efc5a439e629751011a6d0edd9213
geode-core/src/test/java/org/apache/geode/management/internal/cli/JoptOptionParserTest.java 9815a9cbadae77ea94d49288922f204acf308d61
geode-core/src/test/java/org/apache/geode/management/internal/cli/annotations/CliArgumentJUnitTest.java 161f7c65e62f11aa7b25fb31cd94ad256ea497a6
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CliCommandTestBase.java 165f66482758dadb75ae95d23443973e9de05891
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommandsDUnitTest.java 5f885e136e09541a6d095516fc7cb7db88f659aa
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DeployCommandsDUnitTest.java 9ed5bed8faa6bfceb0849347cd94b7c8effd3191
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DiskStoreCommandsDUnitTest.java 5b07f282b83d1539df6a7ea7bf19888436738723
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/GemfireDataCommandsDUnitTest.java e99a7fbc859243579b75a76742e23fec9b738ef5
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/HelpCommandsIntegrationTest.java b91a1f35efaa00f29a0e858a90e55c1c39c163f9
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/QueueCommandsDUnitTest.java 42a0624546742491e0c8c4d8c406ae8e567c1f85
geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelpBlockUnitTest.java PRE-CREATION
geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelperUnitTest.java PRE-CREATION
geode-core/src/test/java/org/apache/geode/management/internal/cli/parser/ParserUtilsJUnitTest.java 9b22e64d263d6cb8c4453e2eea42fa85e7f1f922
geode-core/src/test/java/org/apache/geode/management/internal/cli/parser/preprocessor/PreprocessorJUnitTest.java 97325cb16b42a112680a793b6581a1fea0e8c621
geode-core/src/test/java/org/apache/geode/management/internal/cli/parser/preprocessor/PreprocessorUtilsJUnitTest.java b56cff2cfb7027999a420ae91f1f983243f865f0
geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategyJUnitTest.java 54c7cf7074435b48232b61916627eed69a201f09
geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshHistoryJUnitTest.java 58453b7c70b552af30ca4e16cd624475a89e3426
geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshJunitTest.java PRE-CREATION
geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigDistributionDUnitTest.java abbc5c0c7e83d259cd13bae61c419f8cf07db1f0
geode-core/src/test/java/org/apache/geode/management/internal/security/TestCommand.java ffe6a28163acf20d13b62347c40d5cceea22c629
geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/cli/LuceneIndexCommands.java 4bfa86842fd81873c132e3bbd7b529ff3e64dbc7
geode-lucene/src/test/java/org/apache/geode/cache/lucene/LuceneCommandsSecurityDUnitTest.java ad734e8436822112c1eb8f5c6c0be676884dd009
geode-lucene/src/test/java/org/apache/geode/cache/lucene/internal/cli/LuceneIndexCommandsDUnitTest.java 7f203cee7899340e19d589a4618be4ea7632faac
geode-lucene/src/test/java/org/apache/geode/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java 5c1832575ec5c5cccdce670bd9b3477708f79148
Diff: https://reviews.apache.org/r/58589/diff/3/
Changes: https://reviews.apache.org/r/58589/diff/2-3/
Testing
-------
precheckin passing except one unittest that always fails in concourse but succeed in local.
Thanks,
Jinmei Liao
Re: Review Request 58589: GEODE-1597: use Spring shell's parser and
delete our own parsing code
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58589/
-----------------------------------------------------------
(Updated April 24, 2017, 3:24 p.m.)
Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
Changes
-------
added value completion
Repository: geode
Description
-------
GEODE-1597: use Spring shell's parser and delete our own parsing code
* deleted usage of jopsimple and our own parser code
* reworked help/hint
The biggest change is in GfshParser, instead of directly implement SpringShell's Parser, now it's just a simple wrapper around SpringShell's SimpleParser so that we can use spring's parsing code. The challenge is that SimpleParser expects the user inupt to be in the format of "command option1 value1 option2 value2", while gfsh expects the format to be like "command option1=value1 option2=value2", so most of the code in GfshParser is to turn the input into the SimpleParser input format and then feed it into SimpleParser to get the validation and autocompletion we needed.
So far the difference I noticed with this new implementation are:
1) option validation is what we gain from directly using SpringShell.
2) SpringShell doesn't allow you to specify an option twice, so for multivalued parameters, our old parser can accept command like "change loglevel --member=member1 --member=member2", but now the parser will give you an error, and you should only do "change loglevel --member=member1,member2". The new parser did something speical for --J option though, so for --J, you can specify it multiple times.
3) For value separator, springShell default's to ",", you can only overwrite it with option context "splittingRegex", we do not honor the @CliMetaData(valueSepartor=) anymore. All of our commands uses "," for separators, so this won't break our commands, but if there is any command out there that would define a different @CliMetaData(valueSepartor=) other than ",", SpringShell would not know how to parse it.
4) To specify empty string as parameter value, before you will need to to (put region=A key="''" value="''"), spring shell would think the value you are trying to pass in are two single quotes instead of empty strings, now, you should only do (put region=A key="" value="")
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/management/cli/CliMetaData.java e69d78a2b42b4a7f21066712b9f763dc1cdb92c8
geode-core/src/main/java/org/apache/geode/management/cli/ConverterHint.java f45abc432dce8af67b71ca29fdfb85e0e9f6f87a
geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java 44004454ef1646bfeca8a7af3cffb109fd83e7d7
geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandResponseBuilder.java bda030de50cca303e2aa6d3d82b17a9111fd25ef
geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParseResult.java d879e2d6c253eb3306fb7558fa0a2b7fbe7d1e40
geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java a1d03e45dd4d6559bd9a0869c7dd95908d1858ca
geode-core/src/main/java/org/apache/geode/management/internal/cli/Launcher.java fc0427e20144a9092d97d05fc3ccbd0e21d5b35e
geode-core/src/main/java/org/apache/geode/management/internal/cli/annotation/CliArgument.java e20e73112c3b6545771c62e58795fdc53fc279c5
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ClientCommands.java 39d0442f2cb4ec4276652169a0cfa962a78af2c9
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ConfigCommands.java 50c9caaee88426f9a8c49d8b1abca9c2fbc66163
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommands.java 28ce0921e0cba412063d3a59566165544dfda3dc
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java 6324b5cfc70c0f02233f73ceab6dc56cb253cbb2
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DeployCommands.java 144408854daf2cd5b33dcdcf995efef77b3e942e
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DiskStoreCommands.java 9ad206043a284144293175baa7144bb835652a46
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/FunctionCommands.java 752ca2a612eb59c137de568f80223d58339e7231
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/GfshHelpCommands.java 1d1b28e568a1e175690fea5cde1a45a318b6db5d
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java 313d1bdfb4d1e610f2353db2a4496449453a0fbe
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommands.java 22981e72f4971f3bb297a53e9ac4d6f8ac7e149e
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/PDXCommands.java 4327decc98e3d0fe637d73857c9eed83f7bc88b9
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueueCommands.java 8c568336aa62c6b6cbf4e1a29584d514e84f1c96
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShellCommands.java b37feabe6ed61c081e9653c94128f092ad189d10
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/WanCommands.java 239db48417cdae0d415830ac411cb611b8433919
geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/BooleanConverter.java 5e9cdb9a642e9cda4934e7934c2fc022e7d9eabe
geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/DirConverter.java c97057bb281e64c96983e38172bcf7953f492c83
geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/EnumConverter.java 354cef9708623d9141caea08fe497deaa75f606f
geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/HelpConverter.java e670274f89ce4423a77715dcee07f119c1568929
geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/HintTopicConverter.java b6f9f81af10a7773df27f77ca5707238e7ef3a3b
geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/LogLevelConverter.java 3a262405da0d9821a1c9729ca0aad21ccf626a02
geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/StringArrayConverter.java eacf1810ba53fc0c389cff0d5ae30ba5fea6b0c3
geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/StringListConverter.java eab096b9a5c93fd71f9a0b757d80a2d726690ad1
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandException.java 7e5cba05d93a88fa722d98babfdabf89e4e07be0
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandInvalidException.java a14005923ee3343d2e2e8e1818b8fbac16410ca6
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandMultiModeOptionException.java acbc49618504bec9f8eb6f04124e02a47e0a92c3
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandNotAvailableException.java c471df298f10211a812ae0e644c88f81c68b8132
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionException.java a7e56be98afe94690cdc1b61ea5476a72fbd9140
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionHasMultipleValuesException.java 4b365e3f9d611e74f345f50f87c682f272d20ba4
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionInvalidException.java 1db8906024934a024febf447f8d5facc5f1b2ee8
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionMissingException.java f263dce0be966a361b8b4e84ed8360cc1a659e3b
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionNotApplicableException.java 98147787eff00d74ed5e32d2c684867fd1bf605a
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionValueConversionException.java 7dbf869eab0c8331c217aed6eb82b57ea7696be6
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionValueException.java ee02df8423ef48f02ed482ad3e40260799b2e7d7
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/CliCommandOptionValueMissingException.java 023a8789fe4a0ca2fc11bce7482d0e9ed4aa1b2a
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/ExceptionGenerator.java fda31357a39830b7414b4751966dba2e57526b13
geode-core/src/main/java/org/apache/geode/management/internal/cli/exceptions/ExceptionHandler.java 95afbafe55746bb0159e5067ae4d5aa9c0701b56
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/CliTopic.java 791cdca5ce954153ed5013f3108d738d970b32dd
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/HelpBlock.java PRE-CREATION
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/Helper.java PRE-CREATION
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/Block.java dde1a20ef0bfa98a7b5dcf57c1775f1769ab3c97
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/DataNode.java 8f5e570cc9761ddedcb7fb16b0a9615f2da87209
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/Help.java 68c10bb211c319bed679e39718660d1a466967a9
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/NewHelp.java 90f9eda49326fa008cc5c1abb5c02f9dcfd94fd6
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/format/Row.java ac1ca218668aff03d3a774ae2e1ebaebe8215b3c
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/utils/FormatOutput.java 44998a03bed5203200115ce1b2023a436a52fd84
geode-core/src/main/java/org/apache/geode/management/internal/cli/help/utils/HelpUtils.java 11765c54df0df755b7f9b12b9d50986223d6b775
geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java 126eb47e90c2390953a91136e8cfb36687baf2fd
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/Argument.java 9acbc2a82c9cf4d4ccdb629798bdf70e1e48a305
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/AvailabilityTarget.java eff5fd2c2f67280ed9037216c1b15ab8b4a462d1
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/CommandTarget.java 3dfc01ab26d0ad6ee1867ab4014028f5cc0a3649
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/GfshMethodTarget.java 6f28830c314593004dfbb5eda113540d951fb343
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/GfshOptionParser.java a64933c2e9c5e3122a856d292b2ad70949e5017c
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/MethodParameter.java 599fb00d511de87d5a0b439371a9fec62377c027
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/Option.java 4eec1128c6e4646cde7b89e04f567fee2e419ee9
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/OptionSet.java 42270e53d047de566943a59c53969bdae8b32f54
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/Parameter.java dc371b3ba58b2ede0663e3167835926d17a3a418
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/ParserUtils.java 80f12867b7c0144ef9cb5aa5d5e7cc4f5c36b9f1
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/SyntaxConstants.java 52d92128b96e119f3157e5431d579e90218cbaad
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/jopt/JoptOptionParser.java 52224d40c9c47dd730432859033701e9b8faac4d
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/EnclosingCharacters.java a35e6269bd5ba2644fd2fbeb90c6fd8a92a8b971
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/Preprocessor.java 0dd875ad95ae30a6f2ca962da683e9e9ad0b9559
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/PreprocessorUtils.java a1872c92568a65c11f52603575a7814f2eb4c3f9
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/Stack.java ae47723f1ef2ff9af2e92c9991d02205fa044bac
geode-core/src/main/java/org/apache/geode/management/internal/cli/parser/preprocessor/TrimmedInput.java 8740f003f5a90a6ac296317ae4efee799e713baf
geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java c346eaf77087102f51952a567ecd7ec036593a13
geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java bcf1b415e9ee85822c470ae6888920f0a90159fd
geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/MultiCommandHelper.java 89f93d5a4f491addc955eeaaa11f0318e00f35de
geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/jline/GfshHistory.java dc4a42fc014c73df16f057155229a51bd309efa3
geode-core/src/main/java/org/apache/geode/management/internal/cli/util/CommandStringBuilder.java 16efda5f772e50b3dea4899733760b0a25513a0e
geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DataCommandsController.java cfe60899503193d4079cff4798422cd4da00cd74
geode-core/src/test/java/org/apache/geode/management/internal/cli/CommandManagerJUnitTest.java 503ffb2929758d617e8539e6aefb3f7b545de9b6
geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserIntegrationTest.java f3e3bd8754e18d7a75faf4b75e3ba75b778fc9d7
geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserJUnitTest.java 44e99f461b9efc5a439e629751011a6d0edd9213
geode-core/src/test/java/org/apache/geode/management/internal/cli/JoptOptionParserTest.java 9815a9cbadae77ea94d49288922f204acf308d61
geode-core/src/test/java/org/apache/geode/management/internal/cli/annotations/CliArgumentJUnitTest.java 161f7c65e62f11aa7b25fb31cd94ad256ea497a6
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CliCommandTestBase.java 165f66482758dadb75ae95d23443973e9de05891
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommandsDUnitTest.java 5f885e136e09541a6d095516fc7cb7db88f659aa
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DeployCommandsDUnitTest.java 9ed5bed8faa6bfceb0849347cd94b7c8effd3191
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DiskStoreCommandsDUnitTest.java 5b07f282b83d1539df6a7ea7bf19888436738723
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/GemfireDataCommandsDUnitTest.java e99a7fbc859243579b75a76742e23fec9b738ef5
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/HelpCommandsIntegrationTest.java b91a1f35efaa00f29a0e858a90e55c1c39c163f9
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/QueueCommandsDUnitTest.java 42a0624546742491e0c8c4d8c406ae8e567c1f85
geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelpBlockUnitTest.java PRE-CREATION
geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelperUnitTest.java PRE-CREATION
geode-core/src/test/java/org/apache/geode/management/internal/cli/parser/ParserUtilsJUnitTest.java 9b22e64d263d6cb8c4453e2eea42fa85e7f1f922
geode-core/src/test/java/org/apache/geode/management/internal/cli/parser/preprocessor/PreprocessorJUnitTest.java 97325cb16b42a112680a793b6581a1fea0e8c621
geode-core/src/test/java/org/apache/geode/management/internal/cli/parser/preprocessor/PreprocessorUtilsJUnitTest.java b56cff2cfb7027999a420ae91f1f983243f865f0
geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategyJUnitTest.java 54c7cf7074435b48232b61916627eed69a201f09
geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshHistoryJUnitTest.java 58453b7c70b552af30ca4e16cd624475a89e3426
geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshJunitTest.java PRE-CREATION
geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigDistributionDUnitTest.java abbc5c0c7e83d259cd13bae61c419f8cf07db1f0
geode-core/src/test/java/org/apache/geode/management/internal/security/TestCommand.java ffe6a28163acf20d13b62347c40d5cceea22c629
geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/cli/LuceneIndexCommands.java 4bfa86842fd81873c132e3bbd7b529ff3e64dbc7
geode-lucene/src/test/java/org/apache/geode/cache/lucene/internal/cli/LuceneIndexCommandsDUnitTest.java 7f203cee7899340e19d589a4618be4ea7632faac
geode-lucene/src/test/java/org/apache/geode/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java 5c1832575ec5c5cccdce670bd9b3477708f79148
Diff: https://reviews.apache.org/r/58589/diff/2/
Changes: https://reviews.apache.org/r/58589/diff/1-2/
Testing
-------
precheckin passing except one unittest that always fails in concourse but succeed in local.
Thanks,
Jinmei Liao