----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41836/#review113503 -----------------------------------------------------------
gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/GemFireCacheImpl.java (line 771) <https://reviews.apache.org/r/41836/#comment174252> A bug exists in this method. Change "true" to "isClient". Currently the only caller of this method sets isClient to true so we have not exposed this bug. gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/GemFireCacheImpl.java (line 775) <https://reviews.apache.org/r/41836/#comment174256> I think you should have one catch for all the types of "expected" exceptions (like CacheXmlException, IllegalStateException) and have them just do logger.error(e.getLocalizedMessage()); and the rethrow. But all other "unexpected" Error and RuntimeException do this: catch (Error | RuntimeException e) { logger.error(e); throw e; } That way we will also get a call stack logged. gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/GemFireCacheImpl.java (line 781) <https://reviews.apache.org/r/41836/#comment174253> Change the body of this create to be: return create(false, null, system, cacheConfig); gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/GemFireCacheImpl.java (line 806) <https://reviews.apache.org/r/41836/#comment174254> Does a logger.error need to be added here? gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java (line 615) <https://reviews.apache.org/r/41836/#comment174247> Change "OFF_HEAD" to "OFF_HEAP". :-) gemfire-core/src/test/java/com/gemstone/gemfire/cache30/CacheXml30DUnitTest.java (line 100) <https://reviews.apache.org/r/41836/#comment174248> I think you could have just done the following: addExpectedException(GemFireCache_DECLARATIVE_CACHE_XML_FILERESOURCE_0_DOES_NOT_EXIST.toLocalizedString(nonExistent.getPath())); and no need for the finally because it happens automatically now. The way you did it works; it is just a bit wordier. - Darrel Schneider On Jan. 8, 2016, 9:14 a.m., Swapnil Bawaskar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41836/ > ----------------------------------------------------------- > > (Updated Jan. 8, 2016, 9:14 a.m.) > > > Review request for geode and Darrel Schneider. > > > Repository: geode > > > Description > ------- > > - Catch all Errors and RuntimeExceptions while creating a cache and log > an error level message. > - Display the cache.xml contents before trying to initialize the cache > so that the errors have a better context. > - Converted some exception messages to i18n. > > > Diffs > ----- > > > gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegion.java > 45adbd652fc358e85e7b388791f5aa6e268845f2 > > gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/GemFireCacheImpl.java > 9d775564b6eb576374f968b3dd45466e506ffc73 > > gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java > 2bc2f05b837128a5c35d62017e412e72ac39ccfe > > gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/PoolManagerImpl.java > 35b043f65b452265a04ef485c87cd20810c1db56 > > gemfire-core/src/main/java/com/gemstone/gemfire/internal/i18n/ParentLocalizedStrings.java > 780fe184a3819f52ca5067006cf12f35f451d8f7 > > gemfire-core/src/test/java/com/gemstone/gemfire/cache30/CacheXml30DUnitTest.java > adafa69246ee1c1718c3083954f63cd96a9e5e01 > > gemfire-core/src/test/java/com/gemstone/gemfire/cache30/CacheXml41DUnitTest.java > 5a41d3159098d09b26899491b6c1d2015556219f > > gemfire-core/src/test/java/com/gemstone/gemfire/cache30/CacheXml57DUnitTest.java > 47bb6124cdfd3e5e4d8aa1f64123fa69d02eb726 > > gemfire-core/src/test/java/com/gemstone/gemfire/cache30/CacheXml60DUnitTest.java > 8699a058dada35d4b8ff533a9c3e9a289aabc031 > > gemfire-core/src/test/java/com/gemstone/gemfire/cache30/CacheXml66DUnitTest.java > e5314054952c45aa3eabbf06e2cfd74cb91b01be > > gemfire-core/src/test/java/com/gemstone/gemfire/cache30/CacheXml90DUnitTest.java > 7fd20165fe05c2ed12f34611ab4eef52c32d796d > > Diff: https://reviews.apache.org/r/41836/diff/ > > > Testing > ------- > > > Thanks, > > Swapnil Bawaskar > >
