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/04/24 20:05:51 UTC

Review Request 58682: GEODE-2662: Added unittests to capture failing behavior.

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

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

Repository: geode

Description
-------

GEODE-2662: Corrected buildTable shifting columns when adding values with additional keys.

Refactoring of DataCommandResult and DataCommandFunction

------

The majority of changes to DataCommandFunction and DataCommandResult are refactoring. Two ignored exceptions have been explicitly noted in the logger.

- In DataCommandFunction:423, there's an empty if block. I imagine I should remove that, since empty code is a waste. Looking for it, I couldn't find the comment-hinted separate method, though. Anyone familiar with this corner of the code know if that actuall happens?

- Qualitative changes are in DataCommandResult.buildTable. Items in the table are scanned to build an agggregate key set, and those items missing any of these keys are padded with `MISSING_VALUE`.

- I moved some of the more cumbersome blocks of code to subfunctions, but may have done this more than necessary. Opinions?

- Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive development / note the bug in GEODE-2662. Are there additional tests that would fit naturally in this file?

Diffs
-----

geode-core/build.gradle f07444af7baa41fc0fcd0a088521bd96b317d15f
geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java 423d781248458a450c2e98cd8a0e609c8656edb0
geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java bb77466962a9a07a48bc09976b94d2f3822aed6d
geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java e72654ed4aa11a5eb3f69a275d253cbedeb7df1e
geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryDataInconsistencyDUnitTest.java 1af6261613f51a7cc3afe49bf8f3e2785ed2d60d
geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java PRE-CREATION

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

Testing
-------

precheckin currently running.

Thanks,

Patrick Rhomberg

Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.

Posted by Kirk Lund <ki...@gmail.com>.

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




geode-core/build.gradle
Lines 118 (patched)
<https://reviews.apache.org/r/58682/#comment245890>

    There are one or two tests (probably in geode-assembly) that might fail if you've introduced a new product runtime library dependency here without altering geode-assembly to expect the new jar to show up in our product lib directory. You might even want to mention adding guava as a runtime dependency on geode dev to see what others think. I'm ok with adding guava (which was previously a test-only dependency).


- Kirk Lund


