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