geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Patrick Rhomberg <prhomb...@pivotal.io>
Subject Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.
Date Wed, 26 Apr 2017 18:22:29 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message