You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Kirk Lund <ki...@gmail.com> on 2017/04/25 16:46:35 UTC
Review Request 58712: GEODE-2632: partial cleanup of GemFireCacheImpl
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58712/
-----------------------------------------------------------
Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken Howe, and Patrick Rhomberg.
Bugs: GEODE-2632
https://issues.apache.org/jira/browse/GEODE-2632
Repository: geode
Description
-------
Fixup GemFireCacheImpl code. Also fix some affected collaborator classes and tests.
* change SecurityService from static constant to final member field
* fix typos and javadocs
* reduce scope of constants, variables, methods
* remove deadcode and useless comments/javadocs
* fix misc IntelliJ warnings
* add @Override annotations
* improve some variable names
* add TODOs for a couple problem areas that need further fixing
Diffs
-----
geode-core/src/main/java/org/apache/geode/CancelCriterion.java e4f9a41599e521ce4bfbca9538d04dcc8edaa49b
geode-core/src/main/java/org/apache/geode/cache/Cache.java bc4aa19eb99e4758512934c9b9c43bb18d4c20d8
geode-core/src/main/java/org/apache/geode/cache/client/internal/ProxyCache.java 76306f51fc9479c7d9acaa28022ed908b674b7c0
geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryMonitor.java d6acfbfc769af1cddd247dc0c3584cef00ea6f28
geode-core/src/main/java/org/apache/geode/distributed/internal/DM.java 328a4f8265e54483c6bf34c2ad967f483661f155
geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionManager.java 2ae86e65188b02ec6b8cbddc49ccbc73c55bfad1
geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java 987e4911272ba6c37046d8e39a1187b71556736d
geode-core/src/main/java/org/apache/geode/distributed/internal/LonerDistributionManager.java af4e674cf3c8c58ff23ae2e9ea620b94c81ce81b
geode-core/src/main/java/org/apache/geode/internal/cache/DistTXState.java 6df26233417ea617e31d1928081c0719e26efd99
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java 56243e1b544f5958204e64c2ca391003aa1fd098
geode-core/src/main/java/org/apache/geode/internal/cache/InternalCache.java 709308b57da847845ef9319bece18ebe9f25e569
geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 45035d725241d60cbf6ed4f5417971c967925e86
geode-core/src/main/java/org/apache/geode/internal/cache/execute/util/FindRestEnabledServersFunction.java 5da63adae8f3c0be4c2548034598d566efc517aa
geode-core/src/main/java/org/apache/geode/internal/cache/persistence/PersistenceAdvisorImpl.java 7e30141c63a446852eea67aa4594241fca362668
geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheCreation.java a5f0fc2bc7cf4250565aa8dd139004890b8da07d
geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java d6a1efa73028e1b9514db67d2e3a4b564abee632
geode-core/src/test/java/org/apache/geode/TXJUnitTest.java 54d9e503f2645d045487cea51011143602764f62
geode-core/src/test/java/org/apache/geode/TXWriterTestCase.java 987f22f688ca695a8b37eacf239c69c329bb3b3b
geode-core/src/test/java/org/apache/geode/cache/query/dunit/ResourceManagerWithQueryMonitorDUnitTest.java 903b2123097bac83045d887050f7eb955b27e3cd
geode-core/src/test/java/org/apache/geode/cache/query/internal/index/NewDeclarativeIndexCreationJUnitTest.java 8a0f31cc87aa18b7b0928ab0e02405fd3ad75a7f
geode-core/src/test/java/org/apache/geode/disttx/DistTXDebugDUnitTest.java 0d2f2b6f41e1b1dd829a41472206d0ccb6589a5e
geode-core/src/test/java/org/apache/geode/disttx/DistTXJUnitTest.java 8abccc6c1ab447ffbd77379d7cf3d1c32905728e
geode-core/src/test/java/org/apache/geode/disttx/DistTXPersistentDebugDUnitTest.java 5753f5c28de31353cdfb5235a981c1c9a88d75bd
geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterJUnitTest.java 0a61b1f258d090090321c9ccff1a25781da7c8d1
geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterOOMEJUnitTest.java b99d3fd25cdac5f1862927d098d9d6381894510e
geode-core/src/test/java/org/apache/geode/disttx/DistributedTransactionDUnitTest.java 54715659a389c01b1b4b3fa79642d68f00b52b48
geode-core/src/test/java/org/apache/geode/disttx/PRDistTXJUnitTest.java f27c0993a872b9879f5ee93b577939ee2b6919d9
geode-core/src/test/java/org/apache/geode/internal/cache/PRTXJUnitTest.java d2bad641a47f68edb22da0f89a04c462ab48cd33
geode-core/src/test/java/org/apache/geode/internal/cache/wan/parallel/ParallelQueueRemovalMessageJUnitTest.java d57ce125b9498f6439dbd0b13c72c67db6badb30
geode-cq/src/main/java/org/apache/geode/cache/query/internal/cq/CqServiceImpl.java 570c06c5a4266b84c73e774bc3a021b39fa37b29
geode-cq/src/test/java/org/apache/geode/cache/query/dunit/QueryMonitorDUnitTest.java f298fae6f1840302bc98668a09e4a9b2ed0c0b5c
geode-web-api/src/main/java/org/apache/geode/rest/internal/web/controllers/CommonCrudController.java 0449a45c35569c672af06efdd9a1365283c435c3
Diff: https://reviews.apache.org/r/58712/diff/1/
Testing
-------
precheckin in progress
Thanks,
Kirk Lund
Re: Review Request 58712: GEODE-2632: partial cleanup of
GemFireCacheImpl
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58712/#review172947
-----------------------------------------------------------
I'm not sure how the final key word got added to several methods. This should be removed.
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
Line 1138 (original), 1083 (patched)
<https://reviews.apache.org/r/58712/#comment245944>
TODO: remove final keyword
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
Lines 1405 (patched)
<https://reviews.apache.org/r/58712/#comment245946>
TODO: change these repeated close-inside-try-blocks to use a common closeQuietly method
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
Line 2089 (original), 2029 (patched)
<https://reviews.apache.org/r/58712/#comment245947>
TODO: remove final keyword
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
Line 2099 (original), 2039 (patched)
<https://reviews.apache.org/r/58712/#comment245948>
TODO: remove final keyword
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
Line 4574 (original), 4272 (patched)
<https://reviews.apache.org/r/58712/#comment245951>
TODO: Remove final keyword
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
Line 5514 (original), 5136 (patched)
<https://reviews.apache.org/r/58712/#comment245953>
TODO: remove final keyword
geode-core/src/test/java/org/apache/geode/cache/query/internal/index/NewDeclarativeIndexCreationJUnitTest.java
Line 165 (original), 162 (patched)
<https://reviews.apache.org/r/58712/#comment245954>
I purposely left this here. I'm going to followup with further changes to this test but I didn't want it to hold up this commit.
- Kirk Lund
On April 25, 2017, 4:46 p.m., Kirk Lund wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58712/
> -----------------------------------------------------------
>
> (Updated April 25, 2017, 4:46 p.m.)
>
>
> Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken Howe, and Patrick Rhomberg.
>
>
> Bugs: GEODE-2632
> https://issues.apache.org/jira/browse/GEODE-2632
>
>
> Repository: geode
>
>
> Description
> -------
>
> Fixup GemFireCacheImpl code. Also fix some affected collaborator classes and tests.
>
> * change SecurityService from static constant to final member field
> * fix typos and javadocs
> * reduce scope of constants, variables, methods
> * remove deadcode and useless comments/javadocs
> * fix misc IntelliJ warnings
> * add @Override annotations
> * improve some variable names
> * add TODOs for a couple problem areas that need further fixing
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/CancelCriterion.java e4f9a41599e521ce4bfbca9538d04dcc8edaa49b
> geode-core/src/main/java/org/apache/geode/cache/Cache.java bc4aa19eb99e4758512934c9b9c43bb18d4c20d8
> geode-core/src/main/java/org/apache/geode/cache/client/internal/ProxyCache.java 76306f51fc9479c7d9acaa28022ed908b674b7c0
> geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryMonitor.java d6acfbfc769af1cddd247dc0c3584cef00ea6f28
> geode-core/src/main/java/org/apache/geode/distributed/internal/DM.java 328a4f8265e54483c6bf34c2ad967f483661f155
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionManager.java 2ae86e65188b02ec6b8cbddc49ccbc73c55bfad1
> geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java 987e4911272ba6c37046d8e39a1187b71556736d
> geode-core/src/main/java/org/apache/geode/distributed/internal/LonerDistributionManager.java af4e674cf3c8c58ff23ae2e9ea620b94c81ce81b
> geode-core/src/main/java/org/apache/geode/internal/cache/DistTXState.java 6df26233417ea617e31d1928081c0719e26efd99
> geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java 56243e1b544f5958204e64c2ca391003aa1fd098
> geode-core/src/main/java/org/apache/geode/internal/cache/InternalCache.java 709308b57da847845ef9319bece18ebe9f25e569
> geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 45035d725241d60cbf6ed4f5417971c967925e86
> geode-core/src/main/java/org/apache/geode/internal/cache/execute/util/FindRestEnabledServersFunction.java 5da63adae8f3c0be4c2548034598d566efc517aa
> geode-core/src/main/java/org/apache/geode/internal/cache/persistence/PersistenceAdvisorImpl.java 7e30141c63a446852eea67aa4594241fca362668
> geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheCreation.java a5f0fc2bc7cf4250565aa8dd139004890b8da07d
> geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java d6a1efa73028e1b9514db67d2e3a4b564abee632
> geode-core/src/test/java/org/apache/geode/TXJUnitTest.java 54d9e503f2645d045487cea51011143602764f62
> geode-core/src/test/java/org/apache/geode/TXWriterTestCase.java 987f22f688ca695a8b37eacf239c69c329bb3b3b
> geode-core/src/test/java/org/apache/geode/cache/query/dunit/ResourceManagerWithQueryMonitorDUnitTest.java 903b2123097bac83045d887050f7eb955b27e3cd
> geode-core/src/test/java/org/apache/geode/cache/query/internal/index/NewDeclarativeIndexCreationJUnitTest.java 8a0f31cc87aa18b7b0928ab0e02405fd3ad75a7f
> geode-core/src/test/java/org/apache/geode/disttx/DistTXDebugDUnitTest.java 0d2f2b6f41e1b1dd829a41472206d0ccb6589a5e
> geode-core/src/test/java/org/apache/geode/disttx/DistTXJUnitTest.java 8abccc6c1ab447ffbd77379d7cf3d1c32905728e
> geode-core/src/test/java/org/apache/geode/disttx/DistTXPersistentDebugDUnitTest.java 5753f5c28de31353cdfb5235a981c1c9a88d75bd
> geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterJUnitTest.java 0a61b1f258d090090321c9ccff1a25781da7c8d1
> geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterOOMEJUnitTest.java b99d3fd25cdac5f1862927d098d9d6381894510e
> geode-core/src/test/java/org/apache/geode/disttx/DistributedTransactionDUnitTest.java 54715659a389c01b1b4b3fa79642d68f00b52b48
> geode-core/src/test/java/org/apache/geode/disttx/PRDistTXJUnitTest.java f27c0993a872b9879f5ee93b577939ee2b6919d9
> geode-core/src/test/java/org/apache/geode/internal/cache/PRTXJUnitTest.java d2bad641a47f68edb22da0f89a04c462ab48cd33
> geode-core/src/test/java/org/apache/geode/internal/cache/wan/parallel/ParallelQueueRemovalMessageJUnitTest.java d57ce125b9498f6439dbd0b13c72c67db6badb30
> geode-cq/src/main/java/org/apache/geode/cache/query/internal/cq/CqServiceImpl.java 570c06c5a4266b84c73e774bc3a021b39fa37b29
> geode-cq/src/test/java/org/apache/geode/cache/query/dunit/QueryMonitorDUnitTest.java f298fae6f1840302bc98668a09e4a9b2ed0c0b5c
> geode-web-api/src/main/java/org/apache/geode/rest/internal/web/controllers/CommonCrudController.java 0449a45c35569c672af06efdd9a1365283c435c3
>
>
> Diff: https://reviews.apache.org/r/58712/diff/1/
>
>
> Testing
> -------
>
> precheckin in progress
>
>
> Thanks,
>
> Kirk Lund
>
>
Re: Review Request 58712: GEODE-2632: partial cleanup of
GemFireCacheImpl
Posted by Kirk Lund <ki...@gmail.com>.
> On April 25, 2017, 9:05 p.m., Jared Stewart wrote:
> > geode-core/src/main/java/org/apache/geode/cache/Cache.java
> > Line 390 (original), 387 (patched)
> > <https://reviews.apache.org/r/58712/diff/2/?file=1699723#file1699723line391>
> >
> > This method appears to be unused.
That's actually a User API method. We could @deprecate it but we can't delete it. I think we should do some deprecation-specific sweeps through the code later (Darrel has filed a ton of Geode Jira tickets for deprecating and removal of deprecated APIs).
> On April 25, 2017, 9:05 p.m., Jared Stewart wrote:
> > geode-core/src/main/java/org/apache/geode/cache/Cache.java
> > Line 436 (original), 433 (patched)
> > <https://reviews.apache.org/r/58712/diff/2/?file=1699723#file1699723line437>
> >
> > This method appears to be unused.
Another User API method.
> On April 25, 2017, 9:05 p.m., Jared Stewart wrote:
> > geode-core/src/main/java/org/apache/geode/distributed/internal/LonerDistributionManager.java
> > Lines 1360 (patched)
> > <https://reviews.apache.org/r/58712/diff/2/?file=1699729#file1699729line1360>
> >
> > Perhaps this ought to be extracted to a named constant.
Yep, definitely! The DM type is currently a few final static int constants. That's more of a communications component. I think I'd prefer to defer changing that much more than it is already. I added "getDMType" to the DM interface (the name of which needs to change as well) which is why this method was added. It's currently there just to avoid type casting DM within GemFireCacheImpl.
> On April 25, 2017, 9:05 p.m., Jared Stewart wrote:
> > geode-core/src/main/java/org/apache/geode/internal/cache/DistTXState.java
> > Lines 54 (patched)
> > <https://reviews.apache.org/r/58712/diff/2/?file=1699730#file1699730line54>
> >
> > These fields surprised me initially. It might be nice to add a comment explaining that these are test hooks.
Yeah, I really don't want test hooks like this in product code. I'd like to do a future refactoring to rename all test hooks (vars, methdods etc) to end with something like "ForTesting" and make anything like this package-protected instead of public (even if that means moving tests around).
I've added a comment to each of these Runnables.
> On April 25, 2017, 9:05 p.m., Jared Stewart wrote:
> > geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
> > Line 675 (original), 647 (patched)
> > <https://reviews.apache.org/r/58712/diff/2/?file=1699731#file1699731line741>
> >
> > Perhaps we can simply append
> > ```
> > ExceptionUtils.getStackTrace(creationStack);
> > ```
> >
> > or
> >
> > ```
> > ExceptionUtils.getFullStackTrace(creationStack);
> > ```
> >
> > rather than defining an anonymous class that extends OutputStream.
I'm going to add a TODO for this one. I think "creationStack" is currently only captured in our dunit tests. If a dunit test encounters an existing Cache that's incompatible with what the test needs then it's part of the toString of an Exception that gets thrown.
> On April 25, 2017, 9:05 p.m., Jared Stewart wrote:
> > geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
> > Line 2887 (original), 2683 (patched)
> > <https://reviews.apache.org/r/58712/diff/2/?file=1699731#file1699731line2999>
> >
> > This looks like the first of several unchecked casts to `Set<DistributedMember>`. Perhaps we can have `DM.getAdminMemberSet()` return `Set<DistributedMember>` instead of a raw `Set`.
Yep, I started down that route a couple days ago, but ran into some problems. It spills over into the DistributionAdvisor classes and pretty soon it's a huge change set. I'd like to leave DM and DistributionManager for a future refactoring.
- Kirk
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58712/#review172967
-----------------------------------------------------------
On April 25, 2017, 5:05 p.m., Kirk Lund wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58712/
> -----------------------------------------------------------
>
> (Updated April 25, 2017, 5:05 p.m.)
>
>
> Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken Howe, and Patrick Rhomberg.
>
>
> Bugs: GEODE-2632
> https://issues.apache.org/jira/browse/GEODE-2632
>
>
> Repository: geode
>
>
> Description
> -------
>
> Fixup GemFireCacheImpl code. Also fix some affected collaborator classes and tests.
>
> * change SecurityService from static constant to final member field
> * fix typos and javadocs
> * narrow scope of constants, variables, methods
> * remove deadcode and useless comments/javadocs
> * fix misc IntelliJ warnings
> * add @Override annotations
> * improve some variable names
> * fix (some) generics and types
> * add TODOs for a couple problem areas that need further fixing
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/CancelCriterion.java e4f9a41599e521ce4bfbca9538d04dcc8edaa49b
> geode-core/src/main/java/org/apache/geode/cache/Cache.java bc4aa19eb99e4758512934c9b9c43bb18d4c20d8
> geode-core/src/main/java/org/apache/geode/cache/client/internal/ProxyCache.java 76306f51fc9479c7d9acaa28022ed908b674b7c0
> geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryMonitor.java d6acfbfc769af1cddd247dc0c3584cef00ea6f28
> geode-core/src/main/java/org/apache/geode/distributed/internal/DM.java 328a4f8265e54483c6bf34c2ad967f483661f155
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionManager.java 2ae86e65188b02ec6b8cbddc49ccbc73c55bfad1
> geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java 987e4911272ba6c37046d8e39a1187b71556736d
> geode-core/src/main/java/org/apache/geode/distributed/internal/LonerDistributionManager.java af4e674cf3c8c58ff23ae2e9ea620b94c81ce81b
> geode-core/src/main/java/org/apache/geode/internal/cache/DistTXState.java 6df26233417ea617e31d1928081c0719e26efd99
> geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java 56243e1b544f5958204e64c2ca391003aa1fd098
> geode-core/src/main/java/org/apache/geode/internal/cache/InternalCache.java 709308b57da847845ef9319bece18ebe9f25e569
> geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 45035d725241d60cbf6ed4f5417971c967925e86
> geode-core/src/main/java/org/apache/geode/internal/cache/execute/util/FindRestEnabledServersFunction.java 5da63adae8f3c0be4c2548034598d566efc517aa
> geode-core/src/main/java/org/apache/geode/internal/cache/persistence/PersistenceAdvisorImpl.java 7e30141c63a446852eea67aa4594241fca362668
> geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheCreation.java a5f0fc2bc7cf4250565aa8dd139004890b8da07d
> geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java d6a1efa73028e1b9514db67d2e3a4b564abee632
> geode-core/src/test/java/org/apache/geode/TXJUnitTest.java 54d9e503f2645d045487cea51011143602764f62
> geode-core/src/test/java/org/apache/geode/TXWriterTestCase.java 987f22f688ca695a8b37eacf239c69c329bb3b3b
> geode-core/src/test/java/org/apache/geode/cache/query/dunit/ResourceManagerWithQueryMonitorDUnitTest.java 903b2123097bac83045d887050f7eb955b27e3cd
> geode-core/src/test/java/org/apache/geode/cache/query/internal/index/NewDeclarativeIndexCreationJUnitTest.java 8a0f31cc87aa18b7b0928ab0e02405fd3ad75a7f
> geode-core/src/test/java/org/apache/geode/disttx/DistTXDebugDUnitTest.java 0d2f2b6f41e1b1dd829a41472206d0ccb6589a5e
> geode-core/src/test/java/org/apache/geode/disttx/DistTXJUnitTest.java 8abccc6c1ab447ffbd77379d7cf3d1c32905728e
> geode-core/src/test/java/org/apache/geode/disttx/DistTXPersistentDebugDUnitTest.java 5753f5c28de31353cdfb5235a981c1c9a88d75bd
> geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterJUnitTest.java 0a61b1f258d090090321c9ccff1a25781da7c8d1
> geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterOOMEJUnitTest.java b99d3fd25cdac5f1862927d098d9d6381894510e
> geode-core/src/test/java/org/apache/geode/disttx/DistributedTransactionDUnitTest.java 54715659a389c01b1b4b3fa79642d68f00b52b48
> geode-core/src/test/java/org/apache/geode/disttx/PRDistTXJUnitTest.java f27c0993a872b9879f5ee93b577939ee2b6919d9
> geode-core/src/test/java/org/apache/geode/internal/cache/PRTXJUnitTest.java d2bad641a47f68edb22da0f89a04c462ab48cd33
> geode-core/src/test/java/org/apache/geode/internal/cache/wan/parallel/ParallelQueueRemovalMessageJUnitTest.java d57ce125b9498f6439dbd0b13c72c67db6badb30
> geode-cq/src/main/java/org/apache/geode/cache/query/internal/cq/CqServiceImpl.java 570c06c5a4266b84c73e774bc3a021b39fa37b29
> geode-cq/src/test/java/org/apache/geode/cache/query/dunit/QueryMonitorDUnitTest.java f298fae6f1840302bc98668a09e4a9b2ed0c0b5c
> geode-web-api/src/main/java/org/apache/geode/rest/internal/web/controllers/CommonCrudController.java 0449a45c35569c672af06efdd9a1365283c435c3
>
>
> Diff: https://reviews.apache.org/r/58712/diff/2/
>
>
> Testing
> -------
>
> precheckin in progress
>
>
> Thanks,
>
> Kirk Lund
>
>
Re: Review Request 58712: GEODE-2632: partial cleanup of
GemFireCacheImpl
Posted by Jared Stewart <js...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58712/#review172967
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/cache/Cache.java
Line 390 (original), 387 (patched)
<https://reviews.apache.org/r/58712/#comment246000>
This method appears to be unused.
geode-core/src/main/java/org/apache/geode/cache/Cache.java
Line 436 (original), 433 (patched)
<https://reviews.apache.org/r/58712/#comment246001>
This method appears to be unused.
geode-core/src/main/java/org/apache/geode/distributed/internal/LonerDistributionManager.java
Lines 1360 (patched)
<https://reviews.apache.org/r/58712/#comment246017>
Perhaps this ought to be extracted to a named constant.
geode-core/src/main/java/org/apache/geode/internal/cache/DistTXState.java
Lines 54 (patched)
<https://reviews.apache.org/r/58712/#comment246006>
These fields surprised me initially. It might be nice to add a comment explaining that these are test hooks.
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
Line 675 (original), 647 (patched)
<https://reviews.apache.org/r/58712/#comment246011>
Perhaps we can simply append
```
ExceptionUtils.getStackTrace(creationStack);
```
or
```
ExceptionUtils.getFullStackTrace(creationStack);
```
rather than defining an anonymous class that extends OutputStream.
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
Line 2887 (original), 2683 (patched)
<https://reviews.apache.org/r/58712/#comment246013>
This looks like the first of several unchecked casts to `Set<DistributedMember>`. Perhaps we can have `DM.getAdminMemberSet()` return `Set<DistributedMember>` instead of a raw `Set`.
- Jared Stewart
On April 25, 2017, 5:05 p.m., Kirk Lund wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58712/
> -----------------------------------------------------------
>
> (Updated April 25, 2017, 5:05 p.m.)
>
>
> Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken Howe, and Patrick Rhomberg.
>
>
> Bugs: GEODE-2632
> https://issues.apache.org/jira/browse/GEODE-2632
>
>
> Repository: geode
>
>
> Description
> -------
>
> Fixup GemFireCacheImpl code. Also fix some affected collaborator classes and tests.
>
> * change SecurityService from static constant to final member field
> * fix typos and javadocs
> * narrow scope of constants, variables, methods
> * remove deadcode and useless comments/javadocs
> * fix misc IntelliJ warnings
> * add @Override annotations
> * improve some variable names
> * fix (some) generics and types
> * add TODOs for a couple problem areas that need further fixing
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/CancelCriterion.java e4f9a41599e521ce4bfbca9538d04dcc8edaa49b
> geode-core/src/main/java/org/apache/geode/cache/Cache.java bc4aa19eb99e4758512934c9b9c43bb18d4c20d8
> geode-core/src/main/java/org/apache/geode/cache/client/internal/ProxyCache.java 76306f51fc9479c7d9acaa28022ed908b674b7c0
> geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryMonitor.java d6acfbfc769af1cddd247dc0c3584cef00ea6f28
> geode-core/src/main/java/org/apache/geode/distributed/internal/DM.java 328a4f8265e54483c6bf34c2ad967f483661f155
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionManager.java 2ae86e65188b02ec6b8cbddc49ccbc73c55bfad1
> geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java 987e4911272ba6c37046d8e39a1187b71556736d
> geode-core/src/main/java/org/apache/geode/distributed/internal/LonerDistributionManager.java af4e674cf3c8c58ff23ae2e9ea620b94c81ce81b
> geode-core/src/main/java/org/apache/geode/internal/cache/DistTXState.java 6df26233417ea617e31d1928081c0719e26efd99
> geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java 56243e1b544f5958204e64c2ca391003aa1fd098
> geode-core/src/main/java/org/apache/geode/internal/cache/InternalCache.java 709308b57da847845ef9319bece18ebe9f25e569
> geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 45035d725241d60cbf6ed4f5417971c967925e86
> geode-core/src/main/java/org/apache/geode/internal/cache/execute/util/FindRestEnabledServersFunction.java 5da63adae8f3c0be4c2548034598d566efc517aa
> geode-core/src/main/java/org/apache/geode/internal/cache/persistence/PersistenceAdvisorImpl.java 7e30141c63a446852eea67aa4594241fca362668
> geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheCreation.java a5f0fc2bc7cf4250565aa8dd139004890b8da07d
> geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java d6a1efa73028e1b9514db67d2e3a4b564abee632
> geode-core/src/test/java/org/apache/geode/TXJUnitTest.java 54d9e503f2645d045487cea51011143602764f62
> geode-core/src/test/java/org/apache/geode/TXWriterTestCase.java 987f22f688ca695a8b37eacf239c69c329bb3b3b
> geode-core/src/test/java/org/apache/geode/cache/query/dunit/ResourceManagerWithQueryMonitorDUnitTest.java 903b2123097bac83045d887050f7eb955b27e3cd
> geode-core/src/test/java/org/apache/geode/cache/query/internal/index/NewDeclarativeIndexCreationJUnitTest.java 8a0f31cc87aa18b7b0928ab0e02405fd3ad75a7f
> geode-core/src/test/java/org/apache/geode/disttx/DistTXDebugDUnitTest.java 0d2f2b6f41e1b1dd829a41472206d0ccb6589a5e
> geode-core/src/test/java/org/apache/geode/disttx/DistTXJUnitTest.java 8abccc6c1ab447ffbd77379d7cf3d1c32905728e
> geode-core/src/test/java/org/apache/geode/disttx/DistTXPersistentDebugDUnitTest.java 5753f5c28de31353cdfb5235a981c1c9a88d75bd
> geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterJUnitTest.java 0a61b1f258d090090321c9ccff1a25781da7c8d1
> geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterOOMEJUnitTest.java b99d3fd25cdac5f1862927d098d9d6381894510e
> geode-core/src/test/java/org/apache/geode/disttx/DistributedTransactionDUnitTest.java 54715659a389c01b1b4b3fa79642d68f00b52b48
> geode-core/src/test/java/org/apache/geode/disttx/PRDistTXJUnitTest.java f27c0993a872b9879f5ee93b577939ee2b6919d9
> geode-core/src/test/java/org/apache/geode/internal/cache/PRTXJUnitTest.java d2bad641a47f68edb22da0f89a04c462ab48cd33
> geode-core/src/test/java/org/apache/geode/internal/cache/wan/parallel/ParallelQueueRemovalMessageJUnitTest.java d57ce125b9498f6439dbd0b13c72c67db6badb30
> geode-cq/src/main/java/org/apache/geode/cache/query/internal/cq/CqServiceImpl.java 570c06c5a4266b84c73e774bc3a021b39fa37b29
> geode-cq/src/test/java/org/apache/geode/cache/query/dunit/QueryMonitorDUnitTest.java f298fae6f1840302bc98668a09e4a9b2ed0c0b5c
> geode-web-api/src/main/java/org/apache/geode/rest/internal/web/controllers/CommonCrudController.java 0449a45c35569c672af06efdd9a1365283c435c3
>
>
> Diff: https://reviews.apache.org/r/58712/diff/2/
>
>
> Testing
> -------
>
> precheckin in progress
>
>
> Thanks,
>
> Kirk Lund
>
>
Re: Review Request 58712: GEODE-2632: partial cleanup of
GemFireCacheImpl
Posted by Jared Stewart <js...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58712/#review173004
-----------------------------------------------------------
Ship it!
Ship It!
- Jared Stewart
On April 25, 2017, 11:29 p.m., Kirk Lund wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58712/
> -----------------------------------------------------------
>
> (Updated April 25, 2017, 11:29 p.m.)
>
>
> Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken Howe, and Patrick Rhomberg.
>
>
> Bugs: GEODE-2632
> https://issues.apache.org/jira/browse/GEODE-2632
>
>
> Repository: geode
>
>
> Description
> -------
>
> Fixup GemFireCacheImpl code. Also fix some affected collaborator classes and tests.
>
> * change SecurityService from static constant to final member field
> * fix typos and javadocs
> * narrow scope of constants, variables, methods
> * remove deadcode and useless comments/javadocs
> * fix misc IntelliJ warnings
> * add @Override annotations
> * improve some variable names
> * fix (some) generics and types
> * add TODOs for a couple problem areas that need further fixing
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/CancelCriterion.java e4f9a41599e521ce4bfbca9538d04dcc8edaa49b
> geode-core/src/main/java/org/apache/geode/cache/Cache.java bc4aa19eb99e4758512934c9b9c43bb18d4c20d8
> geode-core/src/main/java/org/apache/geode/cache/client/internal/ProxyCache.java 76306f51fc9479c7d9acaa28022ed908b674b7c0
> geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryMonitor.java d6acfbfc769af1cddd247dc0c3584cef00ea6f28
> geode-core/src/main/java/org/apache/geode/distributed/internal/DM.java 328a4f8265e54483c6bf34c2ad967f483661f155
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionManager.java 2ae86e65188b02ec6b8cbddc49ccbc73c55bfad1
> geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java 987e4911272ba6c37046d8e39a1187b71556736d
> geode-core/src/main/java/org/apache/geode/distributed/internal/LonerDistributionManager.java af4e674cf3c8c58ff23ae2e9ea620b94c81ce81b
> geode-core/src/main/java/org/apache/geode/internal/cache/DistTXState.java 6df26233417ea617e31d1928081c0719e26efd99
> geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java 56243e1b544f5958204e64c2ca391003aa1fd098
> geode-core/src/main/java/org/apache/geode/internal/cache/InternalCache.java 709308b57da847845ef9319bece18ebe9f25e569
> geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 45035d725241d60cbf6ed4f5417971c967925e86
> geode-core/src/main/java/org/apache/geode/internal/cache/execute/util/FindRestEnabledServersFunction.java 5da63adae8f3c0be4c2548034598d566efc517aa
> geode-core/src/main/java/org/apache/geode/internal/cache/persistence/PersistenceAdvisorImpl.java 7e30141c63a446852eea67aa4594241fca362668
> geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheCreation.java a5f0fc2bc7cf4250565aa8dd139004890b8da07d
> geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java d6a1efa73028e1b9514db67d2e3a4b564abee632
> geode-core/src/test/java/org/apache/geode/TXJUnitTest.java 54d9e503f2645d045487cea51011143602764f62
> geode-core/src/test/java/org/apache/geode/TXWriterTestCase.java 987f22f688ca695a8b37eacf239c69c329bb3b3b
> geode-core/src/test/java/org/apache/geode/cache/query/dunit/ResourceManagerWithQueryMonitorDUnitTest.java 903b2123097bac83045d887050f7eb955b27e3cd
> geode-core/src/test/java/org/apache/geode/cache/query/internal/index/NewDeclarativeIndexCreationJUnitTest.java 8a0f31cc87aa18b7b0928ab0e02405fd3ad75a7f
> geode-core/src/test/java/org/apache/geode/disttx/DistTXDebugDUnitTest.java 0d2f2b6f41e1b1dd829a41472206d0ccb6589a5e
> geode-core/src/test/java/org/apache/geode/disttx/DistTXJUnitTest.java 8abccc6c1ab447ffbd77379d7cf3d1c32905728e
> geode-core/src/test/java/org/apache/geode/disttx/DistTXPersistentDebugDUnitTest.java 5753f5c28de31353cdfb5235a981c1c9a88d75bd
> geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterJUnitTest.java 0a61b1f258d090090321c9ccff1a25781da7c8d1
> geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterOOMEJUnitTest.java b99d3fd25cdac5f1862927d098d9d6381894510e
> geode-core/src/test/java/org/apache/geode/disttx/DistributedTransactionDUnitTest.java 54715659a389c01b1b4b3fa79642d68f00b52b48
> geode-core/src/test/java/org/apache/geode/disttx/PRDistTXJUnitTest.java f27c0993a872b9879f5ee93b577939ee2b6919d9
> geode-core/src/test/java/org/apache/geode/internal/cache/PRTXJUnitTest.java d2bad641a47f68edb22da0f89a04c462ab48cd33
> geode-core/src/test/java/org/apache/geode/internal/cache/wan/parallel/ParallelQueueRemovalMessageJUnitTest.java d57ce125b9498f6439dbd0b13c72c67db6badb30
> geode-cq/src/main/java/org/apache/geode/cache/query/internal/cq/CqServiceImpl.java 570c06c5a4266b84c73e774bc3a021b39fa37b29
> geode-cq/src/test/java/org/apache/geode/cache/query/dunit/QueryMonitorDUnitTest.java f298fae6f1840302bc98668a09e4a9b2ed0c0b5c
> geode-web-api/src/main/java/org/apache/geode/rest/internal/web/controllers/CommonCrudController.java 0449a45c35569c672af06efdd9a1365283c435c3
>
>
> Diff: https://reviews.apache.org/r/58712/diff/3/
>
>
> Testing
> -------
>
> precheckin passed
>
>
> Thanks,
>
> Kirk Lund
>
>
Re: Review Request 58712: GEODE-2632: partial cleanup of
GemFireCacheImpl
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58712/
-----------------------------------------------------------
(Updated April 26, 2017, 4:39 p.m.)
Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken Howe, and Patrick Rhomberg.
Changes
-------
I have a green precheckin again. I'm going to take the first commit and push that, then push the second commit with changes for review issues after it passes precheckin too.
Bugs: GEODE-2632
https://issues.apache.org/jira/browse/GEODE-2632
Repository: geode
Description
-------
Fixup GemFireCacheImpl code. Also fix some affected collaborator classes and tests.
* change SecurityService from static constant to final member field
* fix typos and javadocs
* narrow scope of constants, variables, methods
* remove deadcode and useless comments/javadocs
* fix misc IntelliJ warnings
* add @Override annotations
* improve some variable names
* fix (some) generics and types
* add TODOs for a couple problem areas that need further fixing
Diffs
-----
geode-core/src/main/java/org/apache/geode/CancelCriterion.java e4f9a41599e521ce4bfbca9538d04dcc8edaa49b
geode-core/src/main/java/org/apache/geode/cache/Cache.java bc4aa19eb99e4758512934c9b9c43bb18d4c20d8
geode-core/src/main/java/org/apache/geode/cache/client/internal/ProxyCache.java 76306f51fc9479c7d9acaa28022ed908b674b7c0
geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryMonitor.java d6acfbfc769af1cddd247dc0c3584cef00ea6f28
geode-core/src/main/java/org/apache/geode/distributed/internal/DM.java 328a4f8265e54483c6bf34c2ad967f483661f155
geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionManager.java 2ae86e65188b02ec6b8cbddc49ccbc73c55bfad1
geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java 987e4911272ba6c37046d8e39a1187b71556736d
geode-core/src/main/java/org/apache/geode/distributed/internal/LonerDistributionManager.java af4e674cf3c8c58ff23ae2e9ea620b94c81ce81b
geode-core/src/main/java/org/apache/geode/internal/cache/DistTXState.java 6df26233417ea617e31d1928081c0719e26efd99
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java 56243e1b544f5958204e64c2ca391003aa1fd098
geode-core/src/main/java/org/apache/geode/internal/cache/InternalCache.java 709308b57da847845ef9319bece18ebe9f25e569
geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 45035d725241d60cbf6ed4f5417971c967925e86
geode-core/src/main/java/org/apache/geode/internal/cache/execute/util/FindRestEnabledServersFunction.java 5da63adae8f3c0be4c2548034598d566efc517aa
geode-core/src/main/java/org/apache/geode/internal/cache/persistence/PersistenceAdvisorImpl.java 7e30141c63a446852eea67aa4594241fca362668
geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheCreation.java a5f0fc2bc7cf4250565aa8dd139004890b8da07d
geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java d6a1efa73028e1b9514db67d2e3a4b564abee632
geode-core/src/test/java/org/apache/geode/TXJUnitTest.java 54d9e503f2645d045487cea51011143602764f62
geode-core/src/test/java/org/apache/geode/TXWriterTestCase.java 987f22f688ca695a8b37eacf239c69c329bb3b3b
geode-core/src/test/java/org/apache/geode/cache/query/dunit/ResourceManagerWithQueryMonitorDUnitTest.java 903b2123097bac83045d887050f7eb955b27e3cd
geode-core/src/test/java/org/apache/geode/cache/query/internal/index/NewDeclarativeIndexCreationJUnitTest.java 8a0f31cc87aa18b7b0928ab0e02405fd3ad75a7f
geode-core/src/test/java/org/apache/geode/disttx/DistTXDebugDUnitTest.java 0d2f2b6f41e1b1dd829a41472206d0ccb6589a5e
geode-core/src/test/java/org/apache/geode/disttx/DistTXJUnitTest.java 8abccc6c1ab447ffbd77379d7cf3d1c32905728e
geode-core/src/test/java/org/apache/geode/disttx/DistTXPersistentDebugDUnitTest.java 5753f5c28de31353cdfb5235a981c1c9a88d75bd
geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterJUnitTest.java 0a61b1f258d090090321c9ccff1a25781da7c8d1
geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterOOMEJUnitTest.java b99d3fd25cdac5f1862927d098d9d6381894510e
geode-core/src/test/java/org/apache/geode/disttx/DistributedTransactionDUnitTest.java 54715659a389c01b1b4b3fa79642d68f00b52b48
geode-core/src/test/java/org/apache/geode/disttx/PRDistTXJUnitTest.java f27c0993a872b9879f5ee93b577939ee2b6919d9
geode-core/src/test/java/org/apache/geode/internal/cache/PRTXJUnitTest.java d2bad641a47f68edb22da0f89a04c462ab48cd33
geode-core/src/test/java/org/apache/geode/internal/cache/wan/parallel/ParallelQueueRemovalMessageJUnitTest.java d57ce125b9498f6439dbd0b13c72c67db6badb30
geode-cq/src/main/java/org/apache/geode/cache/query/internal/cq/CqServiceImpl.java 570c06c5a4266b84c73e774bc3a021b39fa37b29
geode-cq/src/test/java/org/apache/geode/cache/query/dunit/QueryMonitorDUnitTest.java f298fae6f1840302bc98668a09e4a9b2ed0c0b5c
geode-web-api/src/main/java/org/apache/geode/rest/internal/web/controllers/CommonCrudController.java 0449a45c35569c672af06efdd9a1365283c435c3
Diff: https://reviews.apache.org/r/58712/diff/3/
Testing (updated)
-------
precheckin passed
Thanks,
Kirk Lund
Re: Review Request 58712: GEODE-2632: partial cleanup of
GemFireCacheImpl
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58712/
-----------------------------------------------------------
(Updated April 26, 2017, 12:26 a.m.)
Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken Howe, and Patrick Rhomberg.
Bugs: GEODE-2632
https://issues.apache.org/jira/browse/GEODE-2632
Repository: geode
Description
-------
Fixup GemFireCacheImpl code. Also fix some affected collaborator classes and tests.
* change SecurityService from static constant to final member field
* fix typos and javadocs
* narrow scope of constants, variables, methods
* remove deadcode and useless comments/javadocs
* fix misc IntelliJ warnings
* add @Override annotations
* improve some variable names
* fix (some) generics and types
* add TODOs for a couple problem areas that need further fixing
Diffs
-----
geode-core/src/main/java/org/apache/geode/CancelCriterion.java e4f9a41599e521ce4bfbca9538d04dcc8edaa49b
geode-core/src/main/java/org/apache/geode/cache/Cache.java bc4aa19eb99e4758512934c9b9c43bb18d4c20d8
geode-core/src/main/java/org/apache/geode/cache/client/internal/ProxyCache.java 76306f51fc9479c7d9acaa28022ed908b674b7c0
geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryMonitor.java d6acfbfc769af1cddd247dc0c3584cef00ea6f28
geode-core/src/main/java/org/apache/geode/distributed/internal/DM.java 328a4f8265e54483c6bf34c2ad967f483661f155
geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionManager.java 2ae86e65188b02ec6b8cbddc49ccbc73c55bfad1
geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java 987e4911272ba6c37046d8e39a1187b71556736d
geode-core/src/main/java/org/apache/geode/distributed/internal/LonerDistributionManager.java af4e674cf3c8c58ff23ae2e9ea620b94c81ce81b
geode-core/src/main/java/org/apache/geode/internal/cache/DistTXState.java 6df26233417ea617e31d1928081c0719e26efd99
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java 56243e1b544f5958204e64c2ca391003aa1fd098
geode-core/src/main/java/org/apache/geode/internal/cache/InternalCache.java 709308b57da847845ef9319bece18ebe9f25e569
geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 45035d725241d60cbf6ed4f5417971c967925e86
geode-core/src/main/java/org/apache/geode/internal/cache/execute/util/FindRestEnabledServersFunction.java 5da63adae8f3c0be4c2548034598d566efc517aa
geode-core/src/main/java/org/apache/geode/internal/cache/persistence/PersistenceAdvisorImpl.java 7e30141c63a446852eea67aa4594241fca362668
geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheCreation.java a5f0fc2bc7cf4250565aa8dd139004890b8da07d
geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java d6a1efa73028e1b9514db67d2e3a4b564abee632
geode-core/src/test/java/org/apache/geode/TXJUnitTest.java 54d9e503f2645d045487cea51011143602764f62
geode-core/src/test/java/org/apache/geode/TXWriterTestCase.java 987f22f688ca695a8b37eacf239c69c329bb3b3b
geode-core/src/test/java/org/apache/geode/cache/query/dunit/ResourceManagerWithQueryMonitorDUnitTest.java 903b2123097bac83045d887050f7eb955b27e3cd
geode-core/src/test/java/org/apache/geode/cache/query/internal/index/NewDeclarativeIndexCreationJUnitTest.java 8a0f31cc87aa18b7b0928ab0e02405fd3ad75a7f
geode-core/src/test/java/org/apache/geode/disttx/DistTXDebugDUnitTest.java 0d2f2b6f41e1b1dd829a41472206d0ccb6589a5e
geode-core/src/test/java/org/apache/geode/disttx/DistTXJUnitTest.java 8abccc6c1ab447ffbd77379d7cf3d1c32905728e
geode-core/src/test/java/org/apache/geode/disttx/DistTXPersistentDebugDUnitTest.java 5753f5c28de31353cdfb5235a981c1c9a88d75bd
geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterJUnitTest.java 0a61b1f258d090090321c9ccff1a25781da7c8d1
geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterOOMEJUnitTest.java b99d3fd25cdac5f1862927d098d9d6381894510e
geode-core/src/test/java/org/apache/geode/disttx/DistributedTransactionDUnitTest.java 54715659a389c01b1b4b3fa79642d68f00b52b48
geode-core/src/test/java/org/apache/geode/disttx/PRDistTXJUnitTest.java f27c0993a872b9879f5ee93b577939ee2b6919d9
geode-core/src/test/java/org/apache/geode/internal/cache/PRTXJUnitTest.java d2bad641a47f68edb22da0f89a04c462ab48cd33
geode-core/src/test/java/org/apache/geode/internal/cache/wan/parallel/ParallelQueueRemovalMessageJUnitTest.java d57ce125b9498f6439dbd0b13c72c67db6badb30
geode-cq/src/main/java/org/apache/geode/cache/query/internal/cq/CqServiceImpl.java 570c06c5a4266b84c73e774bc3a021b39fa37b29
geode-cq/src/test/java/org/apache/geode/cache/query/dunit/QueryMonitorDUnitTest.java f298fae6f1840302bc98668a09e4a9b2ed0c0b5c
geode-web-api/src/main/java/org/apache/geode/rest/internal/web/controllers/CommonCrudController.java 0449a45c35569c672af06efdd9a1365283c435c3
Diff: https://reviews.apache.org/r/58712/diff/3/
Testing (updated)
-------
precheckin passed
Looks like the last few changes that went in for fixing review issues has caused ParallelQueueRemovalMessageJUnitTest to fail again. I'll have to back all of those changes out, update the diff again and resubmit precheckin.
Thanks,
Kirk Lund
Re: Review Request 58712: GEODE-2632: partial cleanup of
GemFireCacheImpl
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58712/
-----------------------------------------------------------
(Updated April 25, 2017, 11:29 p.m.)
Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken Howe, and Patrick Rhomberg.
Bugs: GEODE-2632
https://issues.apache.org/jira/browse/GEODE-2632
Repository: geode
Description
-------
Fixup GemFireCacheImpl code. Also fix some affected collaborator classes and tests.
* change SecurityService from static constant to final member field
* fix typos and javadocs
* narrow scope of constants, variables, methods
* remove deadcode and useless comments/javadocs
* fix misc IntelliJ warnings
* add @Override annotations
* improve some variable names
* fix (some) generics and types
* add TODOs for a couple problem areas that need further fixing
Diffs
-----
geode-core/src/main/java/org/apache/geode/CancelCriterion.java e4f9a41599e521ce4bfbca9538d04dcc8edaa49b
geode-core/src/main/java/org/apache/geode/cache/Cache.java bc4aa19eb99e4758512934c9b9c43bb18d4c20d8
geode-core/src/main/java/org/apache/geode/cache/client/internal/ProxyCache.java 76306f51fc9479c7d9acaa28022ed908b674b7c0
geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryMonitor.java d6acfbfc769af1cddd247dc0c3584cef00ea6f28
geode-core/src/main/java/org/apache/geode/distributed/internal/DM.java 328a4f8265e54483c6bf34c2ad967f483661f155
geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionManager.java 2ae86e65188b02ec6b8cbddc49ccbc73c55bfad1
geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java 987e4911272ba6c37046d8e39a1187b71556736d
geode-core/src/main/java/org/apache/geode/distributed/internal/LonerDistributionManager.java af4e674cf3c8c58ff23ae2e9ea620b94c81ce81b
geode-core/src/main/java/org/apache/geode/internal/cache/DistTXState.java 6df26233417ea617e31d1928081c0719e26efd99
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java 56243e1b544f5958204e64c2ca391003aa1fd098
geode-core/src/main/java/org/apache/geode/internal/cache/InternalCache.java 709308b57da847845ef9319bece18ebe9f25e569
geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 45035d725241d60cbf6ed4f5417971c967925e86
geode-core/src/main/java/org/apache/geode/internal/cache/execute/util/FindRestEnabledServersFunction.java 5da63adae8f3c0be4c2548034598d566efc517aa
geode-core/src/main/java/org/apache/geode/internal/cache/persistence/PersistenceAdvisorImpl.java 7e30141c63a446852eea67aa4594241fca362668
geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheCreation.java a5f0fc2bc7cf4250565aa8dd139004890b8da07d
geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java d6a1efa73028e1b9514db67d2e3a4b564abee632
geode-core/src/test/java/org/apache/geode/TXJUnitTest.java 54d9e503f2645d045487cea51011143602764f62
geode-core/src/test/java/org/apache/geode/TXWriterTestCase.java 987f22f688ca695a8b37eacf239c69c329bb3b3b
geode-core/src/test/java/org/apache/geode/cache/query/dunit/ResourceManagerWithQueryMonitorDUnitTest.java 903b2123097bac83045d887050f7eb955b27e3cd
geode-core/src/test/java/org/apache/geode/cache/query/internal/index/NewDeclarativeIndexCreationJUnitTest.java 8a0f31cc87aa18b7b0928ab0e02405fd3ad75a7f
geode-core/src/test/java/org/apache/geode/disttx/DistTXDebugDUnitTest.java 0d2f2b6f41e1b1dd829a41472206d0ccb6589a5e
geode-core/src/test/java/org/apache/geode/disttx/DistTXJUnitTest.java 8abccc6c1ab447ffbd77379d7cf3d1c32905728e
geode-core/src/test/java/org/apache/geode/disttx/DistTXPersistentDebugDUnitTest.java 5753f5c28de31353cdfb5235a981c1c9a88d75bd
geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterJUnitTest.java 0a61b1f258d090090321c9ccff1a25781da7c8d1
geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterOOMEJUnitTest.java b99d3fd25cdac5f1862927d098d9d6381894510e
geode-core/src/test/java/org/apache/geode/disttx/DistributedTransactionDUnitTest.java 54715659a389c01b1b4b3fa79642d68f00b52b48
geode-core/src/test/java/org/apache/geode/disttx/PRDistTXJUnitTest.java f27c0993a872b9879f5ee93b577939ee2b6919d9
geode-core/src/test/java/org/apache/geode/internal/cache/PRTXJUnitTest.java d2bad641a47f68edb22da0f89a04c462ab48cd33
geode-core/src/test/java/org/apache/geode/internal/cache/wan/parallel/ParallelQueueRemovalMessageJUnitTest.java d57ce125b9498f6439dbd0b13c72c67db6badb30
geode-cq/src/main/java/org/apache/geode/cache/query/internal/cq/CqServiceImpl.java 570c06c5a4266b84c73e774bc3a021b39fa37b29
geode-cq/src/test/java/org/apache/geode/cache/query/dunit/QueryMonitorDUnitTest.java f298fae6f1840302bc98668a09e4a9b2ed0c0b5c
geode-web-api/src/main/java/org/apache/geode/rest/internal/web/controllers/CommonCrudController.java 0449a45c35569c672af06efdd9a1365283c435c3
Diff: https://reviews.apache.org/r/58712/diff/3/
Testing (updated)
-------
precheckin passed
Thanks,
Kirk Lund
Re: Review Request 58712: GEODE-2632: partial cleanup of
GemFireCacheImpl
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58712/
-----------------------------------------------------------
(Updated April 25, 2017, 11:27 p.m.)
Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken Howe, and Patrick Rhomberg.
Changes
-------
Add in my 2nd batch of GemFireCacheImpl refactoring. Also address several review issues.
Bugs: GEODE-2632
https://issues.apache.org/jira/browse/GEODE-2632
Repository: geode
Description
-------
Fixup GemFireCacheImpl code. Also fix some affected collaborator classes and tests.
* change SecurityService from static constant to final member field
* fix typos and javadocs
* narrow scope of constants, variables, methods
* remove deadcode and useless comments/javadocs
* fix misc IntelliJ warnings
* add @Override annotations
* improve some variable names
* fix (some) generics and types
* add TODOs for a couple problem areas that need further fixing
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/CancelCriterion.java e4f9a41599e521ce4bfbca9538d04dcc8edaa49b
geode-core/src/main/java/org/apache/geode/cache/Cache.java bc4aa19eb99e4758512934c9b9c43bb18d4c20d8
geode-core/src/main/java/org/apache/geode/cache/client/internal/ProxyCache.java 76306f51fc9479c7d9acaa28022ed908b674b7c0
geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryMonitor.java d6acfbfc769af1cddd247dc0c3584cef00ea6f28
geode-core/src/main/java/org/apache/geode/distributed/internal/DM.java 328a4f8265e54483c6bf34c2ad967f483661f155
geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionManager.java 2ae86e65188b02ec6b8cbddc49ccbc73c55bfad1
geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java 987e4911272ba6c37046d8e39a1187b71556736d
geode-core/src/main/java/org/apache/geode/distributed/internal/LonerDistributionManager.java af4e674cf3c8c58ff23ae2e9ea620b94c81ce81b
geode-core/src/main/java/org/apache/geode/internal/cache/DistTXState.java 6df26233417ea617e31d1928081c0719e26efd99
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java 56243e1b544f5958204e64c2ca391003aa1fd098
geode-core/src/main/java/org/apache/geode/internal/cache/InternalCache.java 709308b57da847845ef9319bece18ebe9f25e569
geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 45035d725241d60cbf6ed4f5417971c967925e86
geode-core/src/main/java/org/apache/geode/internal/cache/execute/util/FindRestEnabledServersFunction.java 5da63adae8f3c0be4c2548034598d566efc517aa
geode-core/src/main/java/org/apache/geode/internal/cache/persistence/PersistenceAdvisorImpl.java 7e30141c63a446852eea67aa4594241fca362668
geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheCreation.java a5f0fc2bc7cf4250565aa8dd139004890b8da07d
geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java d6a1efa73028e1b9514db67d2e3a4b564abee632
geode-core/src/test/java/org/apache/geode/TXJUnitTest.java 54d9e503f2645d045487cea51011143602764f62
geode-core/src/test/java/org/apache/geode/TXWriterTestCase.java 987f22f688ca695a8b37eacf239c69c329bb3b3b
geode-core/src/test/java/org/apache/geode/cache/query/dunit/ResourceManagerWithQueryMonitorDUnitTest.java 903b2123097bac83045d887050f7eb955b27e3cd
geode-core/src/test/java/org/apache/geode/cache/query/internal/index/NewDeclarativeIndexCreationJUnitTest.java 8a0f31cc87aa18b7b0928ab0e02405fd3ad75a7f
geode-core/src/test/java/org/apache/geode/disttx/DistTXDebugDUnitTest.java 0d2f2b6f41e1b1dd829a41472206d0ccb6589a5e
geode-core/src/test/java/org/apache/geode/disttx/DistTXJUnitTest.java 8abccc6c1ab447ffbd77379d7cf3d1c32905728e
geode-core/src/test/java/org/apache/geode/disttx/DistTXPersistentDebugDUnitTest.java 5753f5c28de31353cdfb5235a981c1c9a88d75bd
geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterJUnitTest.java 0a61b1f258d090090321c9ccff1a25781da7c8d1
geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterOOMEJUnitTest.java b99d3fd25cdac5f1862927d098d9d6381894510e
geode-core/src/test/java/org/apache/geode/disttx/DistributedTransactionDUnitTest.java 54715659a389c01b1b4b3fa79642d68f00b52b48
geode-core/src/test/java/org/apache/geode/disttx/PRDistTXJUnitTest.java f27c0993a872b9879f5ee93b577939ee2b6919d9
geode-core/src/test/java/org/apache/geode/internal/cache/PRTXJUnitTest.java d2bad641a47f68edb22da0f89a04c462ab48cd33
geode-core/src/test/java/org/apache/geode/internal/cache/wan/parallel/ParallelQueueRemovalMessageJUnitTest.java d57ce125b9498f6439dbd0b13c72c67db6badb30
geode-cq/src/main/java/org/apache/geode/cache/query/internal/cq/CqServiceImpl.java 570c06c5a4266b84c73e774bc3a021b39fa37b29
geode-cq/src/test/java/org/apache/geode/cache/query/dunit/QueryMonitorDUnitTest.java f298fae6f1840302bc98668a09e4a9b2ed0c0b5c
geode-web-api/src/main/java/org/apache/geode/rest/internal/web/controllers/CommonCrudController.java 0449a45c35569c672af06efdd9a1365283c435c3
Diff: https://reviews.apache.org/r/58712/diff/3/
Changes: https://reviews.apache.org/r/58712/diff/2-3/
Testing
-------
precheckin in progress
Thanks,
Kirk Lund
Re: Review Request 58712: GEODE-2632: partial cleanup of
GemFireCacheImpl
Posted by Kirk Lund <ki...@gmail.com>.
> On April 25, 2017, 8:42 p.m., Patrick Rhomberg wrote:
> > geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
> > Lines 2804-2811 (original), 2600-2607 (patched)
> > <https://reviews.apache.org/r/58712/diff/2/?file=1699731#file1699731line2914>
> >
> > Here and throughout... is there a reason to prefer caching `isDebugEnabled` at the beginning of the method? I can't imagine `logger.isDebugEnabled()` actually effects performance in a significant way here, and that seems to be the standard usage throughout the rest of this file / the codebase.
A method like stopServers() definitely does not need the value cached like that. This actually is the pattern elsewhere in the code where we have multiple debug statements in one method, so if I change this method, it'll be different from other classes. If a method is in a performance sensitive code path such as that for doing a put, then it does make a difference hitting the volatile once instead of 7 times. I'm kind of conflicted about making a further change here at least right now -- I'd rather admit that this method doesn't really need this isDebugEnabled variable but I don't really want to change it either. I don't feel too strongly about it though -- I'd like to revisit this after Jared (or anyone) digs into FastLogger and does some experimenting with JMH benchmarking.
> On April 25, 2017, 8:42 p.m., Patrick Rhomberg wrote:
> > geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
> > Lines 3290-3292 (original), 3064-3066 (patched)
> > <https://reviews.apache.org/r/58712/diff/2/?file=1699731#file1699731line3402>
> >
> > Should any exception, even an explicitly caught one cancel, be caught without at least logging? Empty catches seem like bad form to me.
This should be an exception to the case (hah! sorry). In general an ignored Exception makes me feel dirty.
In some cases it's probably ok to catch an Exception without logging but usually the code returns a different value or does something in reaction. Some of the code in these classes catches something somewhere and then does something elsewhere so it's disjointed (and needs further cleanup). Changing these catch-blocks can cause some really weird test failures. In particular logging an exception where it wasn't logging before (at INFO or above) is going to increase the likelihood of a hydra or dunit test failing grepForErrors (which will fail if an unexpected exception was logged).
In this case, it's probably ok to let the code ignore that exception but rename "e" to "ignore" and then revisit it when we iterate over GemFireCacheImpl with more refactorings.
All of the test failures I've had to deal with so far during this refactoring involves changes that I made to catch-blocks so it's risky.
> On April 25, 2017, 8:42 p.m., Patrick Rhomberg wrote:
> > geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
> > Lines 4436-4449 (original), 4142-4155 (patched)
> > <https://reviews.apache.org/r/58712/diff/2/?file=1699731#file1699731line4565>
> >
> > To return to the question of single vs multiple returns... Would this read better?
> > ```
> > @Override
> > public QueryService getQueryService() {
> > if (!isClient()) {
> > return new DefaultQueryService(this);
> > }
> > Pool defaultPool = getDefaultPool();
> > if (defaultPool == null) {
> > throw new IllegalStateException(
> > "Client cache does not have a default pool. Use getQueryService(String poolName) instead.");
> > }
> > return defaultPool.getQueryService();
> > }
> > ```
The nested if-else is horrific, so yes, that's a good change!
- Kirk
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58712/#review172966
-----------------------------------------------------------
On April 25, 2017, 5:05 p.m., Kirk Lund wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58712/
> -----------------------------------------------------------
>
> (Updated April 25, 2017, 5:05 p.m.)
>
>
> Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken Howe, and Patrick Rhomberg.
>
>
> Bugs: GEODE-2632
> https://issues.apache.org/jira/browse/GEODE-2632
>
>
> Repository: geode
>
>
> Description
> -------
>
> Fixup GemFireCacheImpl code. Also fix some affected collaborator classes and tests.
>
> * change SecurityService from static constant to final member field
> * fix typos and javadocs
> * narrow scope of constants, variables, methods
> * remove deadcode and useless comments/javadocs
> * fix misc IntelliJ warnings
> * add @Override annotations
> * improve some variable names
> * fix (some) generics and types
> * add TODOs for a couple problem areas that need further fixing
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/CancelCriterion.java e4f9a41599e521ce4bfbca9538d04dcc8edaa49b
> geode-core/src/main/java/org/apache/geode/cache/Cache.java bc4aa19eb99e4758512934c9b9c43bb18d4c20d8
> geode-core/src/main/java/org/apache/geode/cache/client/internal/ProxyCache.java 76306f51fc9479c7d9acaa28022ed908b674b7c0
> geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryMonitor.java d6acfbfc769af1cddd247dc0c3584cef00ea6f28
> geode-core/src/main/java/org/apache/geode/distributed/internal/DM.java 328a4f8265e54483c6bf34c2ad967f483661f155
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionManager.java 2ae86e65188b02ec6b8cbddc49ccbc73c55bfad1
> geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java 987e4911272ba6c37046d8e39a1187b71556736d
> geode-core/src/main/java/org/apache/geode/distributed/internal/LonerDistributionManager.java af4e674cf3c8c58ff23ae2e9ea620b94c81ce81b
> geode-core/src/main/java/org/apache/geode/internal/cache/DistTXState.java 6df26233417ea617e31d1928081c0719e26efd99
> geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java 56243e1b544f5958204e64c2ca391003aa1fd098
> geode-core/src/main/java/org/apache/geode/internal/cache/InternalCache.java 709308b57da847845ef9319bece18ebe9f25e569
> geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 45035d725241d60cbf6ed4f5417971c967925e86
> geode-core/src/main/java/org/apache/geode/internal/cache/execute/util/FindRestEnabledServersFunction.java 5da63adae8f3c0be4c2548034598d566efc517aa
> geode-core/src/main/java/org/apache/geode/internal/cache/persistence/PersistenceAdvisorImpl.java 7e30141c63a446852eea67aa4594241fca362668
> geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheCreation.java a5f0fc2bc7cf4250565aa8dd139004890b8da07d
> geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java d6a1efa73028e1b9514db67d2e3a4b564abee632
> geode-core/src/test/java/org/apache/geode/TXJUnitTest.java 54d9e503f2645d045487cea51011143602764f62
> geode-core/src/test/java/org/apache/geode/TXWriterTestCase.java 987f22f688ca695a8b37eacf239c69c329bb3b3b
> geode-core/src/test/java/org/apache/geode/cache/query/dunit/ResourceManagerWithQueryMonitorDUnitTest.java 903b2123097bac83045d887050f7eb955b27e3cd
> geode-core/src/test/java/org/apache/geode/cache/query/internal/index/NewDeclarativeIndexCreationJUnitTest.java 8a0f31cc87aa18b7b0928ab0e02405fd3ad75a7f
> geode-core/src/test/java/org/apache/geode/disttx/DistTXDebugDUnitTest.java 0d2f2b6f41e1b1dd829a41472206d0ccb6589a5e
> geode-core/src/test/java/org/apache/geode/disttx/DistTXJUnitTest.java 8abccc6c1ab447ffbd77379d7cf3d1c32905728e
> geode-core/src/test/java/org/apache/geode/disttx/DistTXPersistentDebugDUnitTest.java 5753f5c28de31353cdfb5235a981c1c9a88d75bd
> geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterJUnitTest.java 0a61b1f258d090090321c9ccff1a25781da7c8d1
> geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterOOMEJUnitTest.java b99d3fd25cdac5f1862927d098d9d6381894510e
> geode-core/src/test/java/org/apache/geode/disttx/DistributedTransactionDUnitTest.java 54715659a389c01b1b4b3fa79642d68f00b52b48
> geode-core/src/test/java/org/apache/geode/disttx/PRDistTXJUnitTest.java f27c0993a872b9879f5ee93b577939ee2b6919d9
> geode-core/src/test/java/org/apache/geode/internal/cache/PRTXJUnitTest.java d2bad641a47f68edb22da0f89a04c462ab48cd33
> geode-core/src/test/java/org/apache/geode/internal/cache/wan/parallel/ParallelQueueRemovalMessageJUnitTest.java d57ce125b9498f6439dbd0b13c72c67db6badb30
> geode-cq/src/main/java/org/apache/geode/cache/query/internal/cq/CqServiceImpl.java 570c06c5a4266b84c73e774bc3a021b39fa37b29
> geode-cq/src/test/java/org/apache/geode/cache/query/dunit/QueryMonitorDUnitTest.java f298fae6f1840302bc98668a09e4a9b2ed0c0b5c
> geode-web-api/src/main/java/org/apache/geode/rest/internal/web/controllers/CommonCrudController.java 0449a45c35569c672af06efdd9a1365283c435c3
>
>
> Diff: https://reviews.apache.org/r/58712/diff/2/
>
>
> Testing
> -------
>
> precheckin in progress
>
>
> Thanks,
>
> Kirk Lund
>
>
Re: Review Request 58712: GEODE-2632: partial cleanup of
GemFireCacheImpl
Posted by Patrick Rhomberg <pr...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58712/#review172966
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
Lines 2512-2519 (original), 2419-2426 (patched)
<https://reviews.apache.org/r/58712/#comment245999>
Would this read better?
```
GemFireCacheImpl cache = GemFireCacheImpl.getInstance();
if (cache == this || cache != null && !cache.isInitialized()){
cache = null;
}
return cache;
```
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
Lines 2804-2811 (original), 2600-2607 (patched)
<https://reviews.apache.org/r/58712/#comment246002>
Here and throughout... is there a reason to prefer caching `isDebugEnabled` at the beginning of the method? I can't imagine `logger.isDebugEnabled()` actually effects performance in a significant way here, and that seems to be the standard usage throughout the rest of this file / the codebase.
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
Lines 3290-3292 (original), 3064-3066 (patched)
<https://reviews.apache.org/r/58712/#comment246005>
Should any exception, even an explicitly caught one cancel, be caught without at least logging? Empty catches seem like bad form to me.
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
Lines 4436-4449 (original), 4142-4155 (patched)
<https://reviews.apache.org/r/58712/#comment246010>
To return to the question of single vs multiple returns... Would this read better?
```
@Override
public QueryService getQueryService() {
if (!isClient()) {
return new DefaultQueryService(this);
}
Pool defaultPool = getDefaultPool();
if (defaultPool == null) {
throw new IllegalStateException(
"Client cache does not have a default pool. Use getQueryService(String poolName) instead.");
}
return defaultPool.getQueryService();
}
```
geode-core/src/test/java/org/apache/geode/cache/query/internal/index/NewDeclarativeIndexCreationJUnitTest.java
Lines 165-166 (original), 162-163 (patched)
<https://reviews.apache.org/r/58712/#comment246012>
Delete dead code? Or is this one of the tests that have a message string that needs to be updated in the test?
- Patrick Rhomberg
On April 25, 2017, 5:05 p.m., Kirk Lund wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58712/
> -----------------------------------------------------------
>
> (Updated April 25, 2017, 5:05 p.m.)
>
>
> Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken Howe, and Patrick Rhomberg.
>
>
> Bugs: GEODE-2632
> https://issues.apache.org/jira/browse/GEODE-2632
>
>
> Repository: geode
>
>
> Description
> -------
>
> Fixup GemFireCacheImpl code. Also fix some affected collaborator classes and tests.
>
> * change SecurityService from static constant to final member field
> * fix typos and javadocs
> * narrow scope of constants, variables, methods
> * remove deadcode and useless comments/javadocs
> * fix misc IntelliJ warnings
> * add @Override annotations
> * improve some variable names
> * fix (some) generics and types
> * add TODOs for a couple problem areas that need further fixing
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/CancelCriterion.java e4f9a41599e521ce4bfbca9538d04dcc8edaa49b
> geode-core/src/main/java/org/apache/geode/cache/Cache.java bc4aa19eb99e4758512934c9b9c43bb18d4c20d8
> geode-core/src/main/java/org/apache/geode/cache/client/internal/ProxyCache.java 76306f51fc9479c7d9acaa28022ed908b674b7c0
> geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryMonitor.java d6acfbfc769af1cddd247dc0c3584cef00ea6f28
> geode-core/src/main/java/org/apache/geode/distributed/internal/DM.java 328a4f8265e54483c6bf34c2ad967f483661f155
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionManager.java 2ae86e65188b02ec6b8cbddc49ccbc73c55bfad1
> geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java 987e4911272ba6c37046d8e39a1187b71556736d
> geode-core/src/main/java/org/apache/geode/distributed/internal/LonerDistributionManager.java af4e674cf3c8c58ff23ae2e9ea620b94c81ce81b
> geode-core/src/main/java/org/apache/geode/internal/cache/DistTXState.java 6df26233417ea617e31d1928081c0719e26efd99
> geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java 56243e1b544f5958204e64c2ca391003aa1fd098
> geode-core/src/main/java/org/apache/geode/internal/cache/InternalCache.java 709308b57da847845ef9319bece18ebe9f25e569
> geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 45035d725241d60cbf6ed4f5417971c967925e86
> geode-core/src/main/java/org/apache/geode/internal/cache/execute/util/FindRestEnabledServersFunction.java 5da63adae8f3c0be4c2548034598d566efc517aa
> geode-core/src/main/java/org/apache/geode/internal/cache/persistence/PersistenceAdvisorImpl.java 7e30141c63a446852eea67aa4594241fca362668
> geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheCreation.java a5f0fc2bc7cf4250565aa8dd139004890b8da07d
> geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java d6a1efa73028e1b9514db67d2e3a4b564abee632
> geode-core/src/test/java/org/apache/geode/TXJUnitTest.java 54d9e503f2645d045487cea51011143602764f62
> geode-core/src/test/java/org/apache/geode/TXWriterTestCase.java 987f22f688ca695a8b37eacf239c69c329bb3b3b
> geode-core/src/test/java/org/apache/geode/cache/query/dunit/ResourceManagerWithQueryMonitorDUnitTest.java 903b2123097bac83045d887050f7eb955b27e3cd
> geode-core/src/test/java/org/apache/geode/cache/query/internal/index/NewDeclarativeIndexCreationJUnitTest.java 8a0f31cc87aa18b7b0928ab0e02405fd3ad75a7f
> geode-core/src/test/java/org/apache/geode/disttx/DistTXDebugDUnitTest.java 0d2f2b6f41e1b1dd829a41472206d0ccb6589a5e
> geode-core/src/test/java/org/apache/geode/disttx/DistTXJUnitTest.java 8abccc6c1ab447ffbd77379d7cf3d1c32905728e
> geode-core/src/test/java/org/apache/geode/disttx/DistTXPersistentDebugDUnitTest.java 5753f5c28de31353cdfb5235a981c1c9a88d75bd
> geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterJUnitTest.java 0a61b1f258d090090321c9ccff1a25781da7c8d1
> geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterOOMEJUnitTest.java b99d3fd25cdac5f1862927d098d9d6381894510e
> geode-core/src/test/java/org/apache/geode/disttx/DistributedTransactionDUnitTest.java 54715659a389c01b1b4b3fa79642d68f00b52b48
> geode-core/src/test/java/org/apache/geode/disttx/PRDistTXJUnitTest.java f27c0993a872b9879f5ee93b577939ee2b6919d9
> geode-core/src/test/java/org/apache/geode/internal/cache/PRTXJUnitTest.java d2bad641a47f68edb22da0f89a04c462ab48cd33
> geode-core/src/test/java/org/apache/geode/internal/cache/wan/parallel/ParallelQueueRemovalMessageJUnitTest.java d57ce125b9498f6439dbd0b13c72c67db6badb30
> geode-cq/src/main/java/org/apache/geode/cache/query/internal/cq/CqServiceImpl.java 570c06c5a4266b84c73e774bc3a021b39fa37b29
> geode-cq/src/test/java/org/apache/geode/cache/query/dunit/QueryMonitorDUnitTest.java f298fae6f1840302bc98668a09e4a9b2ed0c0b5c
> geode-web-api/src/main/java/org/apache/geode/rest/internal/web/controllers/CommonCrudController.java 0449a45c35569c672af06efdd9a1365283c435c3
>
>
> Diff: https://reviews.apache.org/r/58712/diff/2/
>
>
> Testing
> -------
>
> precheckin in progress
>
>
> Thanks,
>
> Kirk Lund
>
>
Re: Review Request 58712: GEODE-2632: partial cleanup of
GemFireCacheImpl
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58712/
-----------------------------------------------------------
(Updated April 25, 2017, 5:05 p.m.)
Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken Howe, and Patrick Rhomberg.
Bugs: GEODE-2632
https://issues.apache.org/jira/browse/GEODE-2632
Repository: geode
Description (updated)
-------
Fixup GemFireCacheImpl code. Also fix some affected collaborator classes and tests.
* change SecurityService from static constant to final member field
* fix typos and javadocs
* narrow scope of constants, variables, methods
* remove deadcode and useless comments/javadocs
* fix misc IntelliJ warnings
* add @Override annotations
* improve some variable names
* fix (some) generics and types
* add TODOs for a couple problem areas that need further fixing
Diffs
-----
geode-core/src/main/java/org/apache/geode/CancelCriterion.java e4f9a41599e521ce4bfbca9538d04dcc8edaa49b
geode-core/src/main/java/org/apache/geode/cache/Cache.java bc4aa19eb99e4758512934c9b9c43bb18d4c20d8
geode-core/src/main/java/org/apache/geode/cache/client/internal/ProxyCache.java 76306f51fc9479c7d9acaa28022ed908b674b7c0
geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryMonitor.java d6acfbfc769af1cddd247dc0c3584cef00ea6f28
geode-core/src/main/java/org/apache/geode/distributed/internal/DM.java 328a4f8265e54483c6bf34c2ad967f483661f155
geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionManager.java 2ae86e65188b02ec6b8cbddc49ccbc73c55bfad1
geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java 987e4911272ba6c37046d8e39a1187b71556736d
geode-core/src/main/java/org/apache/geode/distributed/internal/LonerDistributionManager.java af4e674cf3c8c58ff23ae2e9ea620b94c81ce81b
geode-core/src/main/java/org/apache/geode/internal/cache/DistTXState.java 6df26233417ea617e31d1928081c0719e26efd99
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java 56243e1b544f5958204e64c2ca391003aa1fd098
geode-core/src/main/java/org/apache/geode/internal/cache/InternalCache.java 709308b57da847845ef9319bece18ebe9f25e569
geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 45035d725241d60cbf6ed4f5417971c967925e86
geode-core/src/main/java/org/apache/geode/internal/cache/execute/util/FindRestEnabledServersFunction.java 5da63adae8f3c0be4c2548034598d566efc517aa
geode-core/src/main/java/org/apache/geode/internal/cache/persistence/PersistenceAdvisorImpl.java 7e30141c63a446852eea67aa4594241fca362668
geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheCreation.java a5f0fc2bc7cf4250565aa8dd139004890b8da07d
geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java d6a1efa73028e1b9514db67d2e3a4b564abee632
geode-core/src/test/java/org/apache/geode/TXJUnitTest.java 54d9e503f2645d045487cea51011143602764f62
geode-core/src/test/java/org/apache/geode/TXWriterTestCase.java 987f22f688ca695a8b37eacf239c69c329bb3b3b
geode-core/src/test/java/org/apache/geode/cache/query/dunit/ResourceManagerWithQueryMonitorDUnitTest.java 903b2123097bac83045d887050f7eb955b27e3cd
geode-core/src/test/java/org/apache/geode/cache/query/internal/index/NewDeclarativeIndexCreationJUnitTest.java 8a0f31cc87aa18b7b0928ab0e02405fd3ad75a7f
geode-core/src/test/java/org/apache/geode/disttx/DistTXDebugDUnitTest.java 0d2f2b6f41e1b1dd829a41472206d0ccb6589a5e
geode-core/src/test/java/org/apache/geode/disttx/DistTXJUnitTest.java 8abccc6c1ab447ffbd77379d7cf3d1c32905728e
geode-core/src/test/java/org/apache/geode/disttx/DistTXPersistentDebugDUnitTest.java 5753f5c28de31353cdfb5235a981c1c9a88d75bd
geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterJUnitTest.java 0a61b1f258d090090321c9ccff1a25781da7c8d1
geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterOOMEJUnitTest.java b99d3fd25cdac5f1862927d098d9d6381894510e
geode-core/src/test/java/org/apache/geode/disttx/DistributedTransactionDUnitTest.java 54715659a389c01b1b4b3fa79642d68f00b52b48
geode-core/src/test/java/org/apache/geode/disttx/PRDistTXJUnitTest.java f27c0993a872b9879f5ee93b577939ee2b6919d9
geode-core/src/test/java/org/apache/geode/internal/cache/PRTXJUnitTest.java d2bad641a47f68edb22da0f89a04c462ab48cd33
geode-core/src/test/java/org/apache/geode/internal/cache/wan/parallel/ParallelQueueRemovalMessageJUnitTest.java d57ce125b9498f6439dbd0b13c72c67db6badb30
geode-cq/src/main/java/org/apache/geode/cache/query/internal/cq/CqServiceImpl.java 570c06c5a4266b84c73e774bc3a021b39fa37b29
geode-cq/src/test/java/org/apache/geode/cache/query/dunit/QueryMonitorDUnitTest.java f298fae6f1840302bc98668a09e4a9b2ed0c0b5c
geode-web-api/src/main/java/org/apache/geode/rest/internal/web/controllers/CommonCrudController.java 0449a45c35569c672af06efdd9a1365283c435c3
Diff: https://reviews.apache.org/r/58712/diff/2/
Testing
-------
precheckin in progress
Thanks,
Kirk Lund
Re: Review Request 58712: GEODE-2632: partial cleanup of
GemFireCacheImpl
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58712/
-----------------------------------------------------------
(Updated April 25, 2017, 5:04 p.m.)
Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken Howe, and Patrick Rhomberg.
Changes
-------
Removed final keyword from several methods (I somehow managed to add it to these methods)
Bugs: GEODE-2632
https://issues.apache.org/jira/browse/GEODE-2632
Repository: geode
Description
-------
Fixup GemFireCacheImpl code. Also fix some affected collaborator classes and tests.
* change SecurityService from static constant to final member field
* fix typos and javadocs
* reduce scope of constants, variables, methods
* remove deadcode and useless comments/javadocs
* fix misc IntelliJ warnings
* add @Override annotations
* improve some variable names
* add TODOs for a couple problem areas that need further fixing
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/CancelCriterion.java e4f9a41599e521ce4bfbca9538d04dcc8edaa49b
geode-core/src/main/java/org/apache/geode/cache/Cache.java bc4aa19eb99e4758512934c9b9c43bb18d4c20d8
geode-core/src/main/java/org/apache/geode/cache/client/internal/ProxyCache.java 76306f51fc9479c7d9acaa28022ed908b674b7c0
geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryMonitor.java d6acfbfc769af1cddd247dc0c3584cef00ea6f28
geode-core/src/main/java/org/apache/geode/distributed/internal/DM.java 328a4f8265e54483c6bf34c2ad967f483661f155
geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionManager.java 2ae86e65188b02ec6b8cbddc49ccbc73c55bfad1
geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java 987e4911272ba6c37046d8e39a1187b71556736d
geode-core/src/main/java/org/apache/geode/distributed/internal/LonerDistributionManager.java af4e674cf3c8c58ff23ae2e9ea620b94c81ce81b
geode-core/src/main/java/org/apache/geode/internal/cache/DistTXState.java 6df26233417ea617e31d1928081c0719e26efd99
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java 56243e1b544f5958204e64c2ca391003aa1fd098
geode-core/src/main/java/org/apache/geode/internal/cache/InternalCache.java 709308b57da847845ef9319bece18ebe9f25e569
geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 45035d725241d60cbf6ed4f5417971c967925e86
geode-core/src/main/java/org/apache/geode/internal/cache/execute/util/FindRestEnabledServersFunction.java 5da63adae8f3c0be4c2548034598d566efc517aa
geode-core/src/main/java/org/apache/geode/internal/cache/persistence/PersistenceAdvisorImpl.java 7e30141c63a446852eea67aa4594241fca362668
geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheCreation.java a5f0fc2bc7cf4250565aa8dd139004890b8da07d
geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java d6a1efa73028e1b9514db67d2e3a4b564abee632
geode-core/src/test/java/org/apache/geode/TXJUnitTest.java 54d9e503f2645d045487cea51011143602764f62
geode-core/src/test/java/org/apache/geode/TXWriterTestCase.java 987f22f688ca695a8b37eacf239c69c329bb3b3b
geode-core/src/test/java/org/apache/geode/cache/query/dunit/ResourceManagerWithQueryMonitorDUnitTest.java 903b2123097bac83045d887050f7eb955b27e3cd
geode-core/src/test/java/org/apache/geode/cache/query/internal/index/NewDeclarativeIndexCreationJUnitTest.java 8a0f31cc87aa18b7b0928ab0e02405fd3ad75a7f
geode-core/src/test/java/org/apache/geode/disttx/DistTXDebugDUnitTest.java 0d2f2b6f41e1b1dd829a41472206d0ccb6589a5e
geode-core/src/test/java/org/apache/geode/disttx/DistTXJUnitTest.java 8abccc6c1ab447ffbd77379d7cf3d1c32905728e
geode-core/src/test/java/org/apache/geode/disttx/DistTXPersistentDebugDUnitTest.java 5753f5c28de31353cdfb5235a981c1c9a88d75bd
geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterJUnitTest.java 0a61b1f258d090090321c9ccff1a25781da7c8d1
geode-core/src/test/java/org/apache/geode/disttx/DistTXWriterOOMEJUnitTest.java b99d3fd25cdac5f1862927d098d9d6381894510e
geode-core/src/test/java/org/apache/geode/disttx/DistributedTransactionDUnitTest.java 54715659a389c01b1b4b3fa79642d68f00b52b48
geode-core/src/test/java/org/apache/geode/disttx/PRDistTXJUnitTest.java f27c0993a872b9879f5ee93b577939ee2b6919d9
geode-core/src/test/java/org/apache/geode/internal/cache/PRTXJUnitTest.java d2bad641a47f68edb22da0f89a04c462ab48cd33
geode-core/src/test/java/org/apache/geode/internal/cache/wan/parallel/ParallelQueueRemovalMessageJUnitTest.java d57ce125b9498f6439dbd0b13c72c67db6badb30
geode-cq/src/main/java/org/apache/geode/cache/query/internal/cq/CqServiceImpl.java 570c06c5a4266b84c73e774bc3a021b39fa37b29
geode-cq/src/test/java/org/apache/geode/cache/query/dunit/QueryMonitorDUnitTest.java f298fae6f1840302bc98668a09e4a9b2ed0c0b5c
geode-web-api/src/main/java/org/apache/geode/rest/internal/web/controllers/CommonCrudController.java 0449a45c35569c672af06efdd9a1365283c435c3
Diff: https://reviews.apache.org/r/58712/diff/2/
Changes: https://reviews.apache.org/r/58712/diff/1-2/
Testing
-------
precheckin in progress
Thanks,
Kirk Lund