Re: Review Request 59316: GEODE-2927: fix pulse logging and useLocator, SSL flags

2017-05-17 Thread Jared Stewart 

---
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

2017-05-17 Thread Jinmei Liao 


> 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)
> > 
> >
> > 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)
> > 
> >
> > 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

2017-05-17 Thread Jinmei Liao 

---
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

2017-05-16 Thread Jared Stewart 

---
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)


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)


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

2017-05-16 Thread Jinmei Liao 

---
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

Re: Review Request 59316: GEODE-2927: fix pulse logging and useLocator, SSL flags

2017-05-16 Thread Jinmei Liao 


> On May 16, 2017, 6:59 p.m., Kirk Lund wrote:
> > geode-pulse/src/main/resources/log4j2.properties
> > Lines 17 (patched)
> > 
> >
> > 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

2017-05-16 Thread Kirk Lund 

---
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)


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
> 
>