bschuchardt commented on a change in pull request #6037:
URL: https://github.com/apache/geode/pull/6037#discussion_r579399721
##########
File path:
geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
##########
@@ -273,6 +273,9 @@ boolean isDistributionMessage() {
/**
* Members that have sent a shutdown message. This is used to suppress
suspect processing that
* otherwise becomes pretty aggressive when a member is shutting down.
+ *
+ * Accesses to this map should be synchronized on the map to avoid concurrent
+ * modification exceptions
*/
private final Map<ID, Object> shutdownMembers = new BoundedLinkedHashMap<>();
Review comment:
I'm wondering if this could be made a synchronized collection with
Collections.synchronizedMap(). Then we could get rid of the external
synchronization.
##########
File path:
geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
##########
@@ -677,10 +658,12 @@ private void removeWithViewLock(ID dm, boolean crashed,
String reason) {
return; // Explicit deletion, no upcall.
}
- if (!shutdownMembers.containsKey(dm)) {
- // if we've received a shutdown message then DistributionManager will
already have
- // notified listeners
- listener.memberDeparted(dm, crashed, reason);
+ synchronized (shutdownMembers) {
+ if (!shutdownMembers.containsKey(dm)) {
+ // if we've received a shutdown message then DistributionManager will
already have
+ // notified listeners
+ listener.memberDeparted(dm, crashed, reason);
Review comment:
Is it necessary to hold the lock on shutdownMembers while invoking a
listener? The point of this check is to avoid invoking memberDeparted if a
shutdown message from **dm** has already been processed. In that case we've
already invoked memberDeparted on the listener.
##########
File path:
geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
##########
@@ -1524,15 +1487,9 @@ public boolean isShunned(ID m) {
}
private boolean isShunnedOrNew(final ID m) {
- latestViewReadLock.lock();
- try {
- return shunnedMembers.containsKey(m) || isNew(m);
- } finally { // synchronized
- latestViewReadLock.unlock();
- }
+ return shunnedMembers.containsKey(m) || isNew(m);
}
- // must be invoked under view read or write lock
private boolean isNew(final ID m) {
return !latestView.contains(m) && !surpriseMembers.containsKey(m);
Review comment:
I think the read-lock is still needed here. While the "shunned" state
of the member is disconnected from the other two checks, surpriseMembers is
modified while forming a new membership view and we want these two checks to be
atomic.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
