[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7113: GEODE-9809: stop monitoring of destroyed regions

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

dschneider-pivotal commented on a change in pull request #7113:
URL: https://github.com/apache/geode/pull/7113#discussion_r790964551



##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/persistence/PersistenceAdvisorImpl.java
##########
@@ -721,12 +721,19 @@ public void addListener(PersistentStateListener listener) {
   @Override
   public void removeListener(PersistentStateListener listener) {
     synchronized (this) {
+      if (persistentStateListeners.isEmpty()) {
+        return;
+      }
       Set<PersistentStateListener> tmpListeners = new HashSet<>(persistentStateListeners);
       tmpListeners.remove(listener);
       persistentStateListeners = Collections.unmodifiableSet(tmpListeners);
     }
   }
 
+  Set<PersistentStateListener> getPersistentStateListenerSet() {

Review comment:
       can you mark this with an annotation to say it is visible for testing?

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/PersistentBucketRecoverer.java
##########
@@ -182,12 +186,23 @@ public void run2() {
         if (!warningLogged) {
           sleepMillis = SLEEP_PERIOD / 2;
         }
+        if (regions.isEmpty()) {
+          break;
+        }
         Thread.sleep(sleepMillis);
 
         if (membershipChanged) {
           membershipChanged = false;
           for (RegionStatus region : regions) {
-            region.logWaitingForMembers();
+            try {

Review comment:
       Instead of having a destroyedRegions collection, I think it would be better to use an Iterator on regions. Then in the catch you can just call iterator.remove. 

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/persistence/PersistenceAdvisorImpl.java
##########
@@ -1155,6 +1162,7 @@ public void close() {
     isClosed = true;
     persistentMemberManager.removeRevocationListener(profileChangeListener);
     cacheDistributionAdvisor.removeProfileChangeListener(profileChangeListener);
+    persistentStateListeners = Collections.emptySet();

Review comment:
       But it also seems like if "addListener" is called after this it should not do anything. You could put the "isClosed=true" in the "sync this" block and then test for isClosed in addListener and do nothing. It looks like this class uses "sync this" to protect concurrent access to this set but it also has the method resetState syncing on this but then resetState does not change persistentStateListeners. I don't understand resetState but I think it would be safe to add this extra sync in "close" and the isClosed check in addListener

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/persistence/PersistenceAdvisorImpl.java
##########
@@ -1155,6 +1162,7 @@ public void close() {
     isClosed = true;
     persistentMemberManager.removeRevocationListener(profileChangeListener);
     cacheDistributionAdvisor.removeProfileChangeListener(profileChangeListener);
+    persistentStateListeners = Collections.emptySet();

Review comment:
       I think this new line should also be in a synchronized this block since it sets persistentStateListeners 




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