> On March 21, 2017, 3:28 p.m., Kirk Lund wrote: > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/PulseAppListener.java > > Line 87 (original), 89 (patched) > > <https://reviews.apache.org/r/57822/diff/1/?file=1671157#file1671157line90> > > > > 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. > > Kirk Lund wrote: > I'm leaning towards just committing these changes as they are and then > worry about removing the i18n localization in a future change.
I agree. Removing/revamping i18n should be a separate effort, especially since there's a ticket for it. - Kevin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57822/#review169628 ----------------------------------------------------------- On March 21, 2017, 3: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, 3: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 > >