On April 24, 2017, 8:06 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58682/
> -----------------------------------------------------------
> 
> (Updated April 24, 2017, 8:06 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Added unittests to capture failing behavior.
> 
> Corrected buildTable shifting columns when adding values with additional keys.
> 
> 
> Refactoring of DataCommandResult and DataCommandFunction
> 
> ------
> 
> The majority of changes to DataCommandFunction and DataCommandResult are refactoring.  Two ignored exceptions have been explicitly noted in the logger.
> 
> - In DataCommandFunction:423, there's an empty if block.  I imagine I should remove that, since empty code is a waste.  Looking for it, I couldn't find the comment-hinted separate method, though. Anyone familiar with this corner of the code know if that actuall happens?
> 
> - Qualitative changes are in DataCommandResult.buildTable.  Items in the table are scanned to build an agggregate key set, and those items missing any of these keys are padded with `MISSING_VALUE`.
> 
> - I moved some of the more cumbersome blocks of code to subfunctions, but may have done this more than necessary.  Opinions?
> 
> - Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive development / note the bug in GEODE-2662.  Are there additional tests that would fit naturally in this file?
> 
> 
> Diffs
> -----
> 
>   geode-core/build.gradle f07444af7baa41fc0fcd0a088521bd96b317d15f 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java 423d781248458a450c2e98cd8a0e609c8656edb0 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java bb77466962a9a07a48bc09976b94d2f3822aed6d 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java e72654ed4aa11a5eb3f69a275d253cbedeb7df1e 
>   geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryDataInconsistencyDUnitTest.java 1af6261613f51a7cc3afe49bf8f3e2785ed2d60d 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/58682/diff/1/
> 
> 
> Testing
> -------
> 
> precheckin currently running.
> 
> DistributedTest still pending.  All others returned green.
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>

Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.

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

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


Ship it!




Ship It!

- Ken Howe


On April 27, 2017, 6:30 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58682/
> -----------------------------------------------------------
> 
> (Updated April 27, 2017, 6:30 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Added unittests to capture failing behavior.
> 
> Corrected buildTable shifting columns when adding values with additional keys.
> 
> 
> Refactoring of DataCommandResult and DataCommandFunction
> 
> ------
> 
> The majority of changes to DataCommandFunction and DataCommandResult are refactoring.  Two ignored exceptions have been explicitly noted in the logger.
> 
> - In DataCommandFunction:423, there's an empty if block.  I imagine I should remove that, since empty code is a waste.  Looking for it, I couldn't find the comment-hinted separate method, though. Anyone familiar with this corner of the code know if that actuall happens?
> 
> - Qualitative changes are in DataCommandResult.buildTable.  Items in the table are scanned to build an agggregate key set, and those items missing any of these keys are padded with `MISSING_VALUE`.
> 
> - I moved some of the more cumbersome blocks of code to subfunctions, but may have done this more than necessary.  Opinions?
> 
> - Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive development / note the bug in GEODE-2662.  Are there additional tests that would fit naturally in this file?
> 
> 
> Diffs
> -----
> 
>   geode-core/build.gradle f07444a 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java 6324b5c 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java 423d781 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java bb77466 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java e72654e 
>   geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryDataInconsistencyDUnitTest.java 1af6261 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java ead1047 
> 
> 
> Diff: https://reviews.apache.org/r/58682/diff/5/
> 
> 
> Testing
> -------
> 
> Previous precheckin returned fully green.  Running with new refactoring but expect no issues.  (Common last words.)
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>

Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.

Posted by Kirk Lund <ki...@gmail.com>.

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


Ship it!




Ship It!

- Kirk Lund


On April 27, 2017, 6:30 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58682/
> -----------------------------------------------------------
> 
> (Updated April 27, 2017, 6:30 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Added unittests to capture failing behavior.
> 
> Corrected buildTable shifting columns when adding values with additional keys.
> 
> 
> Refactoring of DataCommandResult and DataCommandFunction
> 
> ------
> 
> The majority of changes to DataCommandFunction and DataCommandResult are refactoring.  Two ignored exceptions have been explicitly noted in the logger.
> 
> - In DataCommandFunction:423, there's an empty if block.  I imagine I should remove that, since empty code is a waste.  Looking for it, I couldn't find the comment-hinted separate method, though. Anyone familiar with this corner of the code know if that actuall happens?
> 
> - Qualitative changes are in DataCommandResult.buildTable.  Items in the table are scanned to build an agggregate key set, and those items missing any of these keys are padded with `MISSING_VALUE`.
> 
> - I moved some of the more cumbersome blocks of code to subfunctions, but may have done this more than necessary.  Opinions?
> 
> - Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive development / note the bug in GEODE-2662.  Are there additional tests that would fit naturally in this file?
> 
> 
> Diffs
> -----
> 
>   geode-core/build.gradle f07444a 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java 6324b5c 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java 423d781 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java bb77466 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java e72654e 
>   geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryDataInconsistencyDUnitTest.java 1af6261 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java ead1047 
> 
> 
> Diff: https://reviews.apache.org/r/58682/diff/5/
> 
> 
> Testing
> -------
> 
> Previous precheckin returned fully green.  Running with new refactoring but expect no issues.  (Common last words.)
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>

Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.

Posted by Kirk Lund <ki...@gmail.com>.


> On May 5, 2017, 10:03 p.m., Ken Howe wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java
> > Line 91 (original)
> > <https://reviews.apache.org/r/58682/diff/5-7/?file=1702040#file1702040line94>
> >
> >     We should be using log4j levels, so restore this line. All of the "fineEnabled" and "logger.fine" should be "logger.isTraceEnabled" and "logger.trace"
> >     
> >     Then remove all the local declarations of:
> >     
> >        LogWriter logger = cache.getLogger();

"fine" translates to isDebugEnabled and logger.debug

"finer" and "finest" both translate to isTraceEnabled and logger.trace


- Kirk


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


On May 5, 2017, 9:22 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58682/
> -----------------------------------------------------------
> 
> (Updated May 5, 2017, 9:22 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Added unittests to capture failing behavior.
> 
> Corrected buildTable shifting columns when adding values with additional keys.
> 
> 
> Refactoring of DataCommandResult and DataCommandFunction
> 
> ------
> 
> The majority of changes to DataCommandFunction and DataCommandResult are refactoring.  Two ignored exceptions have been explicitly noted in the logger.
> 
> - In DataCommandFunction:423, there's an empty if block.  I imagine I should remove that, since empty code is a waste.  Looking for it, I couldn't find the comment-hinted separate method, though. Anyone familiar with this corner of the code know if that actuall happens?
> 
> - Qualitative changes are in DataCommandResult.buildTable.  Items in the table are scanned to build an agggregate key set, and those items missing any of these keys are padded with `MISSING_VALUE`.
> 
> - I moved some of the more cumbersome blocks of code to subfunctions, but may have done this more than necessary.  Opinions?
> 
> - Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive development / note the bug in GEODE-2662.  Are there additional tests that would fit naturally in this file?
> 
> 
> Diffs
> -----
> 
>   geode-core/build.gradle f07444a 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java 29d68bd 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java 423d781 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java bb77466 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java e72654e 
>   geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryDataInconsistencyDUnitTest.java 1af6261 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/GemfireDataCommandsDUnitTest.java 0e2a41e 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java ead1047 
> 
> 
> Diff: https://reviews.apache.org/r/58682/diff/7/
> 
> 
> Testing
> -------
> 
> Previous precheckin returned fully green.  Running with new refactoring but expect no issues.  (Common last words.)
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>

Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.

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


> On May 5, 2017, 10:03 p.m., Ken Howe wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java
> > Line 91 (original)
> > <https://reviews.apache.org/r/58682/diff/5-7/?file=1702040#file1702040line94>
> >
> >     We should be using log4j levels, so restore this line. All of the "fineEnabled" and "logger.fine" should be "logger.isTraceEnabled" and "logger.trace"
> >     
> >     Then remove all the local declarations of:
> >     
> >        LogWriter logger = cache.getLogger();
> 
> Kirk Lund wrote:
>     "fine" translates to isDebugEnabled and logger.debug
>     
>     "finer" and "finest" both translate to isTraceEnabled and logger.trace

I stand corrected! I should have looked up the level translations instead of making a comment from memory.


- Ken


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


On May 5, 2017, 9:22 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58682/
> -----------------------------------------------------------
> 
> (Updated May 5, 2017, 9:22 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Added unittests to capture failing behavior.
> 
> Corrected buildTable shifting columns when adding values with additional keys.
> 
> 
> Refactoring of DataCommandResult and DataCommandFunction
> 
> ------
> 
> The majority of changes to DataCommandFunction and DataCommandResult are refactoring.  Two ignored exceptions have been explicitly noted in the logger.
> 
> - In DataCommandFunction:423, there's an empty if block.  I imagine I should remove that, since empty code is a waste.  Looking for it, I couldn't find the comment-hinted separate method, though. Anyone familiar with this corner of the code know if that actuall happens?
> 
> - Qualitative changes are in DataCommandResult.buildTable.  Items in the table are scanned to build an agggregate key set, and those items missing any of these keys are padded with `MISSING_VALUE`.
> 
> - I moved some of the more cumbersome blocks of code to subfunctions, but may have done this more than necessary.  Opinions?
> 
> - Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive development / note the bug in GEODE-2662.  Are there additional tests that would fit naturally in this file?
> 
> 
> Diffs
> -----
> 
>   geode-core/build.gradle f07444a 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java 29d68bd 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java 423d781 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java bb77466 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java e72654e 
>   geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryDataInconsistencyDUnitTest.java 1af6261 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/GemfireDataCommandsDUnitTest.java 0e2a41e 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java ead1047 
> 
> 
> Diff: https://reviews.apache.org/r/58682/diff/7/
> 
> 
> Testing
> -------
> 
> Previous precheckin returned fully green.  Running with new refactoring but expect no issues.  (Common last words.)
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>

Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.

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

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




geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java
Line 91 (original)
<https://reviews.apache.org/r/58682/#comment247196>

    We should be using log4j levels, so restore this line. All of the "fineEnabled" and "logger.fine" should be "logger.isTraceEnabled" and "logger.trace"
    
    Then remove all the local declarations of:
    
       LogWriter logger = cache.getLogger();


- Ken Howe


On May 5, 2017, 9:22 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58682/
> -----------------------------------------------------------
> 
> (Updated May 5, 2017, 9:22 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Added unittests to capture failing behavior.
> 
> Corrected buildTable shifting columns when adding values with additional keys.
> 
> 
> Refactoring of DataCommandResult and DataCommandFunction
> 
> ------
> 
> The majority of changes to DataCommandFunction and DataCommandResult are refactoring.  Two ignored exceptions have been explicitly noted in the logger.
> 
> - In DataCommandFunction:423, there's an empty if block.  I imagine I should remove that, since empty code is a waste.  Looking for it, I couldn't find the comment-hinted separate method, though. Anyone familiar with this corner of the code know if that actuall happens?
> 
> - Qualitative changes are in DataCommandResult.buildTable.  Items in the table are scanned to build an agggregate key set, and those items missing any of these keys are padded with `MISSING_VALUE`.
> 
> - I moved some of the more cumbersome blocks of code to subfunctions, but may have done this more than necessary.  Opinions?
> 
> - Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive development / note the bug in GEODE-2662.  Are there additional tests that would fit naturally in this file?
> 
> 
> Diffs
> -----
> 
>   geode-core/build.gradle f07444a 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java 29d68bd 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java 423d781 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java bb77466 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java e72654e 
>   geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryDataInconsistencyDUnitTest.java 1af6261 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/GemfireDataCommandsDUnitTest.java 0e2a41e 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java ead1047 
> 
> 
> Diff: https://reviews.apache.org/r/58682/diff/7/
> 
> 
> Testing
> -------
> 
> Previous precheckin returned fully green.  Running with new refactoring but expect no issues.  (Common last words.)
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>

Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.

Posted by Patrick Rhomberg <pr...@pivotal.io>.

(Updated May 5, 2017, 9:22 p.m.)

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

Changes
-------

A couple critical "cleanups" went from (x != null ...) to (Util.isEmpty(x)). Whoops.

Repository: geode

Description
-------

Added unittests to capture failing behavior.

Corrected buildTable shifting columns when adding values with additional keys.

Refactoring of DataCommandResult and DataCommandFunction

------

The majority of changes to DataCommandFunction and DataCommandResult are refactoring. Two ignored exceptions have been explicitly noted in the logger.

- Qualitative changes are in DataCommandResult.buildTable. Items in the table are scanned to build an agggregate key set, and those items missing any of these keys are padded with `MISSING_VALUE`.

- I moved some of the more cumbersome blocks of code to subfunctions, but may have done this more than necessary. Opinions?

- Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive development / note the bug in GEODE-2662. Are there additional tests that would fit naturally in this file?

Diffs (updated)
-----

geode-core/build.gradle f07444a
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java 29d68bd
geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java 423d781
geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java bb77466
geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java e72654e
geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryDataInconsistencyDUnitTest.java 1af6261
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/GemfireDataCommandsDUnitTest.java 0e2a41e
geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java PRE-CREATION
geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java ead1047

Diff: https://reviews.apache.org/r/58682/diff/7/

Changes: https://reviews.apache.org/r/58682/diff/6-7/

Testing
-------

Previous precheckin returned fully green. Running with new refactoring but expect no issues. (Common last words.)

Thanks,

Patrick Rhomberg

Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.

Posted by Jared Stewart <js...@pivotal.io>.

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


Ship it!




Ship It!

- Jared Stewart


On April 27, 2017, 6:30 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58682/
> -----------------------------------------------------------
> 
> (Updated April 27, 2017, 6:30 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Added unittests to capture failing behavior.
> 
> Corrected buildTable shifting columns when adding values with additional keys.
> 
> 
> Refactoring of DataCommandResult and DataCommandFunction
> 
> ------
> 
> The majority of changes to DataCommandFunction and DataCommandResult are refactoring.  Two ignored exceptions have been explicitly noted in the logger.
> 
> - In DataCommandFunction:423, there's an empty if block.  I imagine I should remove that, since empty code is a waste.  Looking for it, I couldn't find the comment-hinted separate method, though. Anyone familiar with this corner of the code know if that actuall happens?
> 
> - Qualitative changes are in DataCommandResult.buildTable.  Items in the table are scanned to build an agggregate key set, and those items missing any of these keys are padded with `MISSING_VALUE`.
> 
> - I moved some of the more cumbersome blocks of code to subfunctions, but may have done this more than necessary.  Opinions?
> 
> - Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive development / note the bug in GEODE-2662.  Are there additional tests that would fit naturally in this file?
> 
> 
> Diffs
> -----
> 
>   geode-core/build.gradle f07444a 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java 6324b5c 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java 423d781 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java bb77466 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java e72654e 
>   geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryDataInconsistencyDUnitTest.java 1af6261 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java ead1047 
> 
> 
> Diff: https://reviews.apache.org/r/58682/diff/5/
> 
> 
> Testing
> -------
> 
> Previous precheckin returned fully green.  Running with new refactoring but expect no issues.  (Common last words.)
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>

Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.

Posted by Kirk Lund <ki...@gmail.com>.

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




geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java
Line 167 (original), 170 (patched)
<https://reviews.apache.org/r/58682/#comment246298>

    Delete these two unused HashSets. I know it's not part of your change, but yuck this is bad!


- Kirk Lund


On April 27, 2017, 6:30 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58682/
> -----------------------------------------------------------
> 
> (Updated April 27, 2017, 6:30 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Added unittests to capture failing behavior.
> 
> Corrected buildTable shifting columns when adding values with additional keys.
> 
> 
> Refactoring of DataCommandResult and DataCommandFunction
> 
> ------
> 
> The majority of changes to DataCommandFunction and DataCommandResult are refactoring.  Two ignored exceptions have been explicitly noted in the logger.
> 
> - In DataCommandFunction:423, there's an empty if block.  I imagine I should remove that, since empty code is a waste.  Looking for it, I couldn't find the comment-hinted separate method, though. Anyone familiar with this corner of the code know if that actuall happens?
> 
> - Qualitative changes are in DataCommandResult.buildTable.  Items in the table are scanned to build an agggregate key set, and those items missing any of these keys are padded with `MISSING_VALUE`.
> 
> - I moved some of the more cumbersome blocks of code to subfunctions, but may have done this more than necessary.  Opinions?
> 
> - Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive development / note the bug in GEODE-2662.  Are there additional tests that would fit naturally in this file?
> 
> 
> Diffs
> -----
> 
>   geode-core/build.gradle f07444a 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java 6324b5c 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java 423d781 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java bb77466 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java e72654e 
>   geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryDataInconsistencyDUnitTest.java 1af6261 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java ead1047 
> 
> 
> Diff: https://reviews.apache.org/r/58682/diff/5/
> 
> 
> Testing
> -------
> 
> Previous precheckin returned fully green.  Running with new refactoring but expect no issues.  (Common last words.)
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>

Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.

Posted by Kirk Lund <ki...@gmail.com>.

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


Ship it!





geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java
Lines 1279 (patched)
<https://reviews.apache.org/r/58682/#comment247012>

    I usually line-feed the comment and then put the open curly on the end of the if-line. I'm guessing that this might fail spotless check?


- Kirk Lund


On April 27, 2017, 6:30 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58682/
> -----------------------------------------------------------
> 
> (Updated April 27, 2017, 6:30 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Added unittests to capture failing behavior.
> 
> Corrected buildTable shifting columns when adding values with additional keys.
> 
> 
> Refactoring of DataCommandResult and DataCommandFunction
> 
> ------
> 
> The majority of changes to DataCommandFunction and DataCommandResult are refactoring.  Two ignored exceptions have been explicitly noted in the logger.
> 
> - In DataCommandFunction:423, there's an empty if block.  I imagine I should remove that, since empty code is a waste.  Looking for it, I couldn't find the comment-hinted separate method, though. Anyone familiar with this corner of the code know if that actuall happens?
> 
> - Qualitative changes are in DataCommandResult.buildTable.  Items in the table are scanned to build an agggregate key set, and those items missing any of these keys are padded with `MISSING_VALUE`.
> 
> - I moved some of the more cumbersome blocks of code to subfunctions, but may have done this more than necessary.  Opinions?
> 
> - Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive development / note the bug in GEODE-2662.  Are there additional tests that would fit naturally in this file?
> 
> 
> Diffs
> -----
> 
>   geode-core/build.gradle f07444a 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java 29d68bd 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java 423d781 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java bb77466 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java e72654e 
>   geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryDataInconsistencyDUnitTest.java 1af6261 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/GemfireDataCommandsDUnitTest.java 0e2a41e 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java ead1047 
> 
> 
> Diff: https://reviews.apache.org/r/58682/diff/6/
> 
> 
> Testing
> -------
> 
> Previous precheckin returned fully green.  Running with new refactoring but expect no issues.  (Common last words.)
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>

Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.

Posted by Patrick Rhomberg <pr...@pivotal.io>.

(Updated April 27, 2017, 6:30 p.m.)

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

Changes
-------

Last little tune-up revisions. precheckin running, but I don't expect any problems.

Repository: geode

Description
-------

Added unittests to capture failing behavior.

Corrected buildTable shifting columns when adding values with additional keys.

Refactoring of DataCommandResult and DataCommandFunction

------

The majority of changes to DataCommandFunction and DataCommandResult are refactoring. Two ignored exceptions have been explicitly noted in the logger.

- Qualitative changes are in DataCommandResult.buildTable. Items in the table are scanned to build an agggregate key set, and those items missing any of these keys are padded with `MISSING_VALUE`.

- I moved some of the more cumbersome blocks of code to subfunctions, but may have done this more than necessary. Opinions?

- Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive development / note the bug in GEODE-2662. Are there additional tests that would fit naturally in this file?

Diffs (updated)
-----

geode-core/build.gradle f07444a
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java 6324b5c
geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java 423d781
geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java bb77466
geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java e72654e
geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryDataInconsistencyDUnitTest.java 1af6261
geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java PRE-CREATION
geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java ead1047

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

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

Testing
-------

Previous precheckin returned fully green. Running with new refactoring but expect no issues. (Common last words.)

Thanks,

Patrick Rhomberg

Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.

Posted by Patrick Rhomberg <pr...@pivotal.io>.

(Updated April 26, 2017, 6:25 p.m.)

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

Repository: geode

Description
-------

Added unittests to capture failing behavior.

Corrected buildTable shifting columns when adding values with additional keys.

Refactoring of DataCommandResult and DataCommandFunction

------

The majority of changes to DataCommandFunction and DataCommandResult are refactoring. Two ignored exceptions have been explicitly noted in the logger.

- Qualitative changes are in DataCommandResult.buildTable. Items in the table are scanned to build an agggregate key set, and those items missing any of these keys are padded with `MISSING_VALUE`.

- I moved some of the more cumbersome blocks of code to subfunctions, but may have done this more than necessary. Opinions?

- Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive development / note the bug in GEODE-2662. Are there additional tests that would fit naturally in this file?

Diffs (updated)
-----

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

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

Testing
-------

precheckin currently running.

DistributedTest still pending. All others returned green.

Thanks,

Patrick Rhomberg

Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.

Posted by Kirk Lund <ki...@gmail.com>.

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




geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java
Lines 269 (patched)
<https://reviews.apache.org/r/58682/#comment245886>

    You should delete the printStackTrace. You might consider logging this first exception.
    
    In general, it's best to avoid nested try-catch blocks as well. Looks like it's maybe required in this case though? I suppose you could set a flag and then do create the new GfJsonObject after the catch-block if the flag is true. I don't feel strongly either way but it's not typical to it this way.



geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java
Line 263 (original)
<https://reviews.apache.org/r/58682/#comment245888>

    Log4J 2 unfortunately implemented things in such a way that log statements below the configured log level results in significant impact to performance.
    
    In the Geode code base we wrap all Loggers inside an instance of FastLogger. The FastLogger uses a single volatile to disable lower log levels when log level is below info (and no filters are configured) but it only works if you keep the isDebugEnabled() checks in place.
    
    If log level is INFO or above then FastLogger help minimize any performance impact. Without it, Geode throughput is decreased substantially by Log4J 2 even at log levels above DEBUG simply by hitting logger.debug statements that are not enabled.


- Kirk Lund


On April 24, 2017, 8:06 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58682/
> -----------------------------------------------------------
> 
> (Updated April 24, 2017, 8:06 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Added unittests to capture failing behavior.
> 
> Corrected buildTable shifting columns when adding values with additional keys.
> 
> 
> Refactoring of DataCommandResult and DataCommandFunction
> 
> ------
> 
> The majority of changes to DataCommandFunction and DataCommandResult are refactoring.  Two ignored exceptions have been explicitly noted in the logger.
> 
> - In DataCommandFunction:423, there's an empty if block.  I imagine I should remove that, since empty code is a waste.  Looking for it, I couldn't find the comment-hinted separate method, though. Anyone familiar with this corner of the code know if that actuall happens?
> 
> - Qualitative changes are in DataCommandResult.buildTable.  Items in the table are scanned to build an agggregate key set, and those items missing any of these keys are padded with `MISSING_VALUE`.
> 
> - I moved some of the more cumbersome blocks of code to subfunctions, but may have done this more than necessary.  Opinions?
> 
> - Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive development / note the bug in GEODE-2662.  Are there additional tests that would fit naturally in this file?
> 
> 
> Diffs
> -----
> 
>   geode-core/build.gradle f07444af7baa41fc0fcd0a088521bd96b317d15f 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java 423d781248458a450c2e98cd8a0e609c8656edb0 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java bb77466962a9a07a48bc09976b94d2f3822aed6d 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java e72654ed4aa11a5eb3f69a275d253cbedeb7df1e 
>   geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryDataInconsistencyDUnitTest.java 1af6261613f51a7cc3afe49bf8f3e2785ed2d60d 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/58682/diff/1/
> 
> 
> Testing
> -------
> 
> precheckin currently running.
> 
> DistributedTest still pending.  All others returned green.
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>

Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.

Posted by Jared Stewart <js...@pivotal.io>.

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




geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java
Line 920 (original), 893 (patched)
<https://reviews.apache.org/r/58682/#comment246057>

    `StringUtils.isEmpty()` again on lines 387 and 391.



geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java
Line 386 (original), 371 (patched)
<https://reviews.apache.org/r/58682/#comment246046>

    I think this would read more clearly as 
    ```
        if (StringUtils.isEmpty(removeAllKeys) && key == null) {
    ```



geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java
Line 682 (original), 680 (patched)
<https://reviews.apache.org/r/58682/#comment246048>

    This can be simplified with `StringUtils` as above.



geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java
Line 832 (original), 816 (patched)
<https://reviews.apache.org/r/58682/#comment246051>

    We can avoid the need to explicitly cast `region1` by including generic type parameters:
    
    ```
        // get a list of all root regions
        Set<Region<?,?>> regions = cache.rootRegions();
    
        for (Region<?,?> region1 : regions) {
          String regionPath = region1.getFullPath();
    ```
    
    Also, super small nitpick, but I think `rootRegion` would be a more descriptive name than `region1`.



geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java
Line 840 (original), 823 (patched)
<https://reviews.apache.org/r/58682/#comment246053>

    We can do the same thing with generics here: 
    
    ```
    Set<Region<?,?>> subregionSet = region.subregions(true);
          if (recursive) {
            for (Region aSubregionSet : subregionSet) {
              list.add( aSubregionSet.getFullPath());
            }
          }
    ```


- Jared Stewart


On April 24, 2017, 8:06 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58682/
> -----------------------------------------------------------
> 
> (Updated April 24, 2017, 8:06 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Added unittests to capture failing behavior.
> 
> Corrected buildTable shifting columns when adding values with additional keys.
> 
> 
> Refactoring of DataCommandResult and DataCommandFunction
> 
> ------
> 
> The majority of changes to DataCommandFunction and DataCommandResult are refactoring.  Two ignored exceptions have been explicitly noted in the logger.
> 
> - In DataCommandFunction:423, there's an empty if block.  I imagine I should remove that, since empty code is a waste.  Looking for it, I couldn't find the comment-hinted separate method, though. Anyone familiar with this corner of the code know if that actuall happens?
> 
> - Qualitative changes are in DataCommandResult.buildTable.  Items in the table are scanned to build an agggregate key set, and those items missing any of these keys are padded with `MISSING_VALUE`.
> 
> - I moved some of the more cumbersome blocks of code to subfunctions, but may have done this more than necessary.  Opinions?
> 
> - Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive development / note the bug in GEODE-2662.  Are there additional tests that would fit naturally in this file?
> 
> 
> Diffs
> -----
> 
>   geode-core/build.gradle f07444a 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java 6324b5c 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java 423d781 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java bb77466 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java e72654e 
>   geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryDataInconsistencyDUnitTest.java 1af6261 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java 0e65354 
> 
> 
> Diff: https://reviews.apache.org/r/58682/diff/3/
> 
> 
> Testing
> -------
> 
> precheckin currently running.
> 
> DistributedTest still pending.  All others returned green.
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>

Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.

Posted by Kirk Lund <ki...@gmail.com>.


> On April 25, 2017, 9:10 p.m., Kirk Lund wrote:
> > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java
> > Lines 154 (patched)
> > <https://reviews.apache.org/r/58682/diff/3/?file=1699792#file1699792line154>
> >
> >     Usually you can use this flavor:
> >     ```java
> >     assertThat(tableJson.getJSONArray(k)).hasSize(4);
> >     ```
> 
> Patrick Rhomberg wrote:
>     Apparently no `hasSize` for JSONArray.

Oops! Yeah that's for Collections and arrays.


- Kirk


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


On April 26, 2017, 6:25 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58682/
> -----------------------------------------------------------
> 
> (Updated April 26, 2017, 6:25 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Added unittests to capture failing behavior.
> 
> Corrected buildTable shifting columns when adding values with additional keys.
> 
> 
> Refactoring of DataCommandResult and DataCommandFunction
> 
> ------
> 
> The majority of changes to DataCommandFunction and DataCommandResult are refactoring.  Two ignored exceptions have been explicitly noted in the logger.
> 
> - In DataCommandFunction:423, there's an empty if block.  I imagine I should remove that, since empty code is a waste.  Looking for it, I couldn't find the comment-hinted separate method, though. Anyone familiar with this corner of the code know if that actuall happens?
> 
> - Qualitative changes are in DataCommandResult.buildTable.  Items in the table are scanned to build an agggregate key set, and those items missing any of these keys are padded with `MISSING_VALUE`.
> 
> - I moved some of the more cumbersome blocks of code to subfunctions, but may have done this more than necessary.  Opinions?
> 
> - Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive development / note the bug in GEODE-2662.  Are there additional tests that would fit naturally in this file?
> 
> 
> Diffs
> -----
> 
>   geode-core/build.gradle f07444a 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java 6324b5c 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java 423d781 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java bb77466 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java e72654e 
>   geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryDataInconsistencyDUnitTest.java 1af6261 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java ead1047 
> 
> 
> Diff: https://reviews.apache.org/r/58682/diff/4/
> 
> 
> Testing
> -------
> 
> Previous precheckin returned fully green.  Running with new refactoring but expect no issues.  (Common last words.)
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>

Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.

Posted by Kirk Lund <ki...@gmail.com>.


> On April 25, 2017, 9:10 p.m., Kirk Lund wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java
> > Line 301 (original), 317 (patched)
> > <https://reviews.apache.org/r/58682/diff/3/?file=1699789#file1699789line359>
> >
> >     Should probably lower this below FATAL. WARN might be a good log level.
> >     
> >     I think we've generally said that FATAL means the cache server is DOA and cannot recover or may lose data.
> 
> Patrick Rhomberg wrote:
>     What do you think about using ERROR, since the command in this case will not execute as expected?

ERROR should be fine as well.


- Kirk


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


On April 26, 2017, 6:25 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58682/
> -----------------------------------------------------------
> 
> (Updated April 26, 2017, 6:25 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Added unittests to capture failing behavior.
> 
> Corrected buildTable shifting columns when adding values with additional keys.
> 
> 
> Refactoring of DataCommandResult and DataCommandFunction
> 
> ------
> 
> The majority of changes to DataCommandFunction and DataCommandResult are refactoring.  Two ignored exceptions have been explicitly noted in the logger.
> 
> - In DataCommandFunction:423, there's an empty if block.  I imagine I should remove that, since empty code is a waste.  Looking for it, I couldn't find the comment-hinted separate method, though. Anyone familiar with this corner of the code know if that actuall happens?
> 
> - Qualitative changes are in DataCommandResult.buildTable.  Items in the table are scanned to build an agggregate key set, and those items missing any of these keys are padded with `MISSING_VALUE`.
> 
> - I moved some of the more cumbersome blocks of code to subfunctions, but may have done this more than necessary.  Opinions?
> 
> - Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive development / note the bug in GEODE-2662.  Are there additional tests that would fit naturally in this file?
> 
> 
> Diffs
> -----
> 
>   geode-core/build.gradle f07444a 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java 6324b5c 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java 423d781 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java bb77466 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java e72654e 
>   geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryDataInconsistencyDUnitTest.java 1af6261 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java ead1047 
> 
> 
> Diff: https://reviews.apache.org/r/58682/diff/4/
> 
> 
> Testing
> -------
> 
> Previous precheckin returned fully green.  Running with new refactoring but expect no issues.  (Common last words.)
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>

Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.

Posted by Patrick Rhomberg <pr...@pivotal.io>.


> On April 25, 2017, 9:10 p.m., Kirk Lund wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java
> > Line 301 (original), 317 (patched)
> > <https://reviews.apache.org/r/58682/diff/3/?file=1699789#file1699789line359>
> >
> >     Should probably lower this below FATAL. WARN might be a good log level.
> >     
> >     I think we've generally said that FATAL means the cache server is DOA and cannot recover or may lose data.

What do you think about using ERROR, since the command in this case will not execute as expected?


- Patrick


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


On April 24, 2017, 8:06 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58682/
> -----------------------------------------------------------
> 
> (Updated April 24, 2017, 8:06 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Added unittests to capture failing behavior.
> 
> Corrected buildTable shifting columns when adding values with additional keys.
> 
> 
> Refactoring of DataCommandResult and DataCommandFunction
> 
> ------
> 
> The majority of changes to DataCommandFunction and DataCommandResult are refactoring.  Two ignored exceptions have been explicitly noted in the logger.
> 
> - In DataCommandFunction:423, there's an empty if block.  I imagine I should remove that, since empty code is a waste.  Looking for it, I couldn't find the comment-hinted separate method, though. Anyone familiar with this corner of the code know if that actuall happens?
> 
> - Qualitative changes are in DataCommandResult.buildTable.  Items in the table are scanned to build an agggregate key set, and those items missing any of these keys are padded with `MISSING_VALUE`.
> 
> - I moved some of the more cumbersome blocks of code to subfunctions, but may have done this more than necessary.  Opinions?
> 
> - Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive development / note the bug in GEODE-2662.  Are there additional tests that would fit naturally in this file?
> 
> 
> Diffs
> -----
> 
>   geode-core/build.gradle f07444a 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java 6324b5c 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java 423d781 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java bb77466 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java e72654e 
>   geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryDataInconsistencyDUnitTest.java 1af6261 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java 0e65354 
> 
> 
> Diff: https://reviews.apache.org/r/58682/diff/3/
> 
> 
> Testing
> -------
> 
> precheckin currently running.
> 
> DistributedTest still pending.  All others returned green.
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>

Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.

Posted by Kirk Lund <ki...@gmail.com>.

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



I left "Open an Issue" unchecked for several of these comments which are kind of loose guidelines or whatever. Feel free to fix or ignore those.


geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java
Line 503 (original), 491 (patched)
<https://reviews.apache.org/r/58682/#comment246014>

    If you're going to clean up the StringBuilder usage here then I recommend eliminating all string concatentations (+) in these statements:
    ```java
    resultStr.append(CliStrings.REBALANCE__MSG__TOTALBUCKETCREATEBYTES).append(" = ").append(rstlist.get(0)).append(newLine);
    ```



geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java
Line 396 (original), 387 (patched)
<https://reviews.apache.org/r/58682/#comment246015>

    Another option for these conditionals is to use StringUtils.isBlank:
    ```java
    import org.apache.geode.internal.lang.StringUtils;
    
    if (StringUtils.isBlank(keyClass)) {
    ```



geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java
Line 520 (original), 539 (patched)
<https://reviews.apache.org/r/58682/#comment246016>

    I would go ahead and delete dead-code like this



geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java
Line 301 (original), 317 (patched)
<https://reviews.apache.org/r/58682/#comment246018>

    Should probably lower this below FATAL. WARN might be a good log level.
    
    I think we've generally said that FATAL means the cache server is DOA and cannot recover or may lose data.



geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java
Lines 54 (patched)
<https://reviews.apache.org/r/58682/#comment246019>

    General rule of thumb is to place these inner classes near the end of the outer class after any methods of the outer class.
    
    You can probably change everything that doesn't @Override to be package-protected (no qualifier).



geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java
Lines 154 (patched)
<https://reviews.apache.org/r/58682/#comment246020>

    Usually you can use this flavor:
    ```java
    assertThat(tableJson.getJSONArray(k)).hasSize(4);
    ```



geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java
Lines 169 (patched)
<https://reviews.apache.org/r/58682/#comment246021>

    Here's a gotcha with AssertJ. This line doesn't actually assert anything because you don't have "isTrue()" on the end.
    
    I'm not 100% sure what dataContent is without searching above, but you can probably use this flavor:
    ```java
    assertThat(dataContent).contains(constructCustomerFromJsonIndex(i, tableJson));
    ```
    The assertions for Collections are really rich in AssertJ.



geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java
Lines 184 (patched)
<https://reviews.apache.org/r/58682/#comment246022>

    This is another one that's not actually asserting anything.



geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java
Lines 200 (patched)
<https://reviews.apache.org/r/58682/#comment246023>

    This is another one that's not actually asserting anything.



geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java
Lines 228 (patched)
<https://reviews.apache.org/r/58682/#comment246024>

    I think this might be a case where it's cleaner to let this method "throws GfJsonException" and then any test that invokes this method can just use "throws Exception" and let that GfJsonException throw out to JUnit which reports the failure very well.


- Kirk Lund


On April 24, 2017, 8:06 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58682/
> -----------------------------------------------------------
> 
> (Updated April 24, 2017, 8:06 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Added unittests to capture failing behavior.
> 
> Corrected buildTable shifting columns when adding values with additional keys.
> 
> 
> Refactoring of DataCommandResult and DataCommandFunction
> 
> ------
> 
> The majority of changes to DataCommandFunction and DataCommandResult are refactoring.  Two ignored exceptions have been explicitly noted in the logger.
> 
> - In DataCommandFunction:423, there's an empty if block.  I imagine I should remove that, since empty code is a waste.  Looking for it, I couldn't find the comment-hinted separate method, though. Anyone familiar with this corner of the code know if that actuall happens?
> 
> - Qualitative changes are in DataCommandResult.buildTable.  Items in the table are scanned to build an agggregate key set, and those items missing any of these keys are padded with `MISSING_VALUE`.
> 
> - I moved some of the more cumbersome blocks of code to subfunctions, but may have done this more than necessary.  Opinions?
> 
> - Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive development / note the bug in GEODE-2662.  Are there additional tests that would fit naturally in this file?
> 
> 
> Diffs
> -----
> 
>   geode-core/build.gradle f07444a 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java 6324b5c 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java 423d781 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java bb77466 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java e72654e 
>   geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryDataInconsistencyDUnitTest.java 1af6261 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java 0e65354 
> 
> 
> Diff: https://reviews.apache.org/r/58682/diff/3/
> 
> 
> Testing
> -------
> 
> precheckin currently running.
> 
> DistributedTest still pending.  All others returned green.
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>

Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.

Posted by Jinmei Liao <ji...@pivotal.io>.

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




geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java
Line 50 (original), 53 (patched)
<https://reviews.apache.org/r/58682/#comment245952>

    A general comment about this class: all those if statement in this class makes me wonder if we can break this class down to parent/children classes. We can have GetCommandResult, PutCommandResult, DeleteCommandResult.. etc. Just a thought.



geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java
Line 117 (original), 117 (patched)
<https://reviews.apache.org/r/58682/#comment245938>

    looks like this method and the one below is not even used. delete them.



geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java
Lines 127 (patched)
<https://reviews.apache.org/r/58682/#comment245945>

    You probably are copying this from another test, but if you use ServerStarterRule, it would be easier to set the cache up:
    
      @ClassRule
      public static ServerStarterRule server = new ServerStarterRule();
      @BeforeClass
      public static void setUp() throws Exception {
        server.withRegion(RegionShortcut.PARTITION, PARTITIONED_REGION).startServer(true);
       region = server.getCache().getRegion(PARTITIONED_REGION);
       ...
       }
       
       
       then you don't need the teardown either


- Jinmei Liao


On April 24, 2017, 8:06 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58682/
> -----------------------------------------------------------
> 
> (Updated April 24, 2017, 8:06 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Added unittests to capture failing behavior.
> 
> Corrected buildTable shifting columns when adding values with additional keys.
> 
> 
> Refactoring of DataCommandResult and DataCommandFunction
> 
> ------
> 
> The majority of changes to DataCommandFunction and DataCommandResult are refactoring.  Two ignored exceptions have been explicitly noted in the logger.
> 
> - In DataCommandFunction:423, there's an empty if block.  I imagine I should remove that, since empty code is a waste.  Looking for it, I couldn't find the comment-hinted separate method, though. Anyone familiar with this corner of the code know if that actuall happens?
> 
> - Qualitative changes are in DataCommandResult.buildTable.  Items in the table are scanned to build an agggregate key set, and those items missing any of these keys are padded with `MISSING_VALUE`.
> 
> - I moved some of the more cumbersome blocks of code to subfunctions, but may have done this more than necessary.  Opinions?
> 
> - Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive development / note the bug in GEODE-2662.  Are there additional tests that would fit naturally in this file?
> 
> 
> Diffs
> -----
> 
>   geode-core/build.gradle f07444af7baa41fc0fcd0a088521bd96b317d15f 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java 423d781248458a450c2e98cd8a0e609c8656edb0 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java bb77466962a9a07a48bc09976b94d2f3822aed6d 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java e72654ed4aa11a5eb3f69a275d253cbedeb7df1e 
>   geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryDataInconsistencyDUnitTest.java 1af6261613f51a7cc3afe49bf8f3e2785ed2d60d 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/58682/diff/1/
> 
> 
> Testing
> -------
> 
> precheckin currently running.
> 
> DistributedTest still pending.  All others returned green.
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>

Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.

Posted by Patrick Rhomberg <pr...@pivotal.io>.

(Updated April 24, 2017, 8:06 p.m.)

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

Summary (updated)
-----------------

GEODE-2662: Missing keys cause columns to shift in gfsh table display.

Repository: geode

Description (updated)
-------

Added unittests to capture failing behavior.

Corrected buildTable shifting columns when adding values with additional keys.

Refactoring of DataCommandResult and DataCommandFunction

------

The majority of changes to DataCommandFunction and DataCommandResult are refactoring. Two ignored exceptions have been explicitly noted in the logger.

- Qualitative changes are in DataCommandResult.buildTable. Items in the table are scanned to build an agggregate key set, and those items missing any of these keys are padded with `MISSING_VALUE`.

- I moved some of the more cumbersome blocks of code to subfunctions, but may have done this more than necessary. Opinions?

- Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive development / note the bug in GEODE-2662. Are there additional tests that would fit naturally in this file?

Diffs
-----

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

Testing
-------

precheckin currently running.

Thanks,

Patrick Rhomberg