You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Patrick Rhomberg <pr...@pivotal.io> on 2017/03/29 22:33:17 UTC

Review Request 58050: GEODE-2725: export logs --dir now honored when not connected via HTTP[S].

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58050/
-----------------------------------------------------------

Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, and Swapnil Bawaskar.


Repository: geode


Description
-------

Also: Collapsed unrelated exception catch blocks.


Diffs
-----

  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/ExportLogCommand.java 3f147c19a128dce78c51c31e6758e517cd2ab496 
  geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java d74f5d6b4fce9a44d9eeebdfc7dcf716e9b1652b 


Diff: https://reviews.apache.org/r/58050/diff/1/


Testing
-------

Manual verification of behavior both with and without --dir option, both connected via HTTP and not.

I should write some unit tests for it, but would like to pair on that since these tests would be more involved than a copy-paste-modify of the other unit test I "wrote."

Question: do we prefer to be consistent with previously existing behavior, or consistent in behavior across connection types?  That initial conditional is only in place to preserve existing behavior, where the non-HTTP connection defaults to placing logs in the locator's directory.  Without that block, we would move logs to the current working directory, which would be consistent with the via-HTTP scenario.


Thanks,

Patrick Rhomberg


Re: Review Request 58050: GEODE-2725: export logs --dir now honored when not connected via HTTP[S].

Posted by Jared Stewart <js...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58050/#review170749
-----------------------------------------------------------


Ship it!




Ship It!

- Jared Stewart


