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