geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jared Stewart <jstew...@pivotal.io>
Subject Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.
Date Tue, 25 Apr 2017 22:51:24 GMT

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


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