You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Swapnil Bawaskar <sb...@apache.org> on 2015/12/31 22:09:12 UTC
Review Request 41836: GEODE-719: Add error logs while cache.xml
processing
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41836/
-----------------------------------------------------------
Review request for geode.
Repository: geode
Description
-------
While processing cache.xml, log an error before throwing an exception so
that the reason for cache close is clear by looking at the log.
Diffs
-----
gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/GemFireCacheImpl.java 506bd7aeeb2c85105d19eaabf960c48639e6de24
Diff: https://reviews.apache.org/r/41836/diff/
Testing
-------
Thanks,
Swapnil Bawaskar
Re: Review Request 41836: GEODE-719: Add error logs while cache.xml
processing
Posted by Jens Deppe <jd...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41836/#review112404
-----------------------------------------------------------
Ship it!
Ship It!
- Jens Deppe
On Dec. 31, 2015, 9:09 p.m., Swapnil Bawaskar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41836/
> -----------------------------------------------------------
>
> (Updated Dec. 31, 2015, 9:09 p.m.)
>
>
> Review request for geode.
>
>
> Repository: geode
>
>
> Description
> -------
>
> While processing cache.xml, log an error before throwing an exception so
> that the reason for cache close is clear by looking at the log.
>
>
> Diffs
> -----
>
> gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/GemFireCacheImpl.java 506bd7aeeb2c85105d19eaabf960c48639e6de24
>
> Diff: https://reviews.apache.org/r/41836/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Swapnil Bawaskar
>
>
Re: Review Request 41836: GEODE-719: Add error logs while cache.xml
processing
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41836/#review113145
-----------------------------------------------------------
gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/GemFireCacheImpl.java (line 1361)
<https://reviews.apache.org/r/41836/#comment173639>
This method has other places it can throw exceptions. Also the caller of it has the same problem. The GemFireCacheImpl constructor may also throw exceptions that will not be logged.
I think in general some code should be added when the constructor and init methods are called to catch all RuntimeExceptions or Errors and log them as an error before rethrowing them.
- Darrel Schneider
On Dec. 31, 2015, 1:09 p.m., Swapnil Bawaskar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41836/
> -----------------------------------------------------------
>
> (Updated Dec. 31, 2015, 1:09 p.m.)
>
>
> Review request for geode.
>
>
> Repository: geode
>
>
> Description
> -------
>
> While processing cache.xml, log an error before throwing an exception so
> that the reason for cache close is clear by looking at the log.
>
>
> Diffs
> -----
>
> gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/GemFireCacheImpl.java 506bd7aeeb2c85105d19eaabf960c48639e6de24
>
> Diff: https://reviews.apache.org/r/41836/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Swapnil Bawaskar
>
>
Re: Review Request 41836: GEODE-719: Add error logs while cache.xml
processing
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 41836: GEODE-719: Add error logs while cache.xml
processing
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41836/#review116129
-----------------------------------------------------------
Ship it!
Ship It!
- Darrel Schneider
On Jan. 22, 2016, 11:22 a.m., Swapnil Bawaskar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41836/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 11:22 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/cache/client/ClientCacheFactory.java c39e33a
> gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegion.java 45adbd6
> gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/GemFireCacheImpl.java 9d77556
> gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java 2bc2f05
> gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/PoolManagerImpl.java 35b043f
> gemfire-core/src/main/java/com/gemstone/gemfire/internal/i18n/ParentLocalizedStrings.java 780fe18
> gemfire-core/src/test/java/com/gemstone/gemfire/cache30/CacheXml30DUnitTest.java adafa69
> gemfire-core/src/test/java/com/gemstone/gemfire/cache30/CacheXml41DUnitTest.java 5a41d31
> gemfire-core/src/test/java/com/gemstone/gemfire/cache30/CacheXml57DUnitTest.java 47bb612
> gemfire-core/src/test/java/com/gemstone/gemfire/cache30/CacheXml60DUnitTest.java 8699a05
> gemfire-core/src/test/java/com/gemstone/gemfire/cache30/CacheXml66DUnitTest.java e531405
> gemfire-core/src/test/java/com/gemstone/gemfire/cache30/CacheXml90DUnitTest.java 7fd2016
>
> Diff: https://reviews.apache.org/r/41836/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Swapnil Bawaskar
>
>
Re: Review Request 41836: GEODE-719: Add error logs while cache.xml
processing
Posted by Swapnil Bawaskar <sb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41836/
-----------------------------------------------------------
(Updated Jan. 22, 2016, 7:22 p.m.)
Review request for geode and Darrel Schneider.
Changes
-------
Incorporating Darrel's feedback.
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 (updated)
-----
gemfire-core/src/main/java/com/gemstone/gemfire/cache/client/ClientCacheFactory.java c39e33a
gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegion.java 45adbd6
gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/GemFireCacheImpl.java 9d77556
gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java 2bc2f05
gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/PoolManagerImpl.java 35b043f
gemfire-core/src/main/java/com/gemstone/gemfire/internal/i18n/ParentLocalizedStrings.java 780fe18
gemfire-core/src/test/java/com/gemstone/gemfire/cache30/CacheXml30DUnitTest.java adafa69
gemfire-core/src/test/java/com/gemstone/gemfire/cache30/CacheXml41DUnitTest.java 5a41d31
gemfire-core/src/test/java/com/gemstone/gemfire/cache30/CacheXml57DUnitTest.java 47bb612
gemfire-core/src/test/java/com/gemstone/gemfire/cache30/CacheXml60DUnitTest.java 8699a05
gemfire-core/src/test/java/com/gemstone/gemfire/cache30/CacheXml66DUnitTest.java e531405
gemfire-core/src/test/java/com/gemstone/gemfire/cache30/CacheXml90DUnitTest.java 7fd2016
Diff: https://reviews.apache.org/r/41836/diff/
Testing
-------
Thanks,
Swapnil Bawaskar
Re: Review Request 41836: GEODE-719: Add error logs while cache.xml
processing
Posted by Swapnil Bawaskar <sb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41836/
-----------------------------------------------------------
(Updated Jan. 8, 2016, 5:14 p.m.)
Review request for geode and Darrel Schneider.
Changes
-------
- 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.
Repository: geode
Description (updated)
-------
- 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 (updated)
-----
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