Re: Review Request 60010: GEODE-3062: replace SecurityService after loading cluster config

2017-06-14 Thread Ken Howe 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60010/#review177928
---


Ship it!




Ship It!

- Ken Howe


On June 13, 2017, 4:29 p.m., Kirk Lund wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60010/
> ---
> 
> (Updated June 13, 2017, 4:29 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Ken Howe, 
> and Patrick Rhomberg.
> 
> 
> Bugs: GEODE-3062
> https://issues.apache.org/jira/browse/GEODE-3062
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Add new test to ClusterConfigWithSecurityDUnitTest that fails due to bug 
> GEODE-3062.
> 
> Remove unused Cache param from applyClusterPropertiesConfiguration so it can 
> be called during Cache construction.
> 
> Move cluster config request to Cache construction and handle jars and 
> properties there. Create new SecurityService in constructor and overwrite the 
> SecurityService in InternalDistributedSystem.
> 
> NOTE: We will later have to fix GEODE-3061 by moving cluster config request 
> from Cache to InternalDistributedSystem construction so that IDS can be 
> created with gemfire.properties from cluster config. At that time we would 
> rip out both cluster config request and creation of security service from 
> Cache construction and pass both into Cache construction.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java
>  22edb6f06c7791929cc9a033ca1a1bfed5751a47 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java
>  4f4881fe39116faa505bc2fbec74efd669efe0ef 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
>  40df0c7dcac8827a381c268c1f90e6acfb97a7f1 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigWithSecurityDUnitTest.java
>  c551ca9104a85dcf54c0bebbc4178fce4114a416 
> 
> 
> Diff: https://reviews.apache.org/r/60010/diff/2/
> 
> 
> Testing
> ---
> 
> Precheckin passes
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 60010: GEODE-3062: replace SecurityService after loading cluster config

2017-06-14 Thread Jared Stewart 


> On June 12, 2017, 7:59 p.m., Jared Stewart wrote:
> > geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
> > Lines 327 (patched)
> > 
> >
> > Are you sure that we want to keep this response around as a member 
> > variable?  I'm afraid that it might be stale (in the sense that the cluster 
> > config has since been changed) if this response is referred to later on.
> 
> Kirk Lund wrote:
> Please see the new diff. I changed it from final to volatile and null it 
> out when the GemFireCacheImpl is finished using it. The only other way to 
> handle it would be as a ThreadLocal instead of a member variable. The 
> advantage of a ThreadLocal is that it wouldn't permanently use an object 
> header worth of heap memory after initialization completes.

Ah, I see - `initialize()` is not called by the constructor but rather through 
.  I was going to propose passing `configurationReponse` as an argument to 
`intialize`, but that won't work here given the way `initialize` is called.


- Jared


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60010/#review177668
---


