geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jared Stewart <jstew...@pivotal.io>
Subject Re: Review Request 58712: GEODE-2632: partial cleanup of GemFireCacheImpl
Date Tue, 25 Apr 2017 21:05:02 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message