----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57822/#review169628 -----------------------------------------------------------
The localization resource bundles in Pulse require some thought (do we delete them? do we use them with log4j2 in the standard way that log4j2 would normally use them?). geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/PulseAppListener.java Line 46 (original), 48 (patched) <https://reviews.apache.org/r/57822/#comment242088> delete any dead code geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/PulseAppListener.java Line 87 (original), 89 (patched) <https://reviews.apache.org/r/57822/#comment242105> Pulse has localization resource bundles: geode-pulse/src/main/resources/Resource Bundle 'LogMessages'/LogMessages_en_US.properties geode-pulse/src/main/resources/Resource Bundle 'LogMessages'/LogMessages_fr_FR.properties I'm a little worried about this use of ResourceBundle. I suspect Log4j2 has a specific way of dealing with resource bundles and I recommend researching that before committing this: http://logging.apache.org/log4j/2.x/manual/configuration.html#PropertySubstitution https://logging.apache.org/log4j/2.0/log4j-api/apidocs/org/apache/logging/log4j/message/LocalizedMessage.html https://dzone.com/articles/log4j-2-configuration-using-properties-file Another thought: GEODE-536 says to remove our hacky/broken i18n from Geode and then we can start over from scratch if we decide to add proper localization support. Perhaps we should delete the Pulse resource bundles and use the English values as the log statements. geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/PulseAppListener.java Line 210 (original), 206 (patched) <https://reviews.apache.org/r/57822/#comment242096> Change any log statements that don't use ResourceBundle to use full log4j2 syntax (we'll revisit the ResourceBundle in the future): ```java logger.info("#SpringProfilesConfigured : {}", sb); ``` No need to invoke toString() because log4j2 will do that when it replaces "{}" Multiple arguments simply have multiple "{}" in the log message. geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/PulseAppListener.java Line 212 (original), 208 (patched) <https://reviews.apache.org/r/57822/#comment242097> Use {} geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/controllers/PulseController.java Line 114 (original), 116 (patched) <https://reviews.apache.org/r/57822/#comment242100> Any place where you're logging exception, don't log getMessage, just give Logger the exception itself. This will allow log4j2 to log the stack trace as well. logger.debug("Exception Occurred : {}", e); - Kirk Lund On March 21, 2017, 10:05 p.m., Patrick Rhomberg wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57822/ > ----------------------------------------------------------- > > (Updated March 21, 2017, 10:05 p.m.) > > > Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, Ken Howe, > Kirk Lund, and Swapnil Bawaskar. > > > Repository: geode > > > Description > ------- > > GEODE-1274: Migration from PulseLogWriter to Log4j standard. > > > Diffs > ----- > > geode-pulse/build.gradle 298ae5a8a32d621defd3336c21614c588b2ac7dc > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/PulseAppListener.java > 5408a5651774a63c16f27722c6ff7bda25cbaaa8 > > 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 > 9b24393792cc52773089e08db6f1739c0d2c553f > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/Cluster.java > 083731ba9e26e711b72f8bf0bdf470d9852aa663 > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/DataBrowser.java > d20be590d12faf53f91a64ad0d96646b92dd118e > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/JMXDataUpdater.java > 758ad4be1f41946b98283c45ac27a022c75a9f14 > > 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 > e94ef631724b4a62d5a2486674fc7a2e5f746788 > > 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/1/ > > > Testing > ------- > > Precheckin up and running. Includes general PulseLogWriter conversion to > Log4j, with the hope that the missing pulse logging will now be gathered with > other artifacts. > > Also included in this patch are a number of minor readability improvements > regarding repeated error blocks and logging text typo corrections. > > > Thanks, > > Patrick Rhomberg > >