On March 31, 2017, 6:22 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58050/
> -----------------------------------------------------------
> 
> (Updated March 31, 2017, 6:22 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> export logs --dir refers to local filesystem when connected via HTTP and refers to the managers filesystem when connected via JMX.  This behavior will be changed in GEODE-2663.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java 3f147c19a128dce78c51c31e6758e517cd2ab496 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java 5b1f089c18c404f64929398f6015839eb783ccb4 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsIntegrationTest.java 268fa397db253f12c0effdbf6faa5e822730144c 
>   geode-core/src/test/resources/org/apache/geode/management/internal/cli/commands/golden-help-offline.properties 3c56def01ad58f98ea1707f4dd6b57e643e9eab1 
> 
> 
> Diff: https://reviews.apache.org/r/58050/diff/4/
> 
> 
> Testing
> -------
> 
> precheckin running.
> 
> Manual verification of behavior both with and without --dir option, both connected via HTTP and not.
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>


Re: Review Request 58050: GEODE-2725: export logs --dir now honored when not connected via HTTP[S].

Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58050/#review170883
-----------------------------------------------------------


Ship it!




Ship It!

- Jinmei Liao


On March 31, 2017, 11:16 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58050/
> -----------------------------------------------------------
> 
> (Updated March 31, 2017, 11:16 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> export logs --dir refers to local filesystem when connected via HTTP and refers to the managers filesystem when connected via JMX.  This behavior will be changed in GEODE-2663.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java 3f147c1 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java 5b1f089 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsIntegrationTest.java 9a39298 
>   geode-core/src/test/resources/org/apache/geode/management/internal/cli/commands/golden-help-offline.properties 3c56def 
>   geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsOverHttpIntegrationTest.java 90f16ea 
> 
> 
> Diff: https://reviews.apache.org/r/58050/diff/5/
> 
> 
> Testing
> -------
> 
> precheckin running.
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>


Re: Review Request 58050: GEODE-2725: export logs --dir now honored when not connected via HTTP[S].

Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58050/#review170921
-----------------------------------------------------------


Ship it!




Ship It!

- Kirk Lund


On March 31, 2017, 11:16 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58050/
> -----------------------------------------------------------
> 
> (Updated March 31, 2017, 11:16 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> export logs --dir refers to local filesystem when connected via HTTP and refers to the managers filesystem when connected via JMX.  This behavior will be changed in GEODE-2663.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java 3f147c1 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java 5b1f089 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsIntegrationTest.java 9a39298 
>   geode-core/src/test/resources/org/apache/geode/management/internal/cli/commands/golden-help-offline.properties 3c56def 
>   geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsOverHttpIntegrationTest.java 90f16ea 
> 
> 
> Diff: https://reviews.apache.org/r/58050/diff/5/
> 
> 
> Testing
> -------
> 
> precheckin running.
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>


Re: Review Request 58050: GEODE-2725: export logs --dir now honored when not connected via HTTP[S].

Posted by Patrick Rhomberg <pr...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58050/
-----------------------------------------------------------

(Updated March 31, 2017, 11:16 p.m.)


Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, and Swapnil Bawaskar.


Changes
-------

Very prematurely closed this before.  Inherited HTTP test class exploded.  Thanks to jstewart for smashing that out.


Repository: geode


Description
-------

export logs --dir refers to local filesystem when connected via HTTP and refers to the managers filesystem when connected via JMX.  This behavior will be changed in GEODE-2663.


Diffs (updated)
-----

  geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java 3f147c1 
  geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java 5b1f089 
  geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsIntegrationTest.java 9a39298 
  geode-core/src/test/resources/org/apache/geode/management/internal/cli/commands/golden-help-offline.properties 3c56def 
  geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsOverHttpIntegrationTest.java 90f16ea 


Diff: https://reviews.apache.org/r/58050/diff/5/

Changes: https://reviews.apache.org/r/58050/diff/4-5/


Testing (updated)
-------

precheckin running.


Thanks,

Patrick Rhomberg


Re: Review Request 58050: GEODE-2725: export logs --dir now honored when not connected via HTTP[S].

Posted by Patrick Rhomberg <pr...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58050/
-----------------------------------------------------------

(Updated March 31, 2017, 6:22 p.m.)


Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, and Swapnil Bawaskar.


Changes
-------

Suggested edits made.  Paths are now hardware agnostic, tests are cleaned.


Repository: geode


Description
-------

export logs --dir refers to local filesystem when connected via HTTP and refers to the managers filesystem when connected via JMX.  This behavior will be changed in GEODE-2663.


Diffs (updated)
-----

  geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java 3f147c19a128dce78c51c31e6758e517cd2ab496 
  geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java 5b1f089c18c404f64929398f6015839eb783ccb4 
  geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsIntegrationTest.java 268fa397db253f12c0effdbf6faa5e822730144c 
  geode-core/src/test/resources/org/apache/geode/management/internal/cli/commands/golden-help-offline.properties 3c56def01ad58f98ea1707f4dd6b57e643e9eab1 


Diff: https://reviews.apache.org/r/58050/diff/4/

Changes: https://reviews.apache.org/r/58050/diff/3-4/


Testing
-------

precheckin running.

Manual verification of behavior both with and without --dir option, both connected via HTTP and not.


Thanks,

Patrick Rhomberg


Re: Review Request 58050: GEODE-2725: export logs --dir now honored when not connected via HTTP[S].

Posted by Ken Howe <kh...@pivotal.io>.

> On March 30, 2017, 10:44 p.m., Jared Stewart wrote:
> > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsIntegrationTest.java
> > Line 32 (original), 35 (patched)
> > <https://reviews.apache.org/r/58050/diff/3/?file=1681140#file1681140line35>
> >
> >     Let's change this to a `@Rule` instead of a `@ClassRule`.  This will cause the locator to be created and destroyed for every `@Test`, rather than once for the whole class.  The all of the `try{} finally{}` blocks below will be unnecessary.  The `LocatorStarterRule` uses a `TemporaryFolder` to back the locator's working dir that will automatically be deleted by the `@Rule`'s lifecycle.

Good suggestion. Also, for the absolute path test we should be able to resolve the TemporaryFolder to an absolute path and use that as the root of the subdirectory instead of arbitarily using /tmp.


- Ken


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58050/#review170648
-----------------------------------------------------------


On March 30, 2017, 6:17 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58050/
> -----------------------------------------------------------
> 
> (Updated March 30, 2017, 6:17 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> export logs --dir refers to local filesystem when connected via HTTP and refers to the managers filesystem when connected via JMX.  This behavior will be changed in GEODE-2663.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java 3f147c19a128dce78c51c31e6758e517cd2ab496 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java 5b1f089c18c404f64929398f6015839eb783ccb4 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsIntegrationTest.java 268fa397db253f12c0effdbf6faa5e822730144c 
>   geode-core/src/test/resources/org/apache/geode/management/internal/cli/commands/golden-help-offline.properties 3c56def01ad58f98ea1707f4dd6b57e643e9eab1 
> 
> 
> Diff: https://reviews.apache.org/r/58050/diff/3/
> 
> 
> Testing
> -------
> 
> precheckin running.
> 
> Manual verification of behavior both with and without --dir option, both connected via HTTP and not.
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>


Re: Review Request 58050: GEODE-2725: export logs --dir now honored when not connected via HTTP[S].

Posted by Jared Stewart <js...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58050/#review170648
-----------------------------------------------------------




geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsIntegrationTest.java
Line 32 (original), 35 (patched)
<https://reviews.apache.org/r/58050/#comment243539>

    Let's change this to a `@Rule` instead of a `@ClassRule`.  This will cause the locator to be created and destroyed for every `@Test`, rather than once for the whole class.  The all of the `try{} finally{}` blocks below will be unnecessary.  The `LocatorStarterRule` uses a `TemporaryFolder` to back the locator's working dir that will automatically be deleted by the `@Rule`'s lifecycle.



geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsIntegrationTest.java
Line 38 (original), 41 (patched)
<https://reviews.apache.org/r/58050/#comment243540>

    If we add the `@Before` annotation to this method, JUnit will automatically invoke it before every `@Test` method. Then we can delete the repetitive calls to `connect()` at the beginning of each `@Test` method below.



geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsIntegrationTest.java
Lines 81 (patched)
<https://reviews.apache.org/r/58050/#comment243538>

    This way of resolving the subdirectory will not work on any OS that does not use "/" as a file separator (i.e. Windows).   Try this instead:
    
    ```
          Path workingDirPath = locator.getWorkingDir().toPath();
          Path subdirPath =    workingDirPath.resolve("some").resolve("test").resolve("directory");
          String relativeDir = workingDirPath.relativize(subdirPath).toString();
    
          gfsh.executeCommand("export logs --dir=some/test/directory");
          assertThat(FileUtils.listFiles(subdirPath.toFile(), extensions, false)).isNotEmpty();
          assertThat(FileUtils.listFiles(locator.getWorkingDir(), extensions, false)).isEmpty();
    ```


- Jared Stewart


On March 30, 2017, 6:17 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58050/
> -----------------------------------------------------------
> 
> (Updated March 30, 2017, 6:17 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> export logs --dir refers to local filesystem when connected via HTTP and refers to the managers filesystem when connected via JMX.  This behavior will be changed in GEODE-2663.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java 3f147c19a128dce78c51c31e6758e517cd2ab496 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java 5b1f089c18c404f64929398f6015839eb783ccb4 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsIntegrationTest.java 268fa397db253f12c0effdbf6faa5e822730144c 
>   geode-core/src/test/resources/org/apache/geode/management/internal/cli/commands/golden-help-offline.properties 3c56def01ad58f98ea1707f4dd6b57e643e9eab1 
> 
> 
> Diff: https://reviews.apache.org/r/58050/diff/3/
> 
> 
> Testing
> -------
> 
> precheckin running.
> 
> Manual verification of behavior both with and without --dir option, both connected via HTTP and not.
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>


Re: Review Request 58050: GEODE-2725: export logs --dir now honored when not connected via HTTP[S].

Posted by Patrick Rhomberg <pr...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58050/
-----------------------------------------------------------

(Updated March 30, 2017, 6:17 p.m.)


Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, and Swapnil Bawaskar.


Changes
-------

Added appropriate unittests, reverted collapse of exceptions in files otherwise untouched, corrected --dir behavior to not also address GEODE-2663.


Repository: geode


Description (updated)
-------

export logs --dir refers to local filesystem when connected via HTTP and refers to the managers filesystem when connected via JMX.  This behavior will be changed in GEODE-2663.


Diffs (updated)
-----

  geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java 3f147c19a128dce78c51c31e6758e517cd2ab496 
  geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsIntegrationTest.java 268fa397db253f12c0effdbf6faa5e822730144c 


Diff: https://reviews.apache.org/r/58050/diff/2/

Changes: https://reviews.apache.org/r/58050/diff/1-2/


Testing (updated)
-------

precheckin running.

Manual verification of behavior both with and without --dir option, both connected via HTTP and not.


Thanks,

Patrick Rhomberg


Re: Review Request 58050: GEODE-2725: export logs --dir now honored when not connected via HTTP[S].

Posted by Ken Howe <kh...@pivotal.io>.

> On March 29, 2017, 11:17 p.m., Kirk Lund wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java
> > Line 53 (original), 53 (patched)
> > <https://reviews.apache.org/r/58050/diff/1/?file=1680124#file1680124line53>
> >
> >     I keep seeing people changing this class and my reviews keep asking for the same things: 1) rename it to ExportLogsCommand to be consistent with other code, 2) create ExportLogsCommandTest which is a UnitTest and start writing some unit tests for this class.

This rename is part of the review request I posted earlier today: https://reviews.apache.org/r/58080/#review170604


- Ken


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58050/#review170497
-----------------------------------------------------------


On March 30, 2017, 6:17 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58050/
> -----------------------------------------------------------
> 
> (Updated March 30, 2017, 6:17 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> export logs --dir refers to local filesystem when connected via HTTP and refers to the managers filesystem when connected via JMX.  This behavior will be changed in GEODE-2663.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java 3f147c19a128dce78c51c31e6758e517cd2ab496 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java 5b1f089c18c404f64929398f6015839eb783ccb4 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsIntegrationTest.java 268fa397db253f12c0effdbf6faa5e822730144c 
>   geode-core/src/test/resources/org/apache/geode/management/internal/cli/commands/golden-help-offline.properties 3c56def01ad58f98ea1707f4dd6b57e643e9eab1 
> 
> 
> Diff: https://reviews.apache.org/r/58050/diff/3/
> 
> 
> Testing
> -------
> 
> precheckin running.
> 
> Manual verification of behavior both with and without --dir option, both connected via HTTP and not.
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>


Re: Review Request 58050: GEODE-2725: export logs --dir now honored when not connected via HTTP[S].

Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58050/#review170497
-----------------------------------------------------------



Every single class that is touched needs to have a new UnitTest written for it. If none exists, then create a new UnitTest "MyClassTest" with @Category(UnitTest.class). We should never touch a class going forward without doing this first.


geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java
Line 182 (original), 182 (patched)
<https://reviews.apache.org/r/58050/#comment243335>

    Please start using tests to drive what you're doing. Example: try adding a new test to GfshParserJUnitTest which is going to cause one of these Exceptions to be thrown. As you start practicing TDD, you'll end up writing tests for everything and before you make any product code changes. You'll also find that you in many cases you'll need to start refactoring the class in order to facilitate proper testing (unit testing).



geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java
Line 53 (original), 53 (patched)
<https://reviews.apache.org/r/58050/#comment243336>

    I keep seeing people changing this class and my reviews keep asking for the same things: 1) rename it to ExportLogsCommand to be consistent with other code, 2) create ExportLogsCommandTest which is a UnitTest and start writing some unit tests for this class.


- Kirk Lund


On March 29, 2017, 10:33 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58050/
> -----------------------------------------------------------
> 
> (Updated March 29, 2017, 10:33 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Also: Collapsed unrelated exception catch blocks.
> 
> 
> Diffs
> -----
> 
>   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/ExportLogCommand.java 3f147c19a128dce78c51c31e6758e517cd2ab496 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java d74f5d6b4fce9a44d9eeebdfc7dcf716e9b1652b 
> 
> 
> Diff: https://reviews.apache.org/r/58050/diff/1/
> 
> 
> Testing
> -------
> 
> Manual verification of behavior both with and without --dir option, both connected via HTTP and not.
> 
> I should write some unit tests for it, but would like to pair on that since these tests would be more involved than a copy-paste-modify of the other unit test I "wrote."
> 
> Question: do we prefer to be consistent with previously existing behavior, or consistent in behavior across connection types?  That initial conditional is only in place to preserve existing behavior, where the non-HTTP connection defaults to placing logs in the locator's directory.  Without that block, we would move logs to the current working directory, which would be consistent with the via-HTTP scenario.
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>