> 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
>
>
