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/05/31 17:42:48 UTC
Review Request 59686: GEODE-2983: correctly handling --J option value
that has ", " inside.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59686/
-----------------------------------------------------------
Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
Repository: geode
Description
-------
GEODE-2983: correctly handling --J option value that has "," inside.
Diffs
-----
geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java 288ea054ae1230c480d141c0159d6ccf9c299a7d
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java 74acfd6e03613ac4d0c62fcdd4ea859d1c74d2f2
geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserParsingTest.java 4467792f60a2a3bf7cc53cf35e997e7462882539
Diff: https://reviews.apache.org/r/59686/diff/1/
Testing
-------
precheckin running
Thanks,
Jinmei Liao
Re: Review Request 59686: GEODE-2983: correctly handling --J option
value that has ", " inside.
Posted by Ken Howe <kh...@pivotal.io>.
> On May 31, 2017, 10:18 p.m., Jared Stewart wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java
> > Lines 48 (patched)
> > <https://reviews.apache.org/r/59686/diff/1/?file=1735898#file1735898line48>
> >
> > I worry that a user may at some point specify a `--J` option that includes `,,`. I think our code could be more robust by using a delimeter that a user can't type on a standard keyboard. The ASCII "Unit separator" character (decimal code 31, hex 0x1f) seems like a natural candidate. That would make this look like:
> >
> > ```
> > private static final char ASCII_UNIT_SEPARATOR = '\u001F';
> > public static final String JARGUMENT_SPLIREGX = "" + ASCII_UNIT_SEPARATOR;
> > ```
> >
> >
> > I also think that `J_ARGUMENT_DELIMITER` might be a more informative name.
+1
- Ken
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59686/#review176535
-----------------------------------------------------------
On May 31, 2017, 5:42 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59686/
> -----------------------------------------------------------
>
> (Updated May 31, 2017, 5:42 p.m.)
>
>
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-2983: correctly handling --J option value that has "," inside.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java 288ea054ae1230c480d141c0159d6ccf9c299a7d
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java 74acfd6e03613ac4d0c62fcdd4ea859d1c74d2f2
> geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserParsingTest.java 4467792f60a2a3bf7cc53cf35e997e7462882539
>
>
> Diff: https://reviews.apache.org/r/59686/diff/1/
>
>
> Testing
> -------
>
> precheckin running
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 59686: GEODE-2983: correctly handling --J option
value that has ", " inside.
Posted by Jared Stewart <js...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59686/#review176535
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java
Lines 48 (patched)
<https://reviews.apache.org/r/59686/#comment249916>
I worry that a user may at some point specify a `--J` option that includes `,,`. I think our code could be more robust by using a delimeter that a user can't type on a standard keyboard. The ASCII "Unit separator" character (decimal code 31, hex 0x1f) seems like a natural candidate. That would make this look like:
```
private static final char ASCII_UNIT_SEPARATOR = '\u001F';
public static final String JARGUMENT_SPLIREGX = "" + ASCII_UNIT_SEPARATOR;
```
I also think that `J_ARGUMENT_DELIMITER` might be a more informative name.
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java
Lines 233 (patched)
<https://reviews.apache.org/r/59686/#comment249921>
What do you think about adding a field to `GfshParser` that can be referenced by all of these `optionContexts`? Basically:
```
class GfshParser {
...
J_ARGUMENT_OPTION_CONTEXT = "splittingRegex=" + JARGUMENT_SPLIREGX;
}
```
```
@CliOption(key = CliStrings.START_LOCATOR__J,
optionContext = GfshParser.J_ARGUMENT_OPTION_CONTEXT,
help = CliStrings.START_LOCATOR__J__HELP) final String[] jvmArgsOpts,
```
- Jared Stewart
On May 31, 2017, 5:42 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59686/
> -----------------------------------------------------------
>
> (Updated May 31, 2017, 5:42 p.m.)
>
>
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-2983: correctly handling --J option value that has "," inside.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java 288ea054ae1230c480d141c0159d6ccf9c299a7d
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java 74acfd6e03613ac4d0c62fcdd4ea859d1c74d2f2
> geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserParsingTest.java 4467792f60a2a3bf7cc53cf35e997e7462882539
>
>
> Diff: https://reviews.apache.org/r/59686/diff/1/
>
>
> Testing
> -------
>
> precheckin running
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 59686: GEODE-2983: correctly handling --J option
value that has ", " inside.
Posted by Emily Yeh <ey...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59686/#review176783
-----------------------------------------------------------
Ship it!
Ship It!
- Emily Yeh
On May 31, 2017, 10:44 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59686/
> -----------------------------------------------------------
>
> (Updated May 31, 2017, 10:44 p.m.)
>
>
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-2983: correctly handling --J option value that has "," inside.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java 288ea054ae1230c480d141c0159d6ccf9c299a7d
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java 74acfd6e03613ac4d0c62fcdd4ea859d1c74d2f2
> geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserParsingTest.java 4467792f60a2a3bf7cc53cf35e997e7462882539
>
>
> Diff: https://reviews.apache.org/r/59686/diff/2/
>
>
> Testing
> -------
>
> precheckin running
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 59686: GEODE-2983: correctly handling --J option
value that has ", " inside.
Posted by Jared Stewart <js...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59686/#review176544
-----------------------------------------------------------
Ship it!
Ship It!
- Jared Stewart
On May 31, 2017, 10:44 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59686/
> -----------------------------------------------------------
>
> (Updated May 31, 2017, 10:44 p.m.)
>
>
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-2983: correctly handling --J option value that has "," inside.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java 288ea054ae1230c480d141c0159d6ccf9c299a7d
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java 74acfd6e03613ac4d0c62fcdd4ea859d1c74d2f2
> geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserParsingTest.java 4467792f60a2a3bf7cc53cf35e997e7462882539
>
>
> Diff: https://reviews.apache.org/r/59686/diff/2/
>
>
> Testing
> -------
>
> precheckin running
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 59686: GEODE-2983: correctly handling --J option
value that has ", " inside.
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59686/
-----------------------------------------------------------
(Updated May 31, 2017, 10:44 p.m.)
Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
Repository: geode
Description
-------
GEODE-2983: correctly handling --J option value that has "," inside.
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java 288ea054ae1230c480d141c0159d6ccf9c299a7d
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java 74acfd6e03613ac4d0c62fcdd4ea859d1c74d2f2
geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserParsingTest.java 4467792f60a2a3bf7cc53cf35e997e7462882539
Diff: https://reviews.apache.org/r/59686/diff/2/
Changes: https://reviews.apache.org/r/59686/diff/1-2/
Testing
-------
precheckin running
Thanks,
Jinmei Liao