----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57822/#review170022 -----------------------------------------------------------
We will need to verify that the logs went to the same place as before, or at least went to a reasonable place. - Jinmei Liao On March 22, 2017, 10:33 p.m., Patrick Rhomberg wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57822/ > ----------------------------------------------------------- > > (Updated March 22, 2017, 10:33 p.m.) > > > Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, Ken Howe, > Kirk Lund, and Swapnil Bawaskar. > > > Repository: geode > > > Description > ------- > > To avoid dependency on geode-core, the pulse loggers are instantiated > directly from LogManager, rather than canonical LogService (which itself > extends LogManager). > > Significant reduction of logging level state checks, relying on Log4j > handling. > Significant reduction of string concatenation, relying on Log4j2 string > substitutions. > Reduction of logging using an exception e.getMessage, favoring instead > passing the exception itself for the stacktrace. > Multiple identical exception blocks collapsed. > > ================ > > Explicitly not addressed in this ticket: > - A great deal of logging could do with localization. > - A number of exception blocks catch one or more types of exception and only > log the exception before moving. These are specific exceptions and not the > general `Exception` class. The catches could be simplified, but could that > lead to a catch an error that would otherwise be raised? > > The original ticket was not about updating from a depreciated class, but > about how logs were not being collected. The answer to this was that Log4j > wasn't tracking them, so they weren't being gathered; extending Log4j to do > Pulse tracking should resolve the issue, but is it okay that this is only > tangentally answering the problem? > > > Diffs > ----- > > geode-pulse/build.gradle a038f734266939f3c12b713ce0745c013b4adb91 > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/PulseAppListener.java > e1666ecf38331983d8c47c4b46b3b7e87faaf854 > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/controllers/ExceptionHandlingAdvice.java > 80a3c63b5d9170cf9933c43edf56da78dc62bf46 > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/controllers/PulseController.java > 12b6172cbbc79651e972179532f2b79623a1992e > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/Cluster.java > e71388d134c96549ee9995c4c874615ee66fe7c1 > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/DataBrowser.java > 03077c27003811718e3974575a15b6892f6d8e2f > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/JMXDataUpdater.java > 9633b9a1f50df051b8fb9b4f4787a1d25a0ab019 > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/JmxManagerFinder.java > fa2b5b79f40a95b0a20be38c20e61d9e94a39688 > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/PulseConfig.java > dc643b49462af9365859d899f2c798f0f2448b72 > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/PulseConstants.java > b36c283630fcd3d143c1398ac0fecf45eec0eb54 > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/Repository.java > b228e4a754100fe07c9dbec232d5e88809aefeef > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/log/LogWriter.java > 6f90e7a48a10ab2778ee00cf3d707a476ebe91eb > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/log/MessageFormatter.java > 924520d3ba70a48379bd6ff9a6ce4d6e0d4ed804 > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/log/PulseLogWriter.java > 241b34bfe5bc56e062c43944ec7aaf1cfaa657c7 > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/log/PulseLogger.java > d22f188248d2d8fb685074523e22eac7c26f5e20 > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/security/GemFireAuthentication.java > 884e51fa71b79de9e5ab9e7f0f933e37b6031438 > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/security/GemFireAuthenticationProvider.java > 4d300f04ff82f701509d44b83dd46698dbc6035e > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/security/LogoutHandler.java > e18e35d596552a40715ee772e559ffc2d077af5e > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/service/ClusterSelectedRegionService.java > a088ccb4176db19f9ec0ec3570ad23d286609f7b > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/service/ClusterSelectedRegionsMemberService.java > d5a913793cd22c408398f3e76767ea7379c14042 > > geode-pulse/src/test/java/org/apache/geode/tools/pulse/testbed/PropMockDataUpdater.java > df315728dbcc07444249f68829b9bed349736ac9 > > > Diff: https://reviews.apache.org/r/57822/diff/2/ > > > Testing > ------- > > precheckin running. Test/LegacyDUnitTest have already come back green. > > Testing not done: actually verifying anything involving Pulse logging. I > still need to learn how pulse is used. > > > Thanks, > > Patrick Rhomberg > >
