jchen21 commented on a change in pull request #6964:
URL: https://github.com/apache/geode/pull/6964#discussion_r725399568
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -1464,10 +1464,18 @@ private void registerPartitionedRegion(boolean
storesData) {
if (logger.isDebugEnabled()) {
logger.debug("registerPartitionedRegion: unable to obtain lock for
{}", this);
}
- cleanupFailedInitialization();
- throw new PartitionedRegionException(
- "Can not create PartitionedRegion (failed to acquire RegionLock).",
- lsde);
+ PartitionedRegionException pre = null;
+ try {
+ cleanupFailedInitialization();
+ } catch (RuntimeException rte) {
+ rte.initCause(lsde);
+ pre = new PartitionedRegionException("Can not create
PartitionedRegion.", rte);
Review comment:
It's better to keep the original message string with `(failed to acquire
RegionLock)` as in line 1476.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -1464,10 +1464,18 @@ private void registerPartitionedRegion(boolean
storesData) {
if (logger.isDebugEnabled()) {
logger.debug("registerPartitionedRegion: unable to obtain lock for
{}", this);
}
- cleanupFailedInitialization();
- throw new PartitionedRegionException(
- "Can not create PartitionedRegion (failed to acquire RegionLock).",
- lsde);
+ PartitionedRegionException pre = null;
+ try {
+ cleanupFailedInitialization();
Review comment:
If we are enclosing `cleanupFailedInitialization` with `try/catch`
block, should we also enclose `cleanupFailedInitialization` in line 1480 with
`try/catch` block?
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -5557,12 +5565,16 @@ public void cleanupFailedInitialization() {
int serials[] = getRegionAdvisor().getBucketSerials();
RegionEventImpl event = new RegionEventImpl(this, Operation.REGION_CLOSE,
null, false,
getMyId(), generateEventID()/* generate EventID */);
+ RuntimeException savedFirstRuntimeException = null;
try {
sendDestroyRegionMessage(event, serials);
} catch (Exception ex) {
logger.warn(
"PartitionedRegion#cleanupFailedInitialization(): Failed to clean
the PartionRegion data store",
ex);
+ if (savedFirstRuntimeException == null && ex instanceof
RuntimeException) {
+ savedFirstRuntimeException = (RuntimeException) ex;
Review comment:
Casting into `RuntimeException` here is not necessary. Also, if `ex` is
not an instance of `RuntimeException`, it is not thrown, only log a warning
message.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -5602,6 +5620,11 @@ public void cleanupFailedInitialization() {
if (logger.isDebugEnabled()) {
logger.debug("cleanupFailedInitialization: end of {}", getName());
}
+ if (savedFirstRuntimeException != null) {
+ logger.info("cleanupFailedInitialization originally failed with ",
Review comment:
Better not use `info` level here, since there is an exception. How about
`logger.warn`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
