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