----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53108/#review154312 -----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/io/RollingFileHandler.java (line 27) <https://reviews.apache.org/r/53108/#comment223798> I think all three of these patterns can be moved to MainWithChildrenRollingFileHandler since it is the only one using them. So they can be static private finals on that class. geode-core/src/main/java/org/apache/geode/internal/logging/ManagerLogWriter.java (line 42) <https://reviews.apache.org/r/53108/#comment223799> I don't see a reason for this being "static". The class being created has no state so the only reason I can think of is that the handler class might be expensive to create. I think many of the old methods that you refactored into the handler class were static just to show that they did not depend on the instance vars in ManagerLogWriter. Your refactoring makes that clear since they are in a seperate class now. geode-core/src/main/java/org/apache/geode/internal/statistics/StatArchiveHandler.java (line 659) <https://reviews.apache.org/r/53108/#comment223800> consider refactoring getArchivePattern and getLogPattern together and moving them to your new handler class geode-core/src/main/java/org/apache/geode/internal/statistics/StatMonitorHandler.java (line 79) <https://reviews.apache.org/r/53108/#comment223801> For this code I think this condition is all that is needed: "if (this.notifier !=null)" geode-core/src/main/java/org/apache/geode/internal/statistics/StatMonitorHandler.java (line 92) <https://reviews.apache.org/r/53108/#comment223803> only the null check is needed here. Shouldn't this block also null out "notifier" if it stops it? Pull the code in lines 79-82 into a stopNotifier method and then call that method from here and there - Darrel Schneider On Oct. 31, 2016, 12:56 p.m., Kirk Lund wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53108/ > ----------------------------------------------------------- > > (Updated Oct. 31, 2016, 12:56 p.m.) > > > Review request for geode, Anthony Baker, Darrel Schneider, Jinmei Liao, Kevin > Duling, Swapnil Bawaskar, and Dan Smith. > > > Bugs: GEODE-2012 > https://issues.apache.org/jira/browse/GEODE-2012 > > > Repository: geode > > > Description > ------- > > GEODE-2012: always write stat types to archive > > * alter NanoTimer and its use to make testing easier > * remove extra spaces in localized string > * import SuppressWarnings to make use of annotation cleaner > * new test for rolling stat files: FileSizeLimitIntegrationTest > * new test for rolling and deleting old stat files: > DiskSpaceLimitIntegrationTest > * new test to expose GEODE-2012 and confirm its fix: > StatTypesAreRolledOverRegressionTest > > > Diffs > ----- > > geode-core/src/main/java/org/apache/geode/internal/NanoTimer.java > 12e91f8e05ee93370f56d0af171558e7741eb0f2 > > geode-core/src/main/java/org/apache/geode/internal/i18n/LocalizedStrings.java > 210539bf33896259d56a41d05546101c996ac553 > > geode-core/src/main/java/org/apache/geode/internal/io/MainWithChildrenRollingFileHandler.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/internal/io/RollingFileHandler.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/internal/logging/ManagerLogWriter.java > a000d2c01008730dcf2a0faab0ae7f5ade5fcae1 > > geode-core/src/main/java/org/apache/geode/internal/statistics/HostStatSampler.java > 6d7b9670385babdbe0f829e70b960dd379259d62 > > geode-core/src/main/java/org/apache/geode/internal/statistics/SampleCollector.java > 610e848b3185baca0852efef085502e6d33ca2aa > > geode-core/src/main/java/org/apache/geode/internal/statistics/SimpleStatSampler.java > 7eaa1e0a914fd9ef01d1e7cfbd110329c43de493 > > geode-core/src/main/java/org/apache/geode/internal/statistics/StatArchiveHandler.java > 5520936ba9835354a4225ff52f3bdcaf0ccb616a > > geode-core/src/main/java/org/apache/geode/internal/statistics/StatArchiveWriter.java > a4c15d13401f20ca6717fdddff040cf29bc26bf5 > > geode-core/src/main/java/org/apache/geode/internal/statistics/StatMonitorHandler.java > a2b0ae4d5cf5c40bed392292180a8987952e80a9 > > geode-core/src/main/java/org/apache/geode/internal/util/concurrent/StoppableCountDownLatch.java > 740fd7fdc98928de0054ca83d11716033225e373 > > geode-core/src/test/java/org/apache/geode/internal/statistics/DiskSpaceLimitIntegrationTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/internal/statistics/FileSizeLimitIntegrationTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/internal/statistics/SampleCollectorTest.java > ab84ea4355df17b93c75487fb01fbb59f943bafc > > geode-core/src/test/java/org/apache/geode/internal/statistics/StatMonitorHandlerTest.java > 85e6f6d4a943989a129d7a752fc70a5330a3428f > > geode-core/src/test/java/org/apache/geode/internal/statistics/StatMonitorHandlerWIthEnabledMonitorThreadTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/internal/statistics/StatSamplerUtils.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/internal/statistics/StatTypesAreRolledOverRegressionTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/internal/statistics/StatisticsMonitorTest.java > b570db2e0094326f30dd47935a1558d9d1b01d48 > > geode-core/src/test/java/org/apache/geode/internal/statistics/ValueMonitorIntegrationTest.java > cef5977bfbc86e956e79e9d50b1a9b99fdab55e6 > > Diff: https://reviews.apache.org/r/53108/diff/ > > > Testing > ------- > > precheckin passed everything! > > > Thanks, > > Kirk Lund > >
