[GitHub] [geode] jchen21 commented on a change in pull request #6964: GEODE-9705: When create PR failed with DistributedSystemDisconnectedE…

[GitBox <gi...@apache.org>]

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: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org