You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Jinmei Liao <ji...@pivotal.io> on 2017/05/16 17:31:24 UTC
Review Request 59316: GEODE-2927: fix pulse logging and useLocator,
SSL flags
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59316/
-----------------------------------------------------------
Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
Repository: geode
Description
-------
* using local mbs server connection will bypass all the mbean security checks
* do not update the mbean attribute since pulse user has no cluster:write privilege at all
* Created EmbeddedPulseRule for tests
* simplify PulseAppListener
* use log4j2 logging configurations
Diffs
-----
geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/EmbeddedPulseRule.java 0ed5403d662eda1de4009d445a072ca50e4e0836
geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseSecurityTest.java 7278c843afda5ddb3b33a700c2aac56473701330
geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseSecurityWithSSLTest.java 3b9cd72d197e8a88999b83475ef14fc56f235feb
geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseVerificationTest.java PRE-CREATION
geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java 9394efb200a65b96244ef19d3ce8c6329c80652f
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java d61e72d9aa37a8631cfe5e5de7c593724e5fd5a0
geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/PulseAppListener.java 35f494bd11f01662b5efd01d45ff284e63d283c2
geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/controllers/PulseController.java c8788791365ac7ba5ac43228c553b1a3a48d3663
geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/Cluster.java 78e92d429e8f2f6c2ff370d88442cbbc1f4761a0
geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/IClusterUpdater.java 3ec820769a897bfb4f10a76a86dd2ce54fba3b70
geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/JMXDataUpdater.java c74e3cd43bc4e9d3c39c390134821472d8ace9bd
geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/Repository.java 52e13a70f1aaa6f233b3804399b90efc3a10328b
geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/security/GemFireAuthenticationProvider.java 096bf6fa39035c93b952c367c965d586df61f81e
geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/security/LogoutHandler.java b169d4f6ac1c5077a48b572f0acefdd8a940309a
geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/service/ClusterMemberService.java b6a129d05126c66e773ca517bc7247166f56bf05
geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/service/ClusterMembersRGraphService.java 90d4ecdb5185dc8a8e8cae5dde3b15bbc914e267
geode-pulse/src/main/resources/log4j2.properties PRE-CREATION
geode-pulse/src/main/resources/pulse.properties d47d46910e46cc7a1178c835c448a0e7745b0032
geode-pulse/src/test/java/org/apache/geode/tools/pulse/controllers/PulseControllerJUnitTest.java ddd799f5158e1f4213b0b6b2de8e1853ba65ab74
Diff: https://reviews.apache.org/r/59316/diff/1/
Testing
-------
Thanks,
Jinmei Liao
Re: Review Request 59316: GEODE-2927: fix pulse logging and
useLocator, SSL flags
Posted by Jinmei Liao <ji...@pivotal.io>.
> On May 16, 2017, 6:59 p.m., Kirk Lund wrote:
> > geode-pulse/src/main/resources/log4j2.properties
> > Lines 17 (patched)
> > <https://reviews.apache.org/r/59316/diff/1/?file=1721724#file1721724line17>
> >
> > Everything in this file is commented out?
> >
> > Also, all of the other config files for log4j2 in geode are xml instead of properties. It's probably best to keep them all the same format.
oh, probably I would provide log4j2.xml file instead of a properties file. This is meant as an example.
- Jinmei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59316/#review175139
-----------------------------------------------------------
On May 16, 2017, 5:31 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59316/
> -----------------------------------------------------------
>
> (Updated May 16, 2017, 5:31 p.m.)
>
>
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
>
>
> Repository: geode
>
>
> Description
> -------
>
> * using local mbs server connection will bypass all the mbean security checks
> * do not update the mbean attribute since pulse user has no cluster:write privilege at all
> * Created EmbeddedPulseRule for tests
> * simplify PulseAppListener
> * use log4j2 logging configurations
>
>
> Diffs
> -----
>
> geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/EmbeddedPulseRule.java 0ed5403d662eda1de4009d445a072ca50e4e0836
> geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseSecurityTest.java 7278c843afda5ddb3b33a700c2aac56473701330
> geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseSecurityWithSSLTest.java 3b9cd72d197e8a88999b83475ef14fc56f235feb
> geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseVerificationTest.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java 9394efb200a65b96244ef19d3ce8c6329c80652f
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java d61e72d9aa37a8631cfe5e5de7c593724e5fd5a0
> geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/PulseAppListener.java 35f494bd11f01662b5efd01d45ff284e63d283c2
> geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/controllers/PulseController.java c8788791365ac7ba5ac43228c553b1a3a48d3663
> geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/Cluster.java 78e92d429e8f2f6c2ff370d88442cbbc1f4761a0
> geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/IClusterUpdater.java 3ec820769a897bfb4f10a76a86dd2ce54fba3b70
> geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/JMXDataUpdater.java c74e3cd43bc4e9d3c39c390134821472d8ace9bd
> geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/Repository.java 52e13a70f1aaa6f233b3804399b90efc3a10328b
> geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/security/GemFireAuthenticationProvider.java 096bf6fa39035c93b952c367c965d586df61f81e
> geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/security/LogoutHandler.java b169d4f6ac1c5077a48b572f0acefdd8a940309a
> geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/service/ClusterMemberService.java b6a129d05126c66e773ca517bc7247166f56bf05
> geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/service/ClusterMembersRGraphService.java 90d4ecdb5185dc8a8e8cae5dde3b15bbc914e267
> geode-pulse/src/main/resources/log4j2.properties PRE-CREATION
> geode-pulse/src/main/resources/pulse.properties d47d46910e46cc7a1178c835c448a0e7745b0032
> geode-pulse/src/test/java/org/apache/geode/tools/pulse/controllers/PulseControllerJUnitTest.java ddd799f5158e1f4213b0b6b2de8e1853ba65ab74
>
>
> Diff: https://reviews.apache.org/r/59316/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 59316: GEODE-2927: fix pulse logging and
useLocator, SSL flags
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59316/#review175139
-----------------------------------------------------------
geode-pulse/src/main/resources/log4j2.properties
Lines 17 (patched)
<https://reviews.apache.org/r/59316/#comment248517>
Everything in this file is commented out?
Also, all of the other config files for log4j2 in geode are xml instead of properties. It's probably best to keep them all the same format.
- Kirk Lund
On May 16, 2017, 5:31 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59316/
> -----------------------------------------------------------
>
> (Updated May 16, 2017, 5:31 p.m.)
>
>
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
>
>
> Repository: geode
>
>
> Description
> -------
>
> * using local mbs server connection will bypass all the mbean security checks
> * do not update the mbean attribute since pulse user has no cluster:write privilege at all
> * Created EmbeddedPulseRule for tests
> * simplify PulseAppListener
> * use log4j2 logging configurations
>
>
> Diffs
> -----
>
> geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/EmbeddedPulseRule.java 0ed5403d662eda1de4009d445a072ca50e4e0836
> geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseSecurityTest.java 7278c843afda5ddb3b33a700c2aac56473701330
> geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseSecurityWithSSLTest.java 3b9cd72d197e8a88999b83475ef14fc56f235feb
> geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseVerificationTest.java PRE-CREATION
> geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java 9394efb200a65b96244ef19d3ce8c6329c80652f
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java d61e72d9aa37a8631cfe5e5de7c593724e5fd5a0
> geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/PulseAppListener.java 35f494bd11f01662b5efd01d45ff284e63d283c2
> geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/controllers/PulseController.java c8788791365ac7ba5ac43228c553b1a3a48d3663
> geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/Cluster.java 78e92d429e8f2f6c2ff370d88442cbbc1f4761a0
> geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/IClusterUpdater.java 3ec820769a897bfb4f10a76a86dd2ce54fba3b70
> geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/JMXDataUpdater.java c74e3cd43bc4e9d3c39c390134821472d8ace9bd
> geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/Repository.java 52e13a70f1aaa6f233b3804399b90efc3a10328b
> geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/security/GemFireAuthenticationProvider.java 096bf6fa39035c93b952c367c965d586df61f81e
> geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/security/LogoutHandler.java b169d4f6ac1c5077a48b572f0acefdd8a940309a
> geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/service/ClusterMemberService.java b6a129d05126c66e773ca517bc7247166f56bf05
> geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/service/ClusterMembersRGraphService.java 90d4ecdb5185dc8a8e8cae5dde3b15bbc914e267
> geode-pulse/src/main/resources/log4j2.properties PRE-CREATION
> geode-pulse/src/main/resources/pulse.properties d47d46910e46cc7a1178c835c448a0e7745b0032
> geode-pulse/src/test/java/org/apache/geode/tools/pulse/controllers/PulseControllerJUnitTest.java ddd799f5158e1f4213b0b6b2de8e1853ba65ab74
>
>
> Diff: https://reviews.apache.org/r/59316/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 59316: GEODE-2927: fix pulse logging and
useLocator, SSL flags
Posted by Jared Stewart <js...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59316/#review175271
-----------------------------------------------------------
Ship it!
Ship It!
- Jared Stewart
On May 17, 2017, 4:39 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59316/
> -----------------------------------------------------------
>
> (Updated May 17, 2017, 4:39 p.m.)
>
>
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
>
>
> Repository: geode
>
>
> Description
> -------
>
> * using local mbs server connection will bypass all the mbean security checks
> * do not update the mbean attribute since pulse user has no cluster:write privilege at all
> * Created EmbeddedPulseRule for tests
> * simplify PulseAppListener
> * use log4j2 logging configurations
>
>
> Diffs
> -----
>
> geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/EmbeddedPulseRule.java 0ed5403d662eda1de4009d445a072ca50e4e0836
>
>
> Diff: https://reviews.apache.org/r/59316/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 59316: GEODE-2927: fix pulse logging and
useLocator, SSL flags
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59316/
-----------------------------------------------------------
(Updated May 17, 2017, 4:39 p.m.)
Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
Repository: geode
Description
-------
* using local mbs server connection will bypass all the mbean security checks
* do not update the mbean attribute since pulse user has no cluster:write privilege at all
* Created EmbeddedPulseRule for tests
* simplify PulseAppListener
* use log4j2 logging configurations
Diffs (updated)
-----
geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/EmbeddedPulseRule.java 0ed5403d662eda1de4009d445a072ca50e4e0836
Diff: https://reviews.apache.org/r/59316/diff/3/
Changes: https://reviews.apache.org/r/59316/diff/2-3/
Testing
-------
Thanks,
Jinmei Liao
Re: Review Request 59316: GEODE-2927: fix pulse logging and
useLocator, SSL flags
Posted by Jinmei Liao <ji...@pivotal.io>.
> On May 16, 2017, 11:07 p.m., Jared Stewart wrote:
> > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/EmbeddedPulseRule.java
> > Lines 38 (patched)
> > <https://reviews.apache.org/r/59316/diff/1/?file=1721708#file1721708line45>
> >
> > I was a bit surprised that these `set` methods need to be called after `before`. Perhaps this would be more clear if `getRepository()` was replaced with a method like
> > ```
> > public Cluster connect() {
> > return repository.getCluster();
> > }
> > ```
> > that indicates the point where the `set` methods must be called before.
this is all due to the evil of Repository.get() will return a singleton. PulseVerificationTest needs to test using either jmx port or locator port, hence the need for these methods. I reworked the rule a bit to cleanup the state.
> On May 16, 2017, 11:07 p.m., Jared Stewart wrote:
> > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/IClusterUpdater.java
> > Lines 36 (patched)
> > <https://reviews.apache.org/r/59316/diff/1/?file=1721717#file1721717line36>
> >
> > What is the benefit of a default implementation that returns null?
there are bunch of MockClusterUpdator used in the old tests. I added this interface and provide a null implementation to avoid updating those mocks.
- Jinmei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59316/#review175171
-----------------------------------------------------------
On May 16, 2017, 10:48 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59316/
> -----------------------------------------------------------
>
> (Updated May 16, 2017, 10:48 p.m.)
>
>
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
>
>
> Repository: geode
>
>
> Description
> -------
>
> * using local mbs server connection will bypass all the mbean security checks
> * do not update the mbean attribute since pulse user has no cluster:write privilege at all
> * Created EmbeddedPulseRule for tests
> * simplify PulseAppListener
> * use log4j2 logging configurations
>
>
> Diffs
> -----
>
> geode-pulse/src/main/resources/log4j2.properties PRE-CREATION
> geode-pulse/src/main/resources/log4j2.xml PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59316/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 59316: GEODE-2927: fix pulse logging and
useLocator, SSL flags
Posted by Jared Stewart <js...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59316/#review175171
-----------------------------------------------------------
geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/EmbeddedPulseRule.java
Lines 38 (patched)
<https://reviews.apache.org/r/59316/#comment248561>
I was a bit surprised that these `set` methods need to be called after `before`. Perhaps this would be more clear if `getRepository()` was replaced with a method like
```
public Cluster connect() {
return repository.getCluster();
}
```
that indicates the point where the `set` methods must be called before.
geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/IClusterUpdater.java
Lines 36 (patched)
<https://reviews.apache.org/r/59316/#comment248562>
What is the benefit of a default implementation that returns null?
- Jared Stewart
On May 16, 2017, 10:48 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59316/
> -----------------------------------------------------------
>
> (Updated May 16, 2017, 10:48 p.m.)
>
>
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
>
>
> Repository: geode
>
>
> Description
> -------
>
> * using local mbs server connection will bypass all the mbean security checks
> * do not update the mbean attribute since pulse user has no cluster:write privilege at all
> * Created EmbeddedPulseRule for tests
> * simplify PulseAppListener
> * use log4j2 logging configurations
>
>
> Diffs
> -----
>
> geode-pulse/src/main/resources/log4j2.properties PRE-CREATION
> geode-pulse/src/main/resources/log4j2.xml PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59316/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 59316: GEODE-2927: fix pulse logging and
useLocator, SSL flags
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59316/
-----------------------------------------------------------
(Updated May 16, 2017, 10:48 p.m.)
Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
Repository: geode
Description
-------
* using local mbs server connection will bypass all the mbean security checks
* do not update the mbean attribute since pulse user has no cluster:write privilege at all
* Created EmbeddedPulseRule for tests
* simplify PulseAppListener
* use log4j2 logging configurations
Diffs (updated)
-----
geode-pulse/src/main/resources/log4j2.properties PRE-CREATION
geode-pulse/src/main/resources/log4j2.xml PRE-CREATION
Diff: https://reviews.apache.org/r/59316/diff/2/
Changes: https://reviews.apache.org/r/59316/diff/1-2/
Testing
-------
Thanks,
Jinmei Liao