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

2017-04-03 Thread Kirk Lund 

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

2017-04-03 Thread Jinmei Liao 

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

2017-03-31 Thread Patrick Rhomberg 

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

2017-03-31 Thread Jared Stewart 

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

2017-03-31 Thread Patrick Rhomberg 

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

2017-03-30 Thread Ken Howe 


> 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)
> > 
> >
> > 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].

2017-03-30 Thread Jared Stewart 

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


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)


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)


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

2017-03-30 Thread Ken Howe 


> 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)
> > 
> >
> > 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].

2017-03-30 Thread Patrick Rhomberg 

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

2017-03-29 Thread Kirk Lund 

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


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)


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