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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
