Re: Review Request 56635: GEODE-2481: extract default properties generation to its own class

2017-02-15 Thread Kirk Lund 


> On Feb. 14, 2017, 6:57 p.m., Jared Stewart wrote:
> > geode-core/src/test/java/org/apache/geode/distributed/internal/DefaultPropertiesGeneratorIntegrationTest.java,
> >  line 20
> > 
> >
> > The test does not compile for me as this import (`import static 
> > org.apache.geode.internal.lang.SystemUtils.getClassPath;`) does not 
> > resolve.  It looks like `import static 
> > org.apache.bcel.util.ClassPath.getClassPath;` is the the right one.
> 
> Kirk Lund wrote:
> No it's definitely import static 
> org.apache.geode.internal.lang.SystemUtils.getClassPath. Trying to break the 
> branch into multiple reviews seems to not be working. The diff should include 
> SystemUtils which now has some new methods. I guess look at my other review 
> and you'll see SystemUtils changes there.

I'm going to close these reviews and submit one big new review.


- Kirk


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


On Feb. 14, 2017, 2:33 a.m., Kirk Lund wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56635/
> ---
> 
> (Updated Feb. 14, 2017, 2:33 a.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, and Ken 
> Howe.
> 
> 
> Bugs: GEODE-2481
> https://issues.apache.org/jira/browse/GEODE-2481
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-2481: extract default properties generation to its own class
> 
> While refactoring GemFireVersion for GEODE-2474, I noticed that 
> GemFireVersionIntegrationJUnitTest has nothing to do with GemFireVersion.
> 
> Extract generation of default properties to DefaultPropertiesGenerator.
> 
> Rename GemFireVersionIntegrationJUnitTest to 
> DefaultPropertiesGeneratorIntegrationTest. Add tests to increase code 
> coverage.
> 
> Note: I have several changes on feature/GEODE-2474 branch which I'll separate 
> into multiple reviews. I'll do a final precheckin on the entire branch and 
> then merge the changes in after everything passes review and precheckin.
> 
> DistributionConfig and DefaultPropertiesGenerator should eventually move to a 
> configuration package, but I don't really want to combine anything that big 
> with this change.
> 
> 
> Diffs
> -
> 
>   geode-assembly/build.gradle f34688043dd3e6bf8e8bdf0cb223d533b692e301 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/DefaultPropertiesGenerator.java
>  PRE-CREATION 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java
>  fa6d13f7cec40ae18f78da28b3b912e01be363aa 
>   
> geode-core/src/test/java/org/apache/geode/distributed/internal/DefaultPropertiesGeneratorIntegrationTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/internal/GemFireVersionIntegrationJUnitTest.java
>  cae331325f17b470e6dd786d0f9a52bba7cb42a6 
> 
> Diff: https://reviews.apache.org/r/56635/diff/
> 
> 
> Testing
> ---
> 
> DefaultPropertiesGeneratorIntegrationTest
> GemFireVersionJUnitTest
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 56635: GEODE-2481: extract default properties generation to its own class

2017-02-15 Thread Kirk Lund 


> On Feb. 14, 2017, 6:57 p.m., Jared Stewart wrote:
> > geode-core/src/test/java/org/apache/geode/distributed/internal/DefaultPropertiesGeneratorIntegrationTest.java,
> >  line 20
> > 
> >
> > The test does not compile for me as this import (`import static 
> > org.apache.geode.internal.lang.SystemUtils.getClassPath;`) does not 
> > resolve.  It looks like `import static 
> > org.apache.bcel.util.ClassPath.getClassPath;` is the the right one.

No it's definitely import static 
org.apache.geode.internal.lang.SystemUtils.getClassPath. Trying to break the 
branch into multiple reviews seems to not be working. The diff should include 
SystemUtils which now has some new methods. I guess look at my other review and 
you'll see SystemUtils changes there.


- Kirk


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


On Feb. 14, 2017, 2:33 a.m., Kirk Lund wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56635/
> ---
> 
> (Updated Feb. 14, 2017, 2:33 a.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, and Ken 
> Howe.
> 
> 
> Bugs: GEODE-2481
> https://issues.apache.org/jira/browse/GEODE-2481
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-2481: extract default properties generation to its own class
> 
> While refactoring GemFireVersion for GEODE-2474, I noticed that 
> GemFireVersionIntegrationJUnitTest has nothing to do with GemFireVersion.
> 
> Extract generation of default properties to DefaultPropertiesGenerator.
> 
> Rename GemFireVersionIntegrationJUnitTest to 
> DefaultPropertiesGeneratorIntegrationTest. Add tests to increase code 
> coverage.
> 
> Note: I have several changes on feature/GEODE-2474 branch which I'll separate 
> into multiple reviews. I'll do a final precheckin on the entire branch and 
> then merge the changes in after everything passes review and precheckin.
> 
> DistributionConfig and DefaultPropertiesGenerator should eventually move to a 
> configuration package, but I don't really want to combine anything that big 
> with this change.
> 
> 
> Diffs
> -
> 
>   geode-assembly/build.gradle f34688043dd3e6bf8e8bdf0cb223d533b692e301 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/DefaultPropertiesGenerator.java
>  PRE-CREATION 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java
>  fa6d13f7cec40ae18f78da28b3b912e01be363aa 
>   
> geode-core/src/test/java/org/apache/geode/distributed/internal/DefaultPropertiesGeneratorIntegrationTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/internal/GemFireVersionIntegrationJUnitTest.java
>  cae331325f17b470e6dd786d0f9a52bba7cb42a6 
> 
> Diff: https://reviews.apache.org/r/56635/diff/
> 
> 
> Testing
> ---
> 
> DefaultPropertiesGeneratorIntegrationTest
> GemFireVersionJUnitTest
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 56635: GEODE-2481: extract default properties generation to its own class

2017-02-15 Thread Kevin Duling 

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


Ship it!




Ship It!

- Kevin Duling


On Feb. 13, 2017, 6:33 p.m., Kirk Lund wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56635/
> ---
> 
> (Updated Feb. 13, 2017, 6:33 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, and Ken 
> Howe.
> 
> 
> Bugs: GEODE-2481
> https://issues.apache.org/jira/browse/GEODE-2481
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-2481: extract default properties generation to its own class
> 
> While refactoring GemFireVersion for GEODE-2474, I noticed that 
> GemFireVersionIntegrationJUnitTest has nothing to do with GemFireVersion.
> 
> Extract generation of default properties to DefaultPropertiesGenerator.
> 
> Rename GemFireVersionIntegrationJUnitTest to 
> DefaultPropertiesGeneratorIntegrationTest. Add tests to increase code 
> coverage.
> 
> Note: I have several changes on feature/GEODE-2474 branch which I'll separate 
> into multiple reviews. I'll do a final precheckin on the entire branch and 
> then merge the changes in after everything passes review and precheckin.
> 
> DistributionConfig and DefaultPropertiesGenerator should eventually move to a 
> configuration package, but I don't really want to combine anything that big 
> with this change.
> 
> 
> Diffs
> -
> 
>   geode-assembly/build.gradle f34688043dd3e6bf8e8bdf0cb223d533b692e301 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/DefaultPropertiesGenerator.java
>  PRE-CREATION 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java
>  fa6d13f7cec40ae18f78da28b3b912e01be363aa 
>   
> geode-core/src/test/java/org/apache/geode/distributed/internal/DefaultPropertiesGeneratorIntegrationTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/internal/GemFireVersionIntegrationJUnitTest.java
>  cae331325f17b470e6dd786d0f9a52bba7cb42a6 
> 
> Diff: https://reviews.apache.org/r/56635/diff/
> 
> 
> Testing
> ---
> 
> DefaultPropertiesGeneratorIntegrationTest
> GemFireVersionJUnitTest
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 56635: GEODE-2481: extract default properties generation to its own class

2017-02-14 Thread Jared Stewart 

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




geode-core/src/test/java/org/apache/geode/distributed/internal/DefaultPropertiesGeneratorIntegrationTest.java
 (line 20)


The test does not compile for me as this import (`import static 
org.apache.geode.internal.lang.SystemUtils.getClassPath;`) does not resolve.  
It looks like `import static org.apache.bcel.util.ClassPath.getClassPath;` is 
the the right one.


- Jared Stewart


On Feb. 14, 2017, 2:33 a.m., Kirk Lund wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56635/
> ---
> 
> (Updated Feb. 14, 2017, 2:33 a.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, and Ken 
> Howe.
> 
> 
> Bugs: GEODE-2481
> https://issues.apache.org/jira/browse/GEODE-2481
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-2481: extract default properties generation to its own class
> 
> While refactoring GemFireVersion for GEODE-2474, I noticed that 
> GemFireVersionIntegrationJUnitTest has nothing to do with GemFireVersion.
> 
> Extract generation of default properties to DefaultPropertiesGenerator.
> 
> Rename GemFireVersionIntegrationJUnitTest to 
> DefaultPropertiesGeneratorIntegrationTest. Add tests to increase code 
> coverage.
> 
> Note: I have several changes on feature/GEODE-2474 branch which I'll separate 
> into multiple reviews. I'll do a final precheckin on the entire branch and 
> then merge the changes in after everything passes review and precheckin.
> 
> DistributionConfig and DefaultPropertiesGenerator should eventually move to a 
> configuration package, but I don't really want to combine anything that big 
> with this change.
> 
> 
> Diffs
> -
> 
>   geode-assembly/build.gradle f34688043dd3e6bf8e8bdf0cb223d533b692e301 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/DefaultPropertiesGenerator.java
>  PRE-CREATION 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java
>  fa6d13f7cec40ae18f78da28b3b912e01be363aa 
>   
> geode-core/src/test/java/org/apache/geode/distributed/internal/DefaultPropertiesGeneratorIntegrationTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/internal/GemFireVersionIntegrationJUnitTest.java
>  cae331325f17b470e6dd786d0f9a52bba7cb42a6 
> 
> Diff: https://reviews.apache.org/r/56635/diff/
> 
> 
> Testing
> ---
> 
> DefaultPropertiesGeneratorIntegrationTest
> GemFireVersionJUnitTest
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 56635: GEODE-2481: extract default properties generation to its own class

2017-02-14 Thread Jinmei Liao 

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


Ship it!




Ship It!

- Jinmei Liao


On Feb. 14, 2017, 2:33 a.m., Kirk Lund wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56635/
> ---
> 
> (Updated Feb. 14, 2017, 2:33 a.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, and Ken 
> Howe.
> 
> 
> Bugs: GEODE-2481
> https://issues.apache.org/jira/browse/GEODE-2481
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-2481: extract default properties generation to its own class
> 
> While refactoring GemFireVersion for GEODE-2474, I noticed that 
> GemFireVersionIntegrationJUnitTest has nothing to do with GemFireVersion.
> 
> Extract generation of default properties to DefaultPropertiesGenerator.
> 
> Rename GemFireVersionIntegrationJUnitTest to 
> DefaultPropertiesGeneratorIntegrationTest. Add tests to increase code 
> coverage.
> 
> Note: I have several changes on feature/GEODE-2474 branch which I'll separate 
> into multiple reviews. I'll do a final precheckin on the entire branch and 
> then merge the changes in after everything passes review and precheckin.
> 
> DistributionConfig and DefaultPropertiesGenerator should eventually move to a 
> configuration package, but I don't really want to combine anything that big 
> with this change.
> 
> 
> Diffs
> -
> 
>   geode-assembly/build.gradle f34688043dd3e6bf8e8bdf0cb223d533b692e301 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/DefaultPropertiesGenerator.java
>  PRE-CREATION 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java
>  fa6d13f7cec40ae18f78da28b3b912e01be363aa 
>   
> geode-core/src/test/java/org/apache/geode/distributed/internal/DefaultPropertiesGeneratorIntegrationTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/internal/GemFireVersionIntegrationJUnitTest.java
>  cae331325f17b470e6dd786d0f9a52bba7cb42a6 
> 
> Diff: https://reviews.apache.org/r/56635/diff/
> 
> 
> Testing
> ---
> 
> DefaultPropertiesGeneratorIntegrationTest
> GemFireVersionJUnitTest
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 56635: GEODE-2481: extract default properties generation to its own class

2017-02-13 Thread Kirk Lund 

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

(Updated Feb. 14, 2017, 2:33 a.m.)


Review request for geode, Jinmei Liao, Jared Stewart, Kevin Duling, and Ken 
Howe.


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


Repository: geode


Description (updated)
---

GEODE-2481: extract default properties generation to its own class

While refactoring GemFireVersion for GEODE-2474, I noticed that 
GemFireVersionIntegrationJUnitTest has nothing to do with GemFireVersion.

Extract generation of default properties to DefaultPropertiesGenerator.

Rename GemFireVersionIntegrationJUnitTest to 
DefaultPropertiesGeneratorIntegrationTest. Add tests to increase code coverage.

Note: I have several changes on feature/GEODE-2474 branch which I'll separate 
into multiple reviews. I'll do a final precheckin on the entire branch and then 
merge the changes in after everything passes review and precheckin.

DistributionConfig and DefaultPropertiesGenerator should eventually move to a 
configuration package, but I don't really want to combine anything that big 
with this change.


Diffs
-

  geode-assembly/build.gradle f34688043dd3e6bf8e8bdf0cb223d533b692e301 
  
geode-core/src/main/java/org/apache/geode/distributed/internal/DefaultPropertiesGenerator.java
 PRE-CREATION 
  
geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java
 fa6d13f7cec40ae18f78da28b3b912e01be363aa 
  
geode-core/src/test/java/org/apache/geode/distributed/internal/DefaultPropertiesGeneratorIntegrationTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/internal/GemFireVersionIntegrationJUnitTest.java
 cae331325f17b470e6dd786d0f9a52bba7cb42a6 

Diff: https://reviews.apache.org/r/56635/diff/


Testing
---

DefaultPropertiesGeneratorIntegrationTest
GemFireVersionJUnitTest


Thanks,

Kirk Lund