Re: Review Request 60985: GEODE-3217: Reimplement gfsh query as a single-step command

2017-07-20 Thread Ken Howe 

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


Ship it!




Ship It!

- Ken Howe


On July 20, 2017, 5:54 p.m., Jared Stewart wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60985/
> ---
> 
> (Updated July 20, 2017, 5:54 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-3217: Reimplement gfsh query as a single-step command
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java
>  3f4397b 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueryCommand.java
>  PRE-CREATION 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueryInterceptor.java
>  PRE-CREATION 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java
>  fe88fc9 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java
>  41cc171 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java
>  8ab7c93 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DataCommandsController.java
>  fb63184 
>   
> geode-core/src/test/java/org/apache/geode/management/DataCommandMBeanTest.java
>  e5d6ce8 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/QueryCommandTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/QueryCommandUnitTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/result/ResultBuilderTest.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60985/diff/2/
> 
> 
> Testing
> ---
> 
> Precheckin passed (except for some flaky failures that appear to be unrelated)
> 
> 
> Thanks,
> 
> Jared Stewart
> 
>

Re: Review Request 60985: GEODE-3217: Reimplement gfsh query as a single-step command

2017-07-20 Thread Jared Stewart 


> On July 20, 2017, 2:52 p.m., Ken Howe wrote:
> > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/QueryCommandUnitTest.java
> > Lines 24 (patched)
> > 
> >
> > Seems there's opportunity for more tests in here, for instance, 
> > queryWithInvalidRegionName, queryInvalidExceptionThrown, etc.
> > 
> > Have you checked coverage, in particular for the new classes 
> > QueryCommand, and QueryInterceptor

I've updated with a revision to add some tests for the sad paths as you 
suggested.  Coverage is as follows: QueryCommand (100% method, 89% line), 
QueryInterceptor (100% method, 92%line).  Most of the lines without coverage 
are things like an extra-cautious 
```
} catch (IOException e) { 
throw new RuntimeException(e);
}
```


- Jared


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


On July 20, 2017, 5:54 p.m., Jared Stewart wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60985/
> ---
> 
> (Updated July 20, 2017, 5:54 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-3217: Reimplement gfsh query as a single-step command
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java
>  3f4397b 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueryCommand.java
>  PRE-CREATION 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueryInterceptor.java
>  PRE-CREATION 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java
>  fe88fc9 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java
>  41cc171 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java
>  8ab7c93 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DataCommandsController.java
>  fb63184 
>   
> geode-core/src/test/java/org/apache/geode/management/DataCommandMBeanTest.java
>  e5d6ce8 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/QueryCommandTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/QueryCommandUnitTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/result/ResultBuilderTest.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60985/diff/2/
> 
> 
> Testing
> ---
> 
> Precheckin passed (except for some flaky failures that appear to be unrelated)
> 
> 
> Thanks,
> 
> Jared Stewart
> 
>

Re: Review Request 60985: GEODE-3217: Reimplement gfsh query as a single-step command

2017-07-20 Thread Jared Stewart 

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

(Updated July 20, 2017, 5:54 p.m.)


Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Changes
---

Add more tests for QueryCommand


Repository: geode


Description
---

GEODE-3217: Reimplement gfsh query as a single-step command


Diffs (updated)
-

  
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java
 3f4397b 
  
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueryCommand.java
 PRE-CREATION 
  
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueryInterceptor.java
 PRE-CREATION 
  
geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java
 fe88fc9 
  
geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java
 41cc171 
  
geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java
 8ab7c93 
  
geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DataCommandsController.java
 fb63184 
  
geode-core/src/test/java/org/apache/geode/management/DataCommandMBeanTest.java 
e5d6ce8 
  
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/QueryCommandTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/QueryCommandUnitTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/cli/result/ResultBuilderTest.java
 PRE-CREATION 


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

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


Testing
---

Precheckin passed (except for some flaky failures that appear to be unrelated)


Thanks,

Jared Stewart

Re: Review Request 60985: GEODE-3217: Reimplement gfsh query as a single-step command

2017-07-20 Thread Ken Howe 

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




geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueryCommand.java
Lines 15 (patched)


Extra blank line. Check this in all of the new class files.



geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/QueryCommandUnitTest.java
Lines 24 (patched)


Seems there's opportunity for more tests in here, for instance, 
queryWithInvalidRegionName, queryInvalidExceptionThrown, etc.

Have you checked coverage, in particular for the new classes QueryCommand, 
and QueryInterceptor


- Ken Howe


On July 19, 2017, 9:06 p.m., Jared Stewart wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60985/
> ---
> 
> (Updated July 19, 2017, 9:06 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-3217: Reimplement gfsh query as a single-step command
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java
>  3f4397b3d025ee5453a6df338eed41589ff339c0 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueryCommand.java
>  PRE-CREATION 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueryInterceptor.java
>  PRE-CREATION 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java
>  fe88fc98d397fa748f8f54b900b1511eb114c0b6 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java
>  41cc171dbabee2cf51a9e28d051b8365a3c44699 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java
>  8ab7c9342ebd5370deba45a35dbf68201f2b7333 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DataCommandsController.java
>  fb6318449e027c94ba4fd2fa87c84f8181ce61a4 
>   
> geode-core/src/test/java/org/apache/geode/management/DataCommandMBeanTest.java
>  e5d6ce8d5bfb35eb503c66c9650fde2a33a51315 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/QueryCommandTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/QueryCommandUnitTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/result/ResultBuilderTest.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60985/diff/1/
> 
> 
> Testing
> ---
> 
> Precheckin passed (except for some flaky failures that appear to be unrelated)
> 
> 
> Thanks,
> 
> Jared Stewart
> 
>

Re: Review Request 60985: GEODE-3217: Reimplement gfsh query as a single-step command

2017-07-19 Thread Jinmei Liao 

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


Ship it!




Ship It!

- Jinmei Liao


On July 19, 2017, 9:06 p.m., Jared Stewart wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60985/
> ---
> 
> (Updated July 19, 2017, 9:06 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-3217: Reimplement gfsh query as a single-step command
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java
>  3f4397b3d025ee5453a6df338eed41589ff339c0 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueryCommand.java
>  PRE-CREATION 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueryInterceptor.java
>  PRE-CREATION 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java
>  fe88fc98d397fa748f8f54b900b1511eb114c0b6 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java
>  41cc171dbabee2cf51a9e28d051b8365a3c44699 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java
>  8ab7c9342ebd5370deba45a35dbf68201f2b7333 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DataCommandsController.java
>  fb6318449e027c94ba4fd2fa87c84f8181ce61a4 
>   
> geode-core/src/test/java/org/apache/geode/management/DataCommandMBeanTest.java
>  e5d6ce8d5bfb35eb503c66c9650fde2a33a51315 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/QueryCommandTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/QueryCommandUnitTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/result/ResultBuilderTest.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60985/diff/1/
> 
> 
> Testing
> ---
> 
> Precheckin passed (except for some flaky failures that appear to be unrelated)
> 
> 
> Thanks,
> 
> Jared Stewart
> 
>