----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58682/#review172874 -----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java Lines 269 (patched) <https://reviews.apache.org/r/58682/#comment245886> You should delete the printStackTrace. You might consider logging this first exception. In general, it's best to avoid nested try-catch blocks as well. Looks like it's maybe required in this case though? I suppose you could set a flag and then do create the new GfJsonObject after the catch-block if the flag is true. I don't feel strongly either way but it's not typical to it this way. geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java Line 263 (original) <https://reviews.apache.org/r/58682/#comment245888> Log4J 2 unfortunately implemented things in such a way that log statements below the configured log level results in significant impact to performance. In the Geode code base we wrap all Loggers inside an instance of FastLogger. The FastLogger uses a single volatile to disable lower log levels when log level is below info (and no filters are configured) but it only works if you keep the isDebugEnabled() checks in place. If log level is INFO or above then FastLogger help minimize any performance impact. Without it, Geode throughput is decreased substantially by Log4J 2 even at log levels above DEBUG simply by hitting logger.debug statements that are not enabled. - 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 f07444af7baa41fc0fcd0a088521bd96b317d15f > > geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java > 423d781248458a450c2e98cd8a0e609c8656edb0 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java > bb77466962a9a07a48bc09976b94d2f3822aed6d > > geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java > e72654ed4aa11a5eb3f69a275d253cbedeb7df1e > > geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryDataInconsistencyDUnitTest.java > 1af6261613f51a7cc3afe49bf8f3e2785ed2d60d > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/58682/diff/1/ > > > Testing > ------- > > precheckin currently running. > > DistributedTest still pending. All others returned green. > > > Thanks, > > Patrick Rhomberg > >
