geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kirk Lund <kirk.l...@gmail.com>
Subject Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.
Date Tue, 25 Apr 2017 21:10:53 GMT

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


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