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