Re: Review Request 59611: GEODE-2989: Improve mechanism for scanning the classpath to find gfsh commands

2017-05-31 Thread Jinmei Liao 


> On May 31, 2017, 5:58 p.m., Jinmei Liao wrote:
> > geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java
> > Lines 96 (patched)
> > 
> >
> > should we put this in the MemberStarterRule so that both locator/server 
> > can have this option?
> 
> Jared Stewart wrote:
> I don't think that will work in this case.  If we tried to put this in 
> `MemberStarterRule`, it would fail since that class is abstract (hence cannot 
> be instantiated): 
> ```
>   public static MemberStarterRule createWithoutTemporaryWorkingDir() {
> return new MemberStarterRule(new 
> File(System.getProperty("user.dir")));
>   }
> ```
> 
> So, since we need to specify `new ServerStarterRule` I think this does 
> belong in `ServerStarterRule`.  We could add a similar method to 
> `LocatorStarterRule`, but I might advocate for waiting until it's needed by a 
> test.

Sure, let's keep it in mind for future improvement, like having the option of 
putting the log statement in the file or in stdout.


- Jinmei


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


On May 26, 2017, 10:02 p.m., Jared Stewart wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59611/
> ---
> 
> (Updated May 26, 2017, 10:02 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jinmei Liao, and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-2989: Improve mechanism for scanning the classpath to find gfsh commands
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java
>  0576e46fce08f9c969726817e0012a2094f79fbe 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/util/ClasspathScanLoadHelper.java
>  20fffbd5c492cfb4642ce41c937da3d499d3434c 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/ClasspathScanLoadHelperJUnitTest.java
>  a13ca351c49da2bc523e6d3ad9dd3e845b7b0429 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshInitFileJUnitTest.java
>  159c47ffbd71c6d08b563d8d28d5d7cdc4fb096b 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/MemberMBeanSecurityJUnitTest.java
>  65fd528641771e535f3d8d0d6601cef53f91af7a 
>   
> geode-core/src/test/java/org/apache/geode/security/PDXPostProcessorDUnitTest.java
>  e9523862da9e045b05417dd8123574b01622c497 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java
>  30ae59fd786b4753ae71849f81deeb0fe7f74c17 
>   
> geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/ShellCommandsControllerJUnitTest.java
>  10e26f6c5d006856e9e88b06a60f5e67cb68a2ce 
> 
> 
> Diff: https://reviews.apache.org/r/59611/diff/1/
> 
> 
> Testing
> ---
> 
> - Precheckin passed
>  - Further cleanup of CommandManager is expected in a subsequent ticket
> 
> 
> Thanks,
> 
> Jared Stewart
> 
>

Re: Review Request 59611: GEODE-2989: Improve mechanism for scanning the classpath to find gfsh commands

2017-05-31 Thread Jared Stewart 


> On May 31, 2017, 5:58 p.m., Jinmei Liao wrote:
> > geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java
> > Lines 96 (patched)
> > 
> >
> > should we put this in the MemberStarterRule so that both locator/server 
> > can have this option?

I don't think that will work in this case.  If we tried to put this in 
`MemberStarterRule`, it would fail since that class is abstract (hence cannot 
be instantiated): 
```
  public static MemberStarterRule createWithoutTemporaryWorkingDir() {
return new MemberStarterRule(new File(System.getProperty("user.dir")));
  }
```

So, since we need to specify `new ServerStarterRule` I think this does belong 
in `ServerStarterRule`.  We could add a similar method to `LocatorStarterRule`, 
but I might advocate for waiting until it's needed by a test.


- Jared


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


On May 26, 2017, 10:02 p.m., Jared Stewart wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59611/
> ---
> 
> (Updated May 26, 2017, 10:02 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jinmei Liao, and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-2989: Improve mechanism for scanning the classpath to find gfsh commands
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java
>  0576e46fce08f9c969726817e0012a2094f79fbe 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/util/ClasspathScanLoadHelper.java
>  20fffbd5c492cfb4642ce41c937da3d499d3434c 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/ClasspathScanLoadHelperJUnitTest.java
>  a13ca351c49da2bc523e6d3ad9dd3e845b7b0429 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshInitFileJUnitTest.java
>  159c47ffbd71c6d08b563d8d28d5d7cdc4fb096b 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/MemberMBeanSecurityJUnitTest.java
>  65fd528641771e535f3d8d0d6601cef53f91af7a 
>   
> geode-core/src/test/java/org/apache/geode/security/PDXPostProcessorDUnitTest.java
>  e9523862da9e045b05417dd8123574b01622c497 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java
>  30ae59fd786b4753ae71849f81deeb0fe7f74c17 
>   
> geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/ShellCommandsControllerJUnitTest.java
>  10e26f6c5d006856e9e88b06a60f5e67cb68a2ce 
> 
> 
> Diff: https://reviews.apache.org/r/59611/diff/1/
> 
> 
> Testing
> ---
> 
> - Precheckin passed
>  - Further cleanup of CommandManager is expected in a subsequent ticket
> 
> 
> Thanks,
> 
> Jared Stewart
> 
>

Re: Review Request 59611: GEODE-2989: Improve mechanism for scanning the classpath to find gfsh commands

2017-05-31 Thread Jinmei Liao 

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




geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java
Lines 96 (patched)


should we put this in the MemberStarterRule so that both locator/server can 
have this option?


- Jinmei Liao


On May 26, 2017, 10:02 p.m., Jared Stewart wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59611/
> ---
> 
> (Updated May 26, 2017, 10:02 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jinmei Liao, and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-2989: Improve mechanism for scanning the classpath to find gfsh commands
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java
>  0576e46fce08f9c969726817e0012a2094f79fbe 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/util/ClasspathScanLoadHelper.java
>  20fffbd5c492cfb4642ce41c937da3d499d3434c 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/ClasspathScanLoadHelperJUnitTest.java
>  a13ca351c49da2bc523e6d3ad9dd3e845b7b0429 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshInitFileJUnitTest.java
>  159c47ffbd71c6d08b563d8d28d5d7cdc4fb096b 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/MemberMBeanSecurityJUnitTest.java
>  65fd528641771e535f3d8d0d6601cef53f91af7a 
>   
> geode-core/src/test/java/org/apache/geode/security/PDXPostProcessorDUnitTest.java
>  e9523862da9e045b05417dd8123574b01622c497 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java
>  30ae59fd786b4753ae71849f81deeb0fe7f74c17 
>   
> geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/ShellCommandsControllerJUnitTest.java
>  10e26f6c5d006856e9e88b06a60f5e67cb68a2ce 
> 
> 
> Diff: https://reviews.apache.org/r/59611/diff/1/
> 
> 
> Testing
> ---
> 
> - Precheckin passed
>  - Further cleanup of CommandManager is expected in a subsequent ticket
> 
> 
> Thanks,
> 
> Jared Stewart
> 
>

Re: Review Request 59611: GEODE-2989: Improve mechanism for scanning the classpath to find gfsh commands

2017-05-30 Thread Jared Stewart 


> On May 30, 2017, 11:33 p.m., Ken Howe wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/ClasspathScanLoadHelper.java
> > Line 63 (original), 41 (patched)
> > 
> >
> > This new method in a product class is only used in a test class. It 
> > would be better to move this out of product code to the test where it's 
> > needed.

Good catch, the old method used to handle both of these two cases and I didn't 
notice after splitting them up that one was only used for tests. I'll pull it 
out of the product code.


- Jared


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


On May 26, 2017, 10:02 p.m., Jared Stewart wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59611/
> ---
> 
> (Updated May 26, 2017, 10:02 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jinmei Liao, and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-2989: Improve mechanism for scanning the classpath to find gfsh commands
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java
>  0576e46fce08f9c969726817e0012a2094f79fbe 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/util/ClasspathScanLoadHelper.java
>  20fffbd5c492cfb4642ce41c937da3d499d3434c 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/ClasspathScanLoadHelperJUnitTest.java
>  a13ca351c49da2bc523e6d3ad9dd3e845b7b0429 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshInitFileJUnitTest.java
>  159c47ffbd71c6d08b563d8d28d5d7cdc4fb096b 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/MemberMBeanSecurityJUnitTest.java
>  65fd528641771e535f3d8d0d6601cef53f91af7a 
>   
> geode-core/src/test/java/org/apache/geode/security/PDXPostProcessorDUnitTest.java
>  e9523862da9e045b05417dd8123574b01622c497 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java
>  30ae59fd786b4753ae71849f81deeb0fe7f74c17 
>   
> geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/ShellCommandsControllerJUnitTest.java
>  10e26f6c5d006856e9e88b06a60f5e67cb68a2ce 
> 
> 
> Diff: https://reviews.apache.org/r/59611/diff/1/
> 
> 
> Testing
> ---
> 
> - Precheckin passed
>  - Further cleanup of CommandManager is expected in a subsequent ticket
> 
> 
> Thanks,
> 
> Jared Stewart
> 
>

Re: Review Request 59611: GEODE-2989: Improve mechanism for scanning the classpath to find gfsh commands

2017-05-30 Thread Ken Howe 

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




geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java
Lines 171 (patched)


Remove extra blank line



geode-core/src/main/java/org/apache/geode/management/internal/cli/util/ClasspathScanLoadHelper.java
Line 63 (original), 41 (patched)


This new method in a product class is only used in a test class. It would 
be better to move this out of product code to the test where it's needed.


- Ken Howe


On May 26, 2017, 10:02 p.m., Jared Stewart wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59611/
> ---
> 
> (Updated May 26, 2017, 10:02 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jinmei Liao, and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-2989: Improve mechanism for scanning the classpath to find gfsh commands
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java
>  0576e46fce08f9c969726817e0012a2094f79fbe 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/util/ClasspathScanLoadHelper.java
>  20fffbd5c492cfb4642ce41c937da3d499d3434c 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/ClasspathScanLoadHelperJUnitTest.java
>  a13ca351c49da2bc523e6d3ad9dd3e845b7b0429 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshInitFileJUnitTest.java
>  159c47ffbd71c6d08b563d8d28d5d7cdc4fb096b 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/MemberMBeanSecurityJUnitTest.java
>  65fd528641771e535f3d8d0d6601cef53f91af7a 
>   
> geode-core/src/test/java/org/apache/geode/security/PDXPostProcessorDUnitTest.java
>  e9523862da9e045b05417dd8123574b01622c497 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java
>  30ae59fd786b4753ae71849f81deeb0fe7f74c17 
>   
> geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/ShellCommandsControllerJUnitTest.java
>  10e26f6c5d006856e9e88b06a60f5e67cb68a2ce 
> 
> 
> Diff: https://reviews.apache.org/r/59611/diff/1/
> 
> 
> Testing
> ---
> 
> - Precheckin passed
>  - Further cleanup of CommandManager is expected in a subsequent ticket
> 
> 
> Thanks,
> 
> Jared Stewart
> 
>

Review Request 59611: GEODE-2989: Improve mechanism for scanning the classpath to find gfsh commands

2017-05-26 Thread Jared Stewart 

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

Review request for geode.


Repository: geode


Description
---

GEODE-2989: Improve mechanism for scanning the classpath to find gfsh commands


Diffs
-

  
geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java
 0576e46fce08f9c969726817e0012a2094f79fbe 
  
geode-core/src/main/java/org/apache/geode/management/internal/cli/util/ClasspathScanLoadHelper.java
 20fffbd5c492cfb4642ce41c937da3d499d3434c 
  
geode-core/src/test/java/org/apache/geode/management/internal/cli/ClasspathScanLoadHelperJUnitTest.java
 a13ca351c49da2bc523e6d3ad9dd3e845b7b0429 
  
geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshInitFileJUnitTest.java
 159c47ffbd71c6d08b563d8d28d5d7cdc4fb096b 
  
geode-core/src/test/java/org/apache/geode/management/internal/security/MemberMBeanSecurityJUnitTest.java
 65fd528641771e535f3d8d0d6601cef53f91af7a 
  
geode-core/src/test/java/org/apache/geode/security/PDXPostProcessorDUnitTest.java
 e9523862da9e045b05417dd8123574b01622c497 
  
geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java
 30ae59fd786b4753ae71849f81deeb0fe7f74c17 
  
geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/ShellCommandsControllerJUnitTest.java
 10e26f6c5d006856e9e88b06a60f5e67cb68a2ce 


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


Testing
---

- Precheckin running
 - Further cleanup of CommandManager is expected in a subsequent ticket


Thanks,

Jared Stewart