----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58682/ -----------------------------------------------------------
(Updated May 5, 2017, 9:22 p.m.) Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Swapnil Bawaskar. Changes ------- A couple critical "cleanups" went from (x != null ...) to (Util.isEmpty(x)). Whoops. 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 (updated) ----- geode-core/build.gradle f07444a geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java 29d68bd 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/commands/GemfireDataCommandsDUnitTest.java 0e2a41e 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 ead1047 Diff: https://reviews.apache.org/r/58682/diff/7/ Changes: https://reviews.apache.org/r/58682/diff/6-7/ Testing ------- Previous precheckin returned fully green. Running with new refactoring but expect no issues. (Common last words.) Thanks, Patrick Rhomberg