On June 13, 2017, 4:29 p.m., Kirk Lund wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60010/
> ---
> 
> (Updated June 13, 2017, 4:29 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Ken Howe, 
> and Patrick Rhomberg.
> 
> 
> Bugs: GEODE-3062
> https://issues.apache.org/jira/browse/GEODE-3062
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Add new test to ClusterConfigWithSecurityDUnitTest that fails due to bug 
> GEODE-3062.
> 
> Remove unused Cache param from applyClusterPropertiesConfiguration so it can 
> be called during Cache construction.
> 
> Move cluster config request to Cache construction and handle jars and 
> properties there. Create new SecurityService in constructor and overwrite the 
> SecurityService in InternalDistributedSystem.
> 
> NOTE: We will later have to fix GEODE-3061 by moving cluster config request 
> from Cache to InternalDistributedSystem construction so that IDS can be 
> created with gemfire.properties from cluster config. At that time we would 
> rip out both cluster config request and creation of security service from 
> Cache construction and pass both into Cache construction.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java
>  22edb6f06c7791929cc9a033ca1a1bfed5751a47 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java
>  4f4881fe39116faa505bc2fbec74efd669efe0ef 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
>  40df0c7dcac8827a381c268c1f90e6acfb97a7f1 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigWithSecurityDUnitTest.java
>  c551ca9104a85dcf54c0bebbc4178fce4114a416 
> 
> 
> Diff: https://reviews.apache.org/r/60010/diff/2/
> 
> 
> Testing
> ---
> 
> Precheckin passes
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 60010: GEODE-3062: replace SecurityService after loading cluster config

2017-06-14 Thread Jared Stewart 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60010/#review177927
---


Ship it!




Ship It!

- Jared Stewart


On June 13, 2017, 4:29 p.m., Kirk Lund wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60010/
> ---
> 
> (Updated June 13, 2017, 4:29 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Ken Howe, 
> and Patrick Rhomberg.
> 
> 
> Bugs: GEODE-3062
> https://issues.apache.org/jira/browse/GEODE-3062
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Add new test to ClusterConfigWithSecurityDUnitTest that fails due to bug 
> GEODE-3062.
> 
> Remove unused Cache param from applyClusterPropertiesConfiguration so it can 
> be called during Cache construction.
> 
> Move cluster config request to Cache construction and handle jars and 
> properties there. Create new SecurityService in constructor and overwrite the 
> SecurityService in InternalDistributedSystem.
> 
> NOTE: We will later have to fix GEODE-3061 by moving cluster config request 
> from Cache to InternalDistributedSystem construction so that IDS can be 
> created with gemfire.properties from cluster config. At that time we would 
> rip out both cluster config request and creation of security service from 
> Cache construction and pass both into Cache construction.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java
>  22edb6f06c7791929cc9a033ca1a1bfed5751a47 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java
>  4f4881fe39116faa505bc2fbec74efd669efe0ef 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
>  40df0c7dcac8827a381c268c1f90e6acfb97a7f1 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigWithSecurityDUnitTest.java
>  c551ca9104a85dcf54c0bebbc4178fce4114a416 
> 
> 
> Diff: https://reviews.apache.org/r/60010/diff/2/
> 
> 
> Testing
> ---
> 
> Precheckin passes
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 60010: GEODE-3062: replace SecurityService after loading cluster config

2017-06-13 Thread Patrick Rhomberg 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60010/#review177784
---




geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigWithSecurityDUnitTest.java
Line 67 (original), 68 (patched)


You're doing the lords' work here.


- Patrick Rhomberg


On June 13, 2017, 4:29 p.m., Kirk Lund wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60010/
> ---
> 
> (Updated June 13, 2017, 4:29 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Ken Howe, 
> and Patrick Rhomberg.
> 
> 
> Bugs: GEODE-3062
> https://issues.apache.org/jira/browse/GEODE-3062
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Add new test to ClusterConfigWithSecurityDUnitTest that fails due to bug 
> GEODE-3062.
> 
> Remove unused Cache param from applyClusterPropertiesConfiguration so it can 
> be called during Cache construction.
> 
> Move cluster config request to Cache construction and handle jars and 
> properties there. Create new SecurityService in constructor and overwrite the 
> SecurityService in InternalDistributedSystem.
> 
> NOTE: We will later have to fix GEODE-3061 by moving cluster config request 
> from Cache to InternalDistributedSystem construction so that IDS can be 
> created with gemfire.properties from cluster config. At that time we would 
> rip out both cluster config request and creation of security service from 
> Cache construction and pass both into Cache construction.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java
>  22edb6f06c7791929cc9a033ca1a1bfed5751a47 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java
>  4f4881fe39116faa505bc2fbec74efd669efe0ef 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
>  40df0c7dcac8827a381c268c1f90e6acfb97a7f1 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigWithSecurityDUnitTest.java
>  c551ca9104a85dcf54c0bebbc4178fce4114a416 
> 
> 
> Diff: https://reviews.apache.org/r/60010/diff/2/
> 
> 
> Testing
> ---
> 
> Precheckin passes
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 60010: GEODE-3062: replace SecurityService after loading cluster config

2017-06-13 Thread Patrick Rhomberg 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60010/#review177783
---


Ship it!




Ship It!

- Patrick Rhomberg


On June 13, 2017, 4:29 p.m., Kirk Lund wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60010/
> ---
> 
> (Updated June 13, 2017, 4:29 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Ken Howe, 
> and Patrick Rhomberg.
> 
> 
> Bugs: GEODE-3062
> https://issues.apache.org/jira/browse/GEODE-3062
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Add new test to ClusterConfigWithSecurityDUnitTest that fails due to bug 
> GEODE-3062.
> 
> Remove unused Cache param from applyClusterPropertiesConfiguration so it can 
> be called during Cache construction.
> 
> Move cluster config request to Cache construction and handle jars and 
> properties there. Create new SecurityService in constructor and overwrite the 
> SecurityService in InternalDistributedSystem.
> 
> NOTE: We will later have to fix GEODE-3061 by moving cluster config request 
> from Cache to InternalDistributedSystem construction so that IDS can be 
> created with gemfire.properties from cluster config. At that time we would 
> rip out both cluster config request and creation of security service from 
> Cache construction and pass both into Cache construction.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java
>  22edb6f06c7791929cc9a033ca1a1bfed5751a47 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java
>  4f4881fe39116faa505bc2fbec74efd669efe0ef 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
>  40df0c7dcac8827a381c268c1f90e6acfb97a7f1 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigWithSecurityDUnitTest.java
>  c551ca9104a85dcf54c0bebbc4178fce4114a416 
> 
> 
> Diff: https://reviews.apache.org/r/60010/diff/2/
> 
> 
> Testing
> ---
> 
> Precheckin passes
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 60010: GEODE-3062: replace SecurityService after loading cluster config

2017-06-13 Thread Emily Yeh 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60010/#review19
---


Ship it!




Ship It!

- Emily Yeh


On June 13, 2017, 4:29 p.m., Kirk Lund wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60010/
> ---
> 
> (Updated June 13, 2017, 4:29 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Ken Howe, 
> and Patrick Rhomberg.
> 
> 
> Bugs: GEODE-3062
> https://issues.apache.org/jira/browse/GEODE-3062
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Add new test to ClusterConfigWithSecurityDUnitTest that fails due to bug 
> GEODE-3062.
> 
> Remove unused Cache param from applyClusterPropertiesConfiguration so it can 
> be called during Cache construction.
> 
> Move cluster config request to Cache construction and handle jars and 
> properties there. Create new SecurityService in constructor and overwrite the 
> SecurityService in InternalDistributedSystem.
> 
> NOTE: We will later have to fix GEODE-3061 by moving cluster config request 
> from Cache to InternalDistributedSystem construction so that IDS can be 
> created with gemfire.properties from cluster config. At that time we would 
> rip out both cluster config request and creation of security service from 
> Cache construction and pass both into Cache construction.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java
>  22edb6f06c7791929cc9a033ca1a1bfed5751a47 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java
>  4f4881fe39116faa505bc2fbec74efd669efe0ef 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
>  40df0c7dcac8827a381c268c1f90e6acfb97a7f1 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigWithSecurityDUnitTest.java
>  c551ca9104a85dcf54c0bebbc4178fce4114a416 
> 
> 
> Diff: https://reviews.apache.org/r/60010/diff/2/
> 
> 
> Testing
> ---
> 
> Precheckin passes
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 60010: GEODE-3062: replace SecurityService after loading cluster config

2017-06-13 Thread Jinmei Liao 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60010/#review177755
---


Ship it!




Ship It!

- Jinmei Liao


On June 13, 2017, 4:29 p.m., Kirk Lund wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60010/
> ---
> 
> (Updated June 13, 2017, 4:29 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Ken Howe, 
> and Patrick Rhomberg.
> 
> 
> Bugs: GEODE-3062
> https://issues.apache.org/jira/browse/GEODE-3062
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Add new test to ClusterConfigWithSecurityDUnitTest that fails due to bug 
> GEODE-3062.
> 
> Remove unused Cache param from applyClusterPropertiesConfiguration so it can 
> be called during Cache construction.
> 
> Move cluster config request to Cache construction and handle jars and 
> properties there. Create new SecurityService in constructor and overwrite the 
> SecurityService in InternalDistributedSystem.
> 
> NOTE: We will later have to fix GEODE-3061 by moving cluster config request 
> from Cache to InternalDistributedSystem construction so that IDS can be 
> created with gemfire.properties from cluster config. At that time we would 
> rip out both cluster config request and creation of security service from 
> Cache construction and pass both into Cache construction.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java
>  22edb6f06c7791929cc9a033ca1a1bfed5751a47 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java
>  4f4881fe39116faa505bc2fbec74efd669efe0ef 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
>  40df0c7dcac8827a381c268c1f90e6acfb97a7f1 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigWithSecurityDUnitTest.java
>  c551ca9104a85dcf54c0bebbc4178fce4114a416 
> 
> 
> Diff: https://reviews.apache.org/r/60010/diff/2/
> 
> 
> Testing
> ---
> 
> Precheckin passes
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 60010: GEODE-3062: replace SecurityService after loading cluster config

2017-06-13 Thread Kirk Lund 


> On June 12, 2017, 7:59 p.m., Jared Stewart wrote:
> > geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
> > Lines 327 (patched)
> > 
> >
> > Are you sure that we want to keep this response around as a member 
> > variable?  I'm afraid that it might be stale (in the sense that the cluster 
> > config has since been changed) if this response is referred to later on.

Please see the new diff. I changed it from final to volatile and null it out 
when the GemFireCacheImpl is finished using it. The only other way to handle it 
would be as a ThreadLocal instead of a member variable. The advantage of a 
ThreadLocal is that it wouldn't permanently use an object header worth of heap 
memory after initialization completes.


- Kirk


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60010/#review177668
---


On June 13, 2017, 4:29 p.m., Kirk Lund wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60010/
> ---
> 
> (Updated June 13, 2017, 4:29 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Ken Howe, 
> and Patrick Rhomberg.
> 
> 
> Bugs: GEODE-3062
> https://issues.apache.org/jira/browse/GEODE-3062
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Add new test to ClusterConfigWithSecurityDUnitTest that fails due to bug 
> GEODE-3062.
> 
> Remove unused Cache param from applyClusterPropertiesConfiguration so it can 
> be called during Cache construction.
> 
> Move cluster config request to Cache construction and handle jars and 
> properties there. Create new SecurityService in constructor and overwrite the 
> SecurityService in InternalDistributedSystem.
> 
> NOTE: We will later have to fix GEODE-3061 by moving cluster config request 
> from Cache to InternalDistributedSystem construction so that IDS can be 
> created with gemfire.properties from cluster config. At that time we would 
> rip out both cluster config request and creation of security service from 
> Cache construction and pass both into Cache construction.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java
>  22edb6f06c7791929cc9a033ca1a1bfed5751a47 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java
>  4f4881fe39116faa505bc2fbec74efd669efe0ef 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
>  40df0c7dcac8827a381c268c1f90e6acfb97a7f1 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigWithSecurityDUnitTest.java
>  c551ca9104a85dcf54c0bebbc4178fce4114a416 
> 
> 
> Diff: https://reviews.apache.org/r/60010/diff/2/
> 
> 
> Testing
> ---
> 
> Precheckin passes
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 60010: GEODE-3062: replace SecurityService after loading cluster config

2017-06-13 Thread Kirk Lund 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60010/
---

(Updated June 13, 2017, 4:29 p.m.)


Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Ken Howe, and 
Patrick Rhomberg.


Changes
---

Null out configurationResponse after finishing with it (it's loaded in 
constructor, used there and then used again in initialize)


Bugs: GEODE-3062
https://issues.apache.org/jira/browse/GEODE-3062


Repository: geode


Description
---

Add new test to ClusterConfigWithSecurityDUnitTest that fails due to bug 
GEODE-3062.

Remove unused Cache param from applyClusterPropertiesConfiguration so it can be 
called during Cache construction.

Move cluster config request to Cache construction and handle jars and 
properties there. Create new SecurityService in constructor and overwrite the 
SecurityService in InternalDistributedSystem.

NOTE: We will later have to fix GEODE-3061 by moving cluster config request 
from Cache to InternalDistributedSystem construction so that IDS can be created 
with gemfire.properties from cluster config. At that time we would rip out both 
cluster config request and creation of security service from Cache construction 
and pass both into Cache construction.


Diffs (updated)
-

  
geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java
 22edb6f06c7791929cc9a033ca1a1bfed5751a47 
  
geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java
 4f4881fe39116faa505bc2fbec74efd669efe0ef 
  
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java 
40df0c7dcac8827a381c268c1f90e6acfb97a7f1 
  
geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigWithSecurityDUnitTest.java
 c551ca9104a85dcf54c0bebbc4178fce4114a416 


Diff: https://reviews.apache.org/r/60010/diff/2/

Changes: https://reviews.apache.org/r/60010/diff/1-2/


Testing
---

Precheckin passes


Thanks,

Kirk Lund

Re: Review Request 60010: GEODE-3062: replace SecurityService after loading cluster config

2017-06-12 Thread Jared Stewart 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60010/#review177668
---




geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
Lines 327 (patched)


Are you sure that we want to keep this response around as a member 
variable?  I'm afraid that it might be stale (in the sense that the cluster 
config has since been changed) if this response is referred to later on.


- Jared Stewart


On June 12, 2017, 5:40 p.m., Kirk Lund wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60010/
> ---
> 
> (Updated June 12, 2017, 5:40 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Ken Howe, 
> and Patrick Rhomberg.
> 
> 
> Bugs: GEODE-3062
> https://issues.apache.org/jira/browse/GEODE-3062
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Add new test to ClusterConfigWithSecurityDUnitTest that fails due to bug 
> GEODE-3062.
> 
> Remove unused Cache param from applyClusterPropertiesConfiguration so it can 
> be called during Cache construction.
> 
> Move cluster config request to Cache construction and handle jars and 
> properties there. Create new SecurityService in constructor and overwrite the 
> SecurityService in InternalDistributedSystem.
> 
> NOTE: We will later have to fix GEODE-3061 by moving cluster config request 
> from Cache to InternalDistributedSystem construction so that IDS can be 
> created with gemfire.properties from cluster config. At that time we would 
> rip out both cluster config request and creation of security service from 
> Cache construction and pass both into Cache construction.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java
>  22edb6f06c7791929cc9a033ca1a1bfed5751a47 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java
>  4f4881fe39116faa505bc2fbec74efd669efe0ef 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
>  40df0c7dcac8827a381c268c1f90e6acfb97a7f1 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigWithSecurityDUnitTest.java
>  c551ca9104a85dcf54c0bebbc4178fce4114a416 
> 
> 
> Diff: https://reviews.apache.org/r/60010/diff/1/
> 
> 
> Testing
> ---
> 
> Precheckin passes
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 60010: GEODE-3062: replace SecurityService after loading cluster config

2017-06-12 Thread Jinmei Liao 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60010/#review177650
---


Ship it!




Ship It!

- Jinmei Liao


On June 12, 2017, 5:40 p.m., Kirk Lund wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60010/
> ---
> 
> (Updated June 12, 2017, 5:40 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Ken Howe, 
> and Patrick Rhomberg.
> 
> 
> Bugs: GEODE-3062
> https://issues.apache.org/jira/browse/GEODE-3062
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Add new test to ClusterConfigWithSecurityDUnitTest that fails due to bug 
> GEODE-3062.
> 
> Remove unused Cache param from applyClusterPropertiesConfiguration so it can 
> be called during Cache construction.
> 
> Move cluster config request to Cache construction and handle jars and 
> properties there. Create new SecurityService in constructor and overwrite the 
> SecurityService in InternalDistributedSystem.
> 
> NOTE: We will later have to fix GEODE-3061 by moving cluster config request 
> from Cache to InternalDistributedSystem construction so that IDS can be 
> created with gemfire.properties from cluster config. At that time we would 
> rip out both cluster config request and creation of security service from 
> Cache construction and pass both into Cache construction.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java
>  22edb6f06c7791929cc9a033ca1a1bfed5751a47 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java
>  4f4881fe39116faa505bc2fbec74efd669efe0ef 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
>  40df0c7dcac8827a381c268c1f90e6acfb97a7f1 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigWithSecurityDUnitTest.java
>  c551ca9104a85dcf54c0bebbc4178fce4114a416 
> 
> 
> Diff: https://reviews.apache.org/r/60010/diff/1/
> 
> 
> Testing
> ---
> 
> Precheckin passes
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Review Request 60010: GEODE-3062: replace SecurityService after loading cluster config

2017-06-12 Thread Kirk Lund 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60010/
---

Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Ken Howe, and 
Patrick Rhomberg.


Bugs: GEODE-3062
https://issues.apache.org/jira/browse/GEODE-3062


Repository: geode


Description
---

Add new test to ClusterConfigWithSecurityDUnitTest that fails due to bug 
GEODE-3062.

Remove unused Cache param from applyClusterPropertiesConfiguration so it can be 
called during Cache construction.

Move cluster config request to Cache construction and handle jars and 
properties there. Create new SecurityService in constructor and overwrite the 
SecurityService in InternalDistributedSystem.

NOTE: We will later have to fix GEODE-3061 by moving cluster config request 
from Cache to InternalDistributedSystem construction so that IDS can be created 
with gemfire.properties from cluster config. At that time we would rip out both 
cluster config request and creation of security service from Cache construction 
and pass both into Cache construction.


Diffs
-

  
geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java
 22edb6f06c7791929cc9a033ca1a1bfed5751a47 
  
geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java
 4f4881fe39116faa505bc2fbec74efd669efe0ef 
  
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java 
40df0c7dcac8827a381c268c1f90e6acfb97a7f1 
  
geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigWithSecurityDUnitTest.java
 c551ca9104a85dcf54c0bebbc4178fce4114a416 


Diff: https://reviews.apache.org/r/60010/diff/1/


Testing
---

Precheckin passes


Thanks,

Kirk Lund