You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Patrick Rhomberg <pr...@pivotal.io> on 2017/03/21 22:05:17 UTC
Review Request 57822: GEODE-1274: Migration from PulseLogWriter to
Log4j standard.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57822/
-----------------------------------------------------------
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
Re: Review Request 57822: GEODE-1274: Migration from PulseLogWriter
to Log4j standard.
Posted by Patrick Rhomberg <pr...@pivotal.io>.
> On March 23, 2017, 5:39 p.m., Jared Stewart wrote:
> > Is there a log4j configuration being applied somewhere? I'm wondering where I should expect the new Pulse logs to show up.
I'm not entirely clear on how log4j workst. That said...
All the pulse loggers are now being instantiated directly from log4j's LogManager. LogService (in geode-core) extends LogManager and adds the configurations. It appears that those configurations are then loaded into the LogManager class. So in theory, those should propogate to the pulse log4j things. I did some cursory testing on gfsh directly, and it appears that logs are showing up in the locator's logs. (Does `start pulse` target the locator? Are the locator and manager separate things? They're both being listed on the same member within pulse, so I don't know if the appenders just happen to be sharing that log file.) [update:] Looking at it again, I'm not 100% sure those aren't the REST API calls that were already showing up in the logs. But there is a marked difference in logging present between the current `develop` branch and this patch, so I was led to believe that we are in fact capturing the pulse log calls.
Could this be a potential problem in a standalone deployment of pulse?
======
After some testing, it looks like, when run embedded, that the LogService's managing of Log4J settings percolates through. Formatting and location should be updated by the log4j configurations.
- Patrick
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57822/#review169899
-----------------------------------------------------------
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
>
>
Re: Review Request 57822: GEODE-1274: Migration from PulseLogWriter
to Log4j standard.
Posted by Jared Stewart <js...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57822/#review169899
-----------------------------------------------------------
Is there a log4j configuration being applied somewhere? I'm wondering where I should expect the new Pulse logs to show up.
- Jared Stewart
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
>
>
Re: Review Request 57822: GEODE-1274: Migration from PulseLogWriter
to Log4j standard.
Posted by Patrick Rhomberg <pr...@pivotal.io>.
> On March 24, 2017, 4:10 p.m., Jinmei Liao wrote:
> > We will need to verify that the logs went to the same place as before, or at least went to a reasonable place.
[The below should be struck-through but isn't rendering that way on my end...]
~~It appears that, at least in my testing, the Pulse logs are sent to the home directory `~/PULSELOG.log`. This appears to be the case whether Pulse is in the stand-alone deployment or embedded.~~
~~Do we want to try to merge this log in with the Geode log in the event that Pulse is running embedded?~~
[The above should be struck-through but isn't rendering that way on my end...]
It looks like the OLD behavior was that the PulseLogWriter would write to the home directory `~/PULSELOG_#.log`. Now it is correctly controlled by the Log4J's wrapper and should appear with the rest of the Geode logs when run embedded. When run standalone, it appears in the server's logs as expected (in my case, `/usr/local/Cellar/tomcat/8.5.12/libexec/logs/localhost.[date].log`. This should still be influenced when a `pulse.properties` is somewhere in the webserver's path for Pulse to find.
- Patrick
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57822/#review170022
-----------------------------------------------------------
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
>
>
Re: Review Request 57822: GEODE-1274: Migration from PulseLogWriter
to Log4j standard.
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 57822: GEODE-1274: Migration from PulseLogWriter
to Log4j standard.
Posted by Patrick Rhomberg <pr...@pivotal.io>.
-----------------------------------------------------------
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 (updated)
-------
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/57822/diff/1-2/
Testing (updated)
-------
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
Re: Review Request 57822: GEODE-1274: Migration from PulseLogWriter
to Log4j standard.
Posted by Jinmei Liao <ji...@pivotal.io>.
> On March 21, 2017, 11:36 p.m., Jinmei Liao wrote:
> > geode-pulse/build.gradle
> > Lines 31 (patched)
> > <https://reviews.apache.org/r/57822/diff/1/?file=1671156#file1671156line31>
> >
> > I just noticed this line. Pulse should NOT require geode-core and geode-common because it is a pulse requirement that the pulse.war can be deployed onto a standalone web server, any communication between pulse and geode needs to be done via http or jmx.
> >
> > I think we will just need to use log4j api to do the logging instead of using LogService.getLogger api if we want to get rid of PulseLogWriter
>
> Kirk Lund wrote:
> This change only includes geode-core and geode-common classes in the classpath for Pulse code. Imagine using org.apache.geode.internal.util.IOUtils in Pulse code. Just because these are on the classpath, doesn't mean Pulse has any communication with Geode Server or Locator other than via HTTP or JMX. I think it's ok for Pulse to use classes from other Geode modules.
>
> Kirk Lund wrote:
> Another reason to add geode-core to Pulse is so we can delete all the fake Geode mbeans and classes. Example: geode-pulse/src/test/java/org/apache/geode/tools/pulse/tests/MemberMBean.java and geode-pulse/src/test/java/org/apache/geode/tools/pulse/tests/Region.java
These are test codes, it's ok for us to bring in geode-core for tests, but not ok to use that in the pulse product.
- Jinmei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57822/#review169644
-----------------------------------------------------------
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
>
>
Re: Review Request 57822: GEODE-1274: Migration from PulseLogWriter
to Log4j standard.
Posted by Kirk Lund <ki...@gmail.com>.
> On March 21, 2017, 11:36 p.m., Jinmei Liao wrote:
> > geode-pulse/build.gradle
> > Lines 31 (patched)
> > <https://reviews.apache.org/r/57822/diff/1/?file=1671156#file1671156line31>
> >
> > I just noticed this line. Pulse should NOT require geode-core and geode-common because it is a pulse requirement that the pulse.war can be deployed onto a standalone web server, any communication between pulse and geode needs to be done via http or jmx.
> >
> > I think we will just need to use log4j api to do the logging instead of using LogService.getLogger api if we want to get rid of PulseLogWriter
This change only includes geode-core and geode-common classes in the classpath for Pulse code. Imagine using org.apache.geode.internal.util.IOUtils in Pulse code. Just because these are on the classpath, doesn't mean Pulse has any communication with Geode Server or Locator other than via HTTP or JMX. I think it's ok for Pulse to use classes from other Geode modules.
- Kirk
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57822/#review169644
-----------------------------------------------------------
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
>
>
Re: Review Request 57822: GEODE-1274: Migration from PulseLogWriter
to Log4j standard.
Posted by Kirk Lund <ki...@gmail.com>.
> On March 21, 2017, 11:36 p.m., Jinmei Liao wrote:
> > geode-pulse/build.gradle
> > Lines 31 (patched)
> > <https://reviews.apache.org/r/57822/diff/1/?file=1671156#file1671156line31>
> >
> > I just noticed this line. Pulse should NOT require geode-core and geode-common because it is a pulse requirement that the pulse.war can be deployed onto a standalone web server, any communication between pulse and geode needs to be done via http or jmx.
> >
> > I think we will just need to use log4j api to do the logging instead of using LogService.getLogger api if we want to get rid of PulseLogWriter
>
> Kirk Lund wrote:
> This change only includes geode-core and geode-common classes in the classpath for Pulse code. Imagine using org.apache.geode.internal.util.IOUtils in Pulse code. Just because these are on the classpath, doesn't mean Pulse has any communication with Geode Server or Locator other than via HTTP or JMX. I think it's ok for Pulse to use classes from other Geode modules.
Another reason to add geode-core to Pulse is so we can delete all the fake Geode mbeans and classes. Example: geode-pulse/src/test/java/org/apache/geode/tools/pulse/tests/MemberMBean.java and geode-pulse/src/test/java/org/apache/geode/tools/pulse/tests/Region.java
- Kirk
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57822/#review169644
-----------------------------------------------------------
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
>
>
Re: Review Request 57822: GEODE-1274: Migration from PulseLogWriter
to Log4j standard.
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57822/#review169644
-----------------------------------------------------------
geode-pulse/build.gradle
Lines 31 (patched)
<https://reviews.apache.org/r/57822/#comment242156>
I just noticed this line. Pulse should NOT require geode-core and geode-common because it is a pulse requirement that the pulse.war can be deployed onto a standalone web server, any communication between pulse and geode needs to be done via http or jmx.
I think we will just need to use log4j api to do the logging instead of using LogService.getLogger api if we want to get rid of PulseLogWriter
- Jinmei Liao
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
>
>
Re: Review Request 57822: GEODE-1274: Migration from PulseLogWriter
to Log4j standard.
Posted by Kevin Duling <kd...@pivotal.io>.
> On March 21, 2017, 3:16 p.m., Jinmei Liao wrote:
> > geode-pulse/build.gradle
> > Line 70 (original)
> > <https://reviews.apache.org/r/57822/diff/1/?file=1671156#file1671156line73>
> >
> > you only updated one test class which doesn't need logging at all. Let's not introduce this test compile dependency.
>
> Kevin Duling wrote:
> I am also introducing this in GEODE-2671 in order to get ahold of the `jmx-manager-port` constant.
Wait...that should be compile, not testCompile.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57822/#review169629
-----------------------------------------------------------
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
>
>
Re: Review Request 57822: GEODE-1274: Migration from PulseLogWriter
to Log4j standard.
Posted by Kevin Duling <kd...@pivotal.io>.
> On March 21, 2017, 3:16 p.m., Jinmei Liao wrote:
> > geode-pulse/build.gradle
> > Line 70 (original)
> > <https://reviews.apache.org/r/57822/diff/1/?file=1671156#file1671156line73>
> >
> > you only updated one test class which doesn't need logging at all. Let's not introduce this test compile dependency.
I am also introducing this in GEODE-2671 in order to get ahold of the `jmx-manager-port` constant.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57822/#review169629
-----------------------------------------------------------
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
>
>
Re: Review Request 57822: GEODE-1274: Migration from PulseLogWriter
to Log4j standard.
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57822/#review169629
-----------------------------------------------------------
geode-pulse/build.gradle
Line 70 (original)
<https://reviews.apache.org/r/57822/#comment242099>
you only updated one test class which doesn't need logging at all. Let's not introduce this test compile dependency.
geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/PulseAppListener.java
Line 46 (original), 48 (patched)
<https://reviews.apache.org/r/57822/#comment242095>
delete this line
geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/Cluster.java
Line 3712 (original), 3713 (patched)
<https://reviews.apache.org/r/57822/#comment242093>
can we just catc Exception here?
geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/PulseConstants.java
Line 20 (original), 20 (patched)
<https://reviews.apache.org/r/57822/#comment242092>
delete this line
geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/log/LogWriter.java
Lines 28 (patched)
<https://reviews.apache.org/r/57822/#comment242091>
Can we delete it?
geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/log/MessageFormatter.java
Lines 40 (patched)
<https://reviews.apache.org/r/57822/#comment242090>
Can we delete it?
geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/log/PulseLogWriter.java
Lines 38 (patched)
<https://reviews.apache.org/r/57822/#comment242089>
Since it's internal package, can we delete it?
- Jinmei Liao
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
>
>
Re: Review Request 57822: GEODE-1274: Migration from PulseLogWriter
to Log4j standard.
Posted by Kirk Lund <ki...@gmail.com>.
> On March 21, 2017, 10: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.
I'm leaning towards just committing these changes as they are and then worry about removing the i18n localization in a future change.
- Kirk
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57822/#review169628
-----------------------------------------------------------
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
>
>
Re: Review Request 57822: GEODE-1274: Migration from PulseLogWriter
to Log4j standard.
Posted by Kevin Duling <kd...@pivotal.io>.
> 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
>
>
Re: Review Request 57822: GEODE-1274: Migration from PulseLogWriter
to Log4j standard.
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
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
>
>