You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by bu...@apache.org on 2021/03/25 21:49:32 UTC
[geode] branch support/1.14 updated (e3653c8 -> f274c73)
This is an automated email from the ASF dual-hosted git repository.
burcham pushed a change to branch support/1.14
in repository https://gitbox.apache.org/repos/asf/geode.git.
from e3653c8 do more testing in parallel
new ff947f1 GEODE-7245: Remove most uses of latestViewReadLock in GMSMembership (#6037)
new f274c73 GEODE-8972: remove shunnedMembers collection from GMSMembership (#6089)
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../internal/membership/api/Membership.java | 7 -
.../internal/membership/api/MembershipView.java | 147 +++----
.../internal/membership/gms/GMSMembership.java | 429 +++++----------------
3 files changed, 150 insertions(+), 433 deletions(-)
[geode] 02/02: GEODE-8972: remove shunnedMembers collection from
GMSMembership (#6089)
Posted by bu...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
burcham pushed a commit to branch support/1.14
in repository https://gitbox.apache.org/repos/asf/geode.git
commit f274c73c105c51fe798ac0f50ef058ec1a939867
Author: Kamilla Aslami <ka...@vmware.com>
AuthorDate: Tue Mar 9 09:59:13 2021 -0600
GEODE-8972: remove shunnedMembers collection from GMSMembership (#6089)
* GEODE-8972: remove shunnedMembers collection from GMSMembership
* Changes after the code review
(cherry picked from commit c2fc107bad1de221157072470e2a6ad426533f20)
---
.../internal/membership/api/Membership.java | 7 -
.../internal/membership/gms/GMSMembership.java | 168 ++-------------------
2 files changed, 12 insertions(+), 163 deletions(-)
diff --git a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/Membership.java b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/Membership.java
index 91583d2..dfaa045 100644
--- a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/Membership.java
+++ b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/Membership.java
@@ -243,13 +243,6 @@ public interface Membership<ID extends MemberIdentifier> {
*/
Throwable getShutdownCause();
- /**
- * If this member is shunned, ensure that a warning is generated at least once.
- *
- * @param mbr the member that may be shunned
- */
- void warnShun(ID mbr);
-
boolean addSurpriseMember(ID mbr);
/**
diff --git a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
index 3c05988..a119c02 100644
--- a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
+++ b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
@@ -258,19 +258,6 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
private volatile boolean hasJoined;
/**
- * Members of the distributed system that we believe have shut down. Keys are instances of
- * {@link ID}, values are Longs indicating the time this member was
- * shunned.
- *
- * Members are removed after {@link #SHUNNED_SUNSET} seconds have passed.
- *
- * Writing to this list needs to be under {@link #latestViewWriteLock}
- *
- * @see System#currentTimeMillis()
- */
- private final Map<ID, Long> shunnedMembers = new ConcurrentHashMap<>();
-
- /**
* 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.
*/
@@ -286,15 +273,6 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
private final Lock shutdownMembersWriteLock = shutdownMembersLock.writeLock();
/**
- * per bug 39552, keep a list of members that have been shunned and for which a message is
- * printed. Contents of this list are cleared at the same time they are removed from
- * {@link #shunnedMembers}.
- *
- * Writing to this list needs to be under {@link #latestViewWriteLock}
- */
- private final Set<ID> shunnedAndWarnedMembers =
- Collections.newSetFromMap(new ConcurrentHashMap<>());
- /**
* The identities and birth-times of others that we have allowed into membership at the
* distributed system level, but have not yet appeared in a view.
* <p>
@@ -323,14 +301,6 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
private final Map<ID, Long> suspectedMembers = new ConcurrentHashMap<>();
/**
- * Length of time, in seconds, that a member is retained in the zombie set
- *
- * @see #shunnedMembers
- */
- private static final int SHUNNED_SUNSET = Integer
- .getInteger(GeodeGlossary.GEMFIRE_PREFIX + "shunned-member-timeout", 300).intValue();
-
- /**
* Set to true when the service should stop.
*/
private volatile boolean shutdownInProgress = false;
@@ -442,14 +412,7 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
}
if (shutdownInProgress()) {
- addShunnedMember(m);
continue; // no additions processed after shutdown begins
- } else {
- boolean wasShunned = endShun(m); // bug #45158 - no longer shun a process that is now in
- // view
- if (wasShunned && logger.isDebugEnabled()) {
- logger.debug("No longer shunning {} as it is in the current membership view", m);
- }
}
logger.info("Membership: Processing addition <{}>", m);
@@ -646,11 +609,10 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
/**
* Remove a member. {@link #latestViewWriteLock} must be held before this method is called. If
- * member
- * is not already shunned, the uplevel event handler is invoked.
+ * member is not already shunned, the uplevel event handler is invoked.
*/
private void removeWithViewLock(ID dm, boolean crashed, String reason) {
- boolean wasShunned = isShunned(dm);
+ final boolean wasShunned = isShunned(dm);
// Delete resources
destroyMember(dm, reason);
@@ -695,7 +657,6 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
@Override
public void startupMessageFailed(ID mbr, String failureMessage) {
// fix for bug #40666
- addShunnedMember(mbr);
listener.memberDeparted(mbr, true,
"failed to pass startup checks");
}
@@ -745,7 +706,6 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
// okay to ignore
}
}).start();
- addShunnedMember(member);
return false;
}
@@ -844,20 +804,6 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
dispatchMessage(msg);
}
- @Override
- public void warnShun(ID m) {
- if (!shunnedMembers.containsKey(m)) {
- return; // not shunned
- }
- if (shunnedAndWarnedMembers.contains(m)) {
- return; // already warned
- }
- shunnedAndWarnedMembers.add(m);
- // issue warning outside of sync since it may cause messaging and we don't
- // want to hold the view lock while doing that
- logger.warn("Membership: disregarding shunned member <{}>", m);
- }
-
/**
* Logic for processing a distribution message.
* <p>
@@ -874,9 +820,7 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
// structure.
if (isShunnedOrNew(m)) {
if (isShunned(m)) {
- if (msg instanceof StopShunningMarker) {
- endShun(m);
- } else {
+ if (!(msg instanceof StopShunningMarker)) {
// fix for bug 41538 - sick alert listener causes deadlock
// due to view latestViewReadWriteLock being held during messaging
shunned = true;
@@ -893,7 +837,7 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
if (shunned) { // bug #41538 - shun notification must be outside synchronization to avoid
// hanging
- warnShun(m);
+ logger.warn("Membership: disregarding shunned member <{}>", m);
if (logger.isTraceEnabled()) {
logger.trace("Membership: Ignoring message from shunned member <{}>:{}", m, msg);
}
@@ -1427,14 +1371,6 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
latestViewWriteLock.unlock();
}
- // Trickiness: there is a minor recursion
- // with addShunnedMembers, since it will
- // attempt to destroy really really old members. Performing the check
- // here breaks the recursion.
- if (!isShunned(member)) {
- addShunnedMember(member);
- }
-
lifecycleListener.destroyMember(member, reason);
}
@@ -1443,33 +1379,22 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
*
* @param m the member in question
*
- * This also checks the time the given member was shunned, and has the side effect of
- * removing the member from the list if it was shunned too far in the past.
- *
* @return true if the given member is a zombie
*/
@Override
public boolean isShunned(ID m) {
- if (!shunnedMembers.containsKey(m)) {
- return false;
- }
-
- // Make sure that the entry isn't stale...
- long shunTime = shunnedMembers.get(m).longValue();
- long now = System.currentTimeMillis();
- if (shunTime + SHUNNED_SUNSET * 1000L > now) {
- return true;
- }
-
- // Oh, it _is_ stale. Remove it while we're here.
- endShun(m);
- return false;
+ final MembershipView<ID> view = latestView;
+ return m.getVmViewId() <= view.getViewId() && !view.contains(m);
}
private boolean isShunnedOrNew(final ID m) {
+ final MembershipView<ID> view = latestView;
+ if (m.getVmViewId() <= view.getViewId() && view.contains(m)) {
+ return false;
+ }
latestViewReadLock.lock();
try {
- return shunnedMembers.containsKey(m) || isNew(m);
+ return isShunned(m) || isNew(m);
} finally { // synchronized
latestViewReadLock.unlock();
}
@@ -1483,11 +1408,9 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
/**
* Indicate whether the given member is in the surprise member list
* <P>
- * Unlike isShunned, this method will not cause expiry of a surprise member. That must be done
+ * This method will not cause expiry of a surprise member. That must be done
* during view processing.
* <p>
- * Like isShunned, this method holds the view lock while executing
- *
* Concurrency: protected by {@link #latestViewReadLock}
*
* @param m the member in question
@@ -1535,73 +1458,6 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
return this.surpriseMemberTimeout;
}
- private boolean endShun(ID m) {
- boolean wasShunned = (shunnedMembers.remove(m) != null);
- shunnedAndWarnedMembers.remove(m);
- return wasShunned;
- }
-
- /**
- * Add the given member to the shunned list. Also, purge any shunned members that are really
- * really old.
- * <p>
- *
- * @param m the member to add
- */
- private void addShunnedMember(ID m) {
- long deathTime = System.currentTimeMillis() - SHUNNED_SUNSET * 1000L;
-
- surpriseMembers.remove(m); // for safety
-
- // Update the shunned set.
- if (!isShunned(m)) {
- shunnedMembers.put(m, Long.valueOf(System.currentTimeMillis()));
- }
-
- // Remove really really old shunned members.
- // First, make a copy of the old set. New arrivals _a priori_ don't matter,
- // and we're going to be updating the list so we don't want to disturb
- // the iterator.
- Set<Map.Entry<ID, Long>> oldMembers = new HashSet<>(shunnedMembers.entrySet());
-
- Set<ID> removedMembers = new HashSet<>();
-
- for (final Map.Entry<ID, Long> oldMember : oldMembers) {
- Entry<ID, Long> e = oldMember;
-
- // Key is the member. Value is the time to remove it.
- long ll = e.getValue().longValue();
- if (ll >= deathTime) {
- continue; // too new.
- }
-
- ID mm = e.getKey();
-
- if (latestView.contains(mm)) {
- // Fault tolerance: a shunned member can conceivably linger but never
- // disconnect.
- //
- // We may not delete it at the time that we shun it because the view
- // isn't necessarily stable. (Note that a well-behaved cache member
- // will depart on its own accord, but we force the issue here.)
- destroyMember(mm, "shunned but never disconnected");
- }
- if (logger.isDebugEnabled()) {
- logger.debug("Membership: finally removed shunned member entry <{}>", mm);
- }
-
- removedMembers.add(mm);
- }
-
- // removed timed-out entries from the shunned-members collections
- if (removedMembers.size() > 0) {
- for (final ID removedMember : removedMembers) {
- ID idm = removedMember;
- endShun(idm);
- }
- }
- }
-
@Override
public void setReconnectCompleted(boolean reconnectCompleted) {
this.reconnectCompleted = reconnectCompleted;
[geode] 01/02: GEODE-7245: Remove most uses of latestViewReadLock
in GMSMembership (#6037)
Posted by bu...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
burcham pushed a commit to branch support/1.14
in repository https://gitbox.apache.org/repos/asf/geode.git
commit ff947f1328f87ec8a8f4a914aea80f3d59eaef59
Author: Kamilla Aslami <ka...@vmware.com>
AuthorDate: Tue Mar 2 11:50:00 2021 -0600
GEODE-7245: Remove most uses of latestViewReadLock in GMSMembership (#6037)
* Remove most uses of latestViewReadLock in GMSMembership
* make MembershipView immutable
* Constructor consolidation and better naming of add/remove
* Tighten constructor chaining
Co-authored-by: Bill Burcham <bi...@gmail.com>
(cherry picked from commit 477ffba8e155dca0bdeeefd312b6b32b4b481100)
---
.../internal/membership/api/MembershipView.java | 147 +++-------
.../internal/membership/gms/GMSMembership.java | 313 ++++++++-------------
2 files changed, 164 insertions(+), 296 deletions(-)
diff --git a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/MembershipView.java b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/MembershipView.java
index 22a7691..902837c 100644
--- a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/MembershipView.java
+++ b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/MembershipView.java
@@ -14,75 +14,53 @@
*/
package org.apache.geode.distributed.internal.membership.api;
+import static java.util.stream.Collectors.toList;
+
import java.util.ArrayList;
-import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
+import java.util.stream.Stream;
/**
- * The MembershipView class represents a membership view. MembershipViews are typically
- * unmodifiable though you an create and manipulate one for local usel A MembershipView
- * defines who is in the cluster and knows which node created the view. It also knows which
- * members left or were removed when the view was created. MemberIdentifiers in the view
- * are marked with the viewId of the MembershipView in which they joined the cluster.
+ * The MembershipView class represents a membership view. A MembershipView defines who is in the
+ * cluster and knows which node created the view. It also knows which members left or were removed
+ * when the view was created. MemberIdentifiers in the view are marked with the viewId of the
+ * MembershipView in which they joined the cluster.
*/
public class MembershipView<ID extends MemberIdentifier> {
- private int viewId;
- private List<ID> members;
- private Set<ID> shutdownMembers;
- private Set<ID> crashedMembers;
- private ID creator;
- private Set<ID> hashedMembers;
- private volatile boolean unmodifiable;
-
+ private final int viewId;
+ private final List<ID> members;
+ private final Set<ID> shutdownMembers;
+ private final Set<ID> crashedMembers;
+ private final ID creator;
+ private final Set<ID> hashedMembers;
public MembershipView() {
- viewId = -1;
- members = new ArrayList<>(0);
- this.hashedMembers = new HashSet<>(members);
- shutdownMembers = Collections.emptySet();
- crashedMembers = new HashSet<>();
- creator = null;
+ this(null, -1, Collections.emptyList());
}
- public MembershipView(ID creator, int viewId,
- List<ID> members) {
- this.viewId = viewId;
- this.members = new ArrayList<>(members);
- hashedMembers = new HashSet<>(this.members);
- shutdownMembers = new HashSet<>();
- crashedMembers = Collections.emptySet();
- this.creator = creator;
- }
-
- /**
- * Create a new view with the contents of the given view and the specified view ID
- */
- public MembershipView(MembershipView<ID> other, int viewId) {
- this.creator = other.creator;
- this.viewId = viewId;
- this.members = new ArrayList<>(other.members);
- this.hashedMembers = new HashSet<>(other.members);
- this.shutdownMembers = new HashSet<>(other.shutdownMembers);
- this.crashedMembers = new HashSet<>(other.crashedMembers);
+ public MembershipView(final ID creator, final int viewId, final List<ID> members) {
+ this(creator, viewId, members, Collections.emptySet(), Collections.emptySet());
}
- public MembershipView(ID creator, int viewId,
- List<ID> mbrs, Set<ID> shutdowns,
- Set<ID> crashes) {
+ public MembershipView(final ID creator, final int viewId, final List<ID> members,
+ final Set<ID> shutdowns, final Set<ID> crashes) {
this.creator = creator;
this.viewId = viewId;
- this.members = mbrs;
- this.hashedMembers = new HashSet<>(mbrs);
- this.shutdownMembers = shutdowns;
- this.crashedMembers = crashes;
- }
- public void makeUnmodifiable() {
- unmodifiable = true;
+ /*
+ * Copy each collection, then store the ref to the unmodifiable
+ * wrapper so we can expose it.
+ */
+ this.members = Collections.unmodifiableList(new ArrayList<>(members));
+ this.shutdownMembers = Collections.unmodifiableSet(new HashSet<>(shutdowns));
+ this.crashedMembers = Collections.unmodifiableSet(new HashSet<>(crashes));
+
+ // make this unmodifiable for good measure (even though we don't expose it)
+ this.hashedMembers = Collections.unmodifiableSet(new HashSet<>(members));
}
public int getViewId() {
@@ -93,55 +71,24 @@ public class MembershipView<ID extends MemberIdentifier> {
return this.creator;
}
- public void setCreator(ID creator) {
- this.creator = creator;
- }
-
- public void setViewId(int viewId) {
- this.viewId = viewId;
- }
-
-
-
public List<ID> getMembers() {
- return Collections.unmodifiableList(this.members);
- }
-
- /**
- * return members that are i this view but not the given old view
- */
- public List<ID> getNewMembers(MembershipView<ID> olderView) {
- List<ID> result = new ArrayList<>(members);
- result.removeAll(olderView.getMembers());
- return result;
+ return members;
}
public Object get(int i) {
return this.members.get(i);
}
- public void add(ID mbr) {
- if (unmodifiable) {
- throw new IllegalStateException("this membership view is not modifiable");
- }
- this.hashedMembers.add(mbr);
- this.members.add(mbr);
+ public MembershipView<ID> createNewViewWithMember(ID member) {
+ return new MembershipView<>(creator, viewId,
+ Stream.concat(members.stream(), Stream.of(member)).collect(toList()), shutdownMembers,
+ crashedMembers);
}
- public boolean remove(ID mbr) {
- if (unmodifiable) {
- throw new IllegalStateException("this membership view is not modifiable");
- }
- this.hashedMembers.remove(mbr);
- return this.members.remove(mbr);
- }
-
- public void removeAll(Collection<ID> ids) {
- if (unmodifiable) {
- throw new IllegalStateException("this membership view is not modifiable");
- }
- this.hashedMembers.removeAll(ids);
- ids.forEach(this::remove);
+ public MembershipView<ID> createNewViewWithoutMember(final ID member) {
+ return new MembershipView<>(creator, viewId,
+ members.stream().filter(m -> !m.equals(member)).collect(toList()), shutdownMembers,
+ crashedMembers);
}
public boolean contains(MemberIdentifier mbr) {
@@ -166,7 +113,7 @@ public class MembershipView<ID extends MemberIdentifier> {
* Returns the ID from this view that is equal to the argument. If no such ID exists the argument
* is returned.
*/
- public synchronized ID getCanonicalID(ID id) {
+ public ID getCanonicalID(ID id) {
if (hashedMembers.contains(id)) {
for (ID m : this.members) {
if (id.equals(m)) {
@@ -178,7 +125,6 @@ public class MembershipView<ID extends MemberIdentifier> {
}
-
public ID getCoordinator() {
for (ID addr : members) {
if (addr.preferredForCoordinator()) {
@@ -191,10 +137,6 @@ public class MembershipView<ID extends MemberIdentifier> {
return null;
}
- public Set<ID> getShutdownMembers() {
- return this.shutdownMembers;
- }
-
public Set<ID> getCrashedMembers() {
return this.crashedMembers;
}
@@ -206,8 +148,9 @@ public class MembershipView<ID extends MemberIdentifier> {
sb.append("View[").append(creator).append('|').append(viewId).append("] members: [");
boolean first = true;
for (ID mbr : this.members) {
- if (!first)
+ if (!first) {
sb.append(", ");
+ }
sb.append(mbr);
if (mbr == lead) {
sb.append("{lead}");
@@ -218,8 +161,9 @@ public class MembershipView<ID extends MemberIdentifier> {
sb.append("] shutdown: [");
first = true;
for (ID mbr : this.shutdownMembers) {
- if (!first)
+ if (!first) {
sb.append(", ");
+ }
sb.append(mbr);
first = false;
}
@@ -228,8 +172,9 @@ public class MembershipView<ID extends MemberIdentifier> {
sb.append("] crashed: [");
first = true;
for (ID mbr : this.crashedMembers) {
- if (!first)
+ if (!first) {
sb.append(", ");
+ }
sb.append(mbr);
first = false;
}
@@ -239,7 +184,7 @@ public class MembershipView<ID extends MemberIdentifier> {
}
@Override
- public synchronized boolean equals(Object other) {
+ public boolean equals(Object other) {
if (other == this) {
return true;
}
@@ -250,7 +195,7 @@ public class MembershipView<ID extends MemberIdentifier> {
}
@Override
- public synchronized int hashCode() {
+ public int hashCode() {
return this.members.hashCode();
}
diff --git a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
index 3d13e1b..3c05988 100644
--- a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
+++ b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
@@ -223,12 +223,13 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
/**
* This is the latest view (ordered list of IDs) that has been installed
*
- * All accesses to this object are protected via {@link #latestViewLock}
+ * Writing to this object is protected via {@link #latestViewWriteLock}
*/
private volatile MembershipView<ID> latestView = new MembershipView<>();
/**
- * This is the lock for protecting access to latestView
+ * This is the lock for protecting access to latestView and modification of data used in
+ * {@link #processView} method
*
* @see #latestView
*/
@@ -263,27 +264,36 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
*
* Members are removed after {@link #SHUNNED_SUNSET} seconds have passed.
*
- * Accesses to this list needs to be under the read or write lock of {@link #latestViewLock}
+ * Writing to this list needs to be under {@link #latestViewWriteLock}
*
* @see System#currentTimeMillis()
*/
- // protected final Set shunnedMembers = Collections.synchronizedSet(new HashSet());
private final Map<ID, Long> shunnedMembers = new ConcurrentHashMap<>();
/**
* 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.
*/
- private final Map<ID, Object> shutdownMembers = new BoundedLinkedHashMap<>();
+ private final Set<ID> shutdownMembers = Collections.newSetFromMap(new BoundedLinkedHashMap<>());
+
+ /**
+ * This is the lock for protecting access to shutdownMembers
+ *
+ * @see #shutdownMembers
+ */
+ private final ReadWriteLock shutdownMembersLock = new ReentrantReadWriteLock();
+ private final Lock shutdownMembersReadLock = shutdownMembersLock.readLock();
+ private final Lock shutdownMembersWriteLock = shutdownMembersLock.writeLock();
/**
* per bug 39552, keep a list of members that have been shunned and for which a message is
* printed. Contents of this list are cleared at the same time they are removed from
* {@link #shunnedMembers}.
*
- * Accesses to this list needs to be under the read or write lock of {@link #latestViewLock}
+ * Writing to this list needs to be under {@link #latestViewWriteLock}
*/
- private final HashSet<ID> shunnedAndWarnedMembers = new HashSet<>();
+ private final Set<ID> shunnedAndWarnedMembers =
+ Collections.newSetFromMap(new ConcurrentHashMap<>());
/**
* The identities and birth-times of others that we have allowed into membership at the
* distributed system level, but have not yet appeared in a view.
@@ -295,7 +305,7 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
* {@link #surpriseMemberTimeout} milliseconds have passed, a view containing the member has not
* arrived, the member is removed from membership and member-left notification is performed.
* <p>
- * > Accesses to this list needs to be under the read or write lock of {@link #latestViewLock}
+ * > Writing to this list needs to be under {@link #latestViewWriteLock}
*
* @see System#currentTimeMillis()
*/
@@ -359,7 +369,7 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
/**
* Analyze a given view object, generate events as appropriate
*/
- public void processView(MembershipView<ID> newView) {
+ public void processView(final MembershipView<ID> newView) {
// Sanity check...
if (logger.isDebugEnabled()) {
StringBuilder msg = new StringBuilder(200);
@@ -388,7 +398,7 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
// update the view to reflect our changes, so that
// callbacks will see the new (updated) view.
- MembershipView<ID> newlatestView = new MembershipView<>(newView, newView.getViewId());
+ MembershipView<ID> newlatestView = newView;
// look for additions
for (int i = 0; i < newView.getMembers().size(); i++) { // additions
@@ -488,7 +498,7 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
"not seen in membership view in " + this.surpriseMemberTimeout + "ms");
} else {
if (!newlatestView.contains(entry.getKey())) {
- newlatestView.add(entry.getKey());
+ newlatestView = newlatestView.createNewViewWithMember(entry.getKey());
}
}
}
@@ -505,7 +515,6 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
}
// the view is complete - let's install it
- newlatestView.makeUnmodifiable();
latestView = newlatestView;
listener.viewInstalled(latestView);
} finally {
@@ -535,21 +544,14 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
}
}
- public boolean isCleanupTimerStarted() {
- return this.cleanupTimer != null;
- }
-
/**
* the timer used to perform periodic tasks
- *
- * Concurrency: protected by {@link #latestViewLock} ReentrantReadWriteLock
*/
private ScheduledExecutorService cleanupTimer;
private Services<ID> services;
-
/**
* Joins the distributed system
*
@@ -567,9 +569,7 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
// connect
services.getJoinLeave().join();
- MembershipView<ID> initialView = createGeodeView(services.getJoinLeave().getView());
- latestView = new MembershipView<>(initialView, initialView.getViewId());
- latestView.makeUnmodifiable();
+ latestView = createGeodeView(services.getJoinLeave().getView());
listener.viewInstalled(latestView);
} finally {
this.isJoining = false;
@@ -580,28 +580,9 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
}
private MembershipView<ID> createGeodeView(GMSMembershipView<ID> view) {
- MembershipView<ID> result =
- createGeodeView(view.getCreator(), view.getViewId(), view.getMembers(),
- view.getShutdownMembers(),
- view.getCrashedMembers());
- result.makeUnmodifiable();
- return result;
- }
-
- private MembershipView<ID> createGeodeView(ID gmsCreator, int viewId,
- List<ID> gmsMembers,
- Set<ID> gmsShutdowns, Set<ID> gmsCrashes) {
- ID geodeCreator = gmsCreator;
- List<ID> geodeMembers = new ArrayList<>(gmsMembers.size());
- for (ID member : gmsMembers) {
- geodeMembers.add(member);
- }
- Set<ID> geodeShutdownMembers =
- gmsMemberCollectionToIDSet(gmsShutdowns);
- Set<ID> geodeCrashedMembers =
- gmsMemberCollectionToIDSet(gmsCrashes);
- return new MembershipView<>(geodeCreator, viewId, geodeMembers, geodeShutdownMembers,
- geodeCrashedMembers);
+ return new MembershipView<>(view.getCreator(), view.getViewId(), view.getMembers(),
+ view.getShutdownMembers(),
+ view.getCrashedMembers());
}
private Set<ID> gmsMemberCollectionToIDSet(
@@ -664,7 +645,8 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
}
/**
- * Remove a member. {@link #latestViewLock} must be held before this method is called. If member
+ * Remove a member. {@link #latestViewWriteLock} must be held before this method is called. If
+ * member
* is not already shunned, the uplevel event handler is invoked.
*/
private void removeWithViewLock(ID dm, boolean crashed, String reason) {
@@ -677,13 +659,22 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
return; // Explicit deletion, no upcall.
}
- if (!shutdownMembers.containsKey(dm)) {
+ if (!isMemberShuttingDown(dm)) {
// if we've received a shutdown message then DistributionManager will already have
// notified listeners
listener.memberDeparted(dm, crashed, reason);
}
}
+ private boolean isMemberShuttingDown(ID dm) {
+ shutdownMembersReadLock.lock();
+ try {
+ return shutdownMembers.contains(dm);
+ } finally {
+ shutdownMembersReadLock.unlock();
+ }
+ }
+
/**
* Process a surprise connect event, or place it on the startup queue.
*
@@ -714,8 +705,8 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
* Logic for handling a direct connection event (message received from a member not in the view).
* Does not employ the startup queue.
* <p>
- * Must be called with {@link #latestViewLock} held. Waits until there is a stable view. If the
- * member has already been added, simply returns; else adds the member.
+ * Waits until there is a stable view. If the member has already been added, simply returns;
+ * else adds the member.
*
* @param dm the member joining
*/
@@ -786,10 +777,7 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
// should ensure it is not chosen as an elder.
// This will get corrected when the member finally shows up in the
// view.
- MembershipView<ID> newMembers = new MembershipView<>(latestView, latestView.getViewId());
- newMembers.add(member);
- newMembers.makeUnmodifiable();
- latestView = newMembers;
+ latestView = latestView.createNewViewWithMember(member);
}
} finally {
latestViewWriteLock.unlock();
@@ -858,18 +846,13 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
@Override
public void warnShun(ID m) {
- latestViewWriteLock.lock();
- try {
- if (!shunnedMembers.containsKey(m)) {
- return; // not shunned
- }
- if (shunnedAndWarnedMembers.contains(m)) {
- return; // already warned
- }
- shunnedAndWarnedMembers.add(m);
- } finally {
- latestViewWriteLock.unlock();
+ if (!shunnedMembers.containsKey(m)) {
+ return; // not shunned
+ }
+ if (shunnedAndWarnedMembers.contains(m)) {
+ return; // already warned
}
+ shunnedAndWarnedMembers.add(m);
// issue warning outside of sync since it may cause messaging and we don't
// want to hold the view lock while doing that
logger.warn("Membership: disregarding shunned member <{}>", m);
@@ -890,26 +873,21 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
// If this member is shunned or new, grab the latestViewWriteLock: update the appropriate data
// structure.
if (isShunnedOrNew(m)) {
- latestViewWriteLock.lock();
- try {
- if (isShunned(m)) {
- if (msg instanceof StopShunningMarker) {
- endShun(m);
- } else {
- // fix for bug 41538 - sick alert listener causes deadlock
- // due to view latestViewReadWriteLock being held during messaging
- shunned = true;
- }
+ if (isShunned(m)) {
+ if (msg instanceof StopShunningMarker) {
+ endShun(m);
+ } else {
+ // fix for bug 41538 - sick alert listener causes deadlock
+ // due to view latestViewReadWriteLock being held during messaging
+ shunned = true;
}
+ }
- if (!shunned) {
- // If it's a new sender, wait our turn, generate the event
- if (isNew(m)) {
- shunned = !addSurpriseMember(m);
- }
+ if (!shunned) {
+ // If it's a new sender, wait our turn, generate the event
+ if (isNew(m)) {
+ shunned = !addSurpriseMember(m);
}
- } finally {
- latestViewWriteLock.unlock();
}
}
@@ -965,22 +943,16 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
return;
}
}
- latestViewWriteLock.lock();
- try {
- if (!processingEvents) {
- synchronized (startupLock) {
- if (!startupMessagesDrained) {
- startupMessages.add(new StartupEvent<>(viewArg));
- return;
- }
+ if (!processingEvents) {
+ synchronized (startupLock) {
+ if (!startupMessagesDrained) {
+ startupMessages.add(new StartupEvent<>(viewArg));
+ return;
}
}
-
- viewExecutor.submit(() -> processView(viewArg));
-
- } finally {
- latestViewWriteLock.unlock();
}
+
+ viewExecutor.submit(() -> processView(viewArg));
}
private ID gmsMemberToDMember(ID gmsMember) {
@@ -993,23 +965,17 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
* @param suspectInfo the suspectee and suspector
*/
protected void handleOrDeferSuspect(SuspectMember<ID> suspectInfo) {
- latestViewWriteLock.lock();
- try {
- if (!processingEvents) {
- return;
- }
- ID suspect = gmsMemberToDMember(suspectInfo.suspectedMember);
- ID who = gmsMemberToDMember(suspectInfo.whoSuspected);
- this.suspectedMembers.put(suspect, Long.valueOf(System.currentTimeMillis()));
- listener.memberSuspect(suspect, who, suspectInfo.reason);
- } finally {
- latestViewWriteLock.unlock();
+ if (!processingEvents) {
+ return;
}
+ ID suspect = gmsMemberToDMember(suspectInfo.suspectedMember);
+ ID who = gmsMemberToDMember(suspectInfo.whoSuspected);
+ this.suspectedMembers.put(suspect, System.currentTimeMillis());
+ listener.memberSuspect(suspect, who, suspectInfo.reason);
}
/**
- * Process a potential direct connect. Does not use the startup queue. It grabs the
- * {@link #latestViewLock} and then processes the event.
+ * Process a potential direct connect. Does not use the startup queue.
* <p>
* It is a <em>potential</em> event, because we don't know until we've grabbed a stable view if
* this is really a new member.
@@ -1145,10 +1111,7 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
*/
@Override
public MembershipView<ID> getView() {
- // Grab the latest view under a mutex...
- MembershipView<ID> v = latestView;
- MembershipView<ID> result = new MembershipView<>(v, v.getViewId());
- return result;
+ return latestView;
}
public boolean isJoining() {
@@ -1162,20 +1125,13 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
*/
@Override
public ID getCoordinator() {
- latestViewReadLock.lock();
- try {
- return latestView == null ? null : latestView.getCoordinator();
- } finally {
- latestViewReadLock.unlock();
- }
+ final MembershipView<ID> view = latestView;
+ return view == null ? null : view.getCoordinator();
}
@Override
public boolean memberExists(ID m) {
- latestViewReadLock.lock();
- MembershipView<ID> v = latestView;
- latestViewReadLock.unlock();
- return v.contains(m);
+ return latestView.contains(m);
}
/**
@@ -1231,23 +1187,24 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
if (logger.isDebugEnabled()) {
logger.debug("Membership: recording shutdown status of {}", id);
}
- synchronized (this.shutdownMembers) {
- this.shutdownMembers.put(id, id);
- services.getHealthMonitor()
- .memberShutdown(id, reason);
+ shutdownMembersWriteLock.lock();
+ try {
+ this.shutdownMembers.add(id);
services.getJoinLeave().memberShutdown(id, reason);
+ } finally {
+ shutdownMembersWriteLock.unlock();
}
}
@Override
public Set<ID> getMembersNotShuttingDown() {
- latestViewReadLock.lock();
+ shutdownMembersReadLock.lock();
try {
- return latestView.getMembers().stream().filter(id -> !shutdownMembers.containsKey(id))
+ return latestView.getMembers().stream().filter(id -> !shutdownMembers.contains(id))
.collect(
Collectors.toSet());
} finally {
- latestViewReadLock.unlock();
+ shutdownMembersReadLock.unlock();
}
}
@@ -1335,7 +1292,7 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
@Override
public void suspectMember(ID mbr, String reason) {
- if (!this.shutdownInProgress && !this.shutdownMembers.containsKey(mbr)) {
+ if (!this.shutdownInProgress && !isMemberShuttingDown(mbr)) {
verifyMember(mbr, reason);
}
}
@@ -1360,13 +1317,8 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
@Override
public ID[] getAllMembers(final ID[] arrayType) {
- latestViewReadLock.lock();
- try {
- List<ID> keySet = latestView.getMembers();
- return keySet.toArray(arrayType);
- } finally {
- latestViewReadLock.unlock();
- }
+ final List<ID> keySet = latestView.getMembers();
+ return keySet.toArray(arrayType);
}
@Override
@@ -1448,6 +1400,10 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
}
}
+ /**
+ * This method holds the {@link #latestViewWriteLock} to protect
+ * {@link #shutdownInProgress} from modification while {@link #processView} is in progress.
+ */
@Override
public void setShutdown() {
latestViewWriteLock.lock();
@@ -1457,8 +1413,6 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
/**
* Clean up and create consistent new view with member removed. No uplevel events are generated.
- *
- * Must be called with the {@link #latestViewLock} held.
*/
private void destroyMember(final ID member, final String reason) {
@@ -1466,17 +1420,13 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
latestViewWriteLock.lock();
try {
if (latestView.contains(member)) {
- MembershipView<ID> newView = new MembershipView<>(latestView, latestView.getViewId());
- newView.remove(member);
- newView.makeUnmodifiable();
- latestView = newView;
+ latestView = latestView.createNewViewWithoutMember(member);
}
+ surpriseMembers.remove(member);
} finally {
latestViewWriteLock.unlock();
}
- surpriseMembers.remove(member);
-
// Trickiness: there is a minor recursion
// with addShunnedMembers, since it will
// attempt to destroy really really old members. Performing the check
@@ -1496,8 +1446,6 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
* This also checks the time the given member was shunned, and has the side effect of
* removing the member from the list if it was shunned too far in the past.
*
- * Concurrency: protected by {@link #latestViewLock} ReentrantReadWriteLock
- *
* @return true if the given member is a zombie
*/
@Override
@@ -1506,21 +1454,16 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
return false;
}
- latestViewWriteLock.lock();
- try {
- // Make sure that the entry isn't stale...
- long shunTime = shunnedMembers.get(m).longValue();
- long now = System.currentTimeMillis();
- if (shunTime + SHUNNED_SUNSET * 1000L > now) {
- return true;
- }
-
- // Oh, it _is_ stale. Remove it while we're here.
- endShun(m);
- return false;
- } finally {
- latestViewWriteLock.unlock();
+ // Make sure that the entry isn't stale...
+ long shunTime = shunnedMembers.get(m).longValue();
+ long now = System.currentTimeMillis();
+ if (shunTime + SHUNNED_SUNSET * 1000L > now) {
+ return true;
}
+
+ // Oh, it _is_ stale. Remove it while we're here.
+ endShun(m);
+ return false;
}
private boolean isShunnedOrNew(final ID m) {
@@ -1545,7 +1488,7 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
* <p>
* Like isShunned, this method holds the view lock while executing
*
- * Concurrency: protected by {@link #latestViewLock} ReentrantReadWriteLock
+ * Concurrency: protected by {@link #latestViewReadLock}
*
* @param m the member in question
* @return true if the given member is a surprise member
@@ -1555,8 +1498,8 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
latestViewReadLock.lock();
try {
if (surpriseMembers.containsKey(m)) {
- long birthTime = surpriseMembers.get(m).longValue();
- long now = System.currentTimeMillis();
+ final long birthTime = surpriseMembers.get(m);
+ final long now = System.currentTimeMillis();
return (birthTime >= (now - this.surpriseMemberTimeout));
}
return false;
@@ -1602,7 +1545,6 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
* Add the given member to the shunned list. Also, purge any shunned members that are really
* really old.
* <p>
- * Must be called with {@link #latestViewLock} held and the view stable.
*
* @param m the member to add
*/
@@ -1687,25 +1629,22 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
return shutdownInProgress;
}
- // TODO GEODE-1752 rewrite this to get rid of the latches, which are currently a memory leak
@Override
public boolean waitForNewMember(ID remoteId) {
boolean foundRemoteId = false;
CountDownLatch currentLatch = null;
- // ARB: preconditions
- // remoteId != null
+ // latestViewWriteLock protects memberLatch from modification while processView is in progress
latestViewWriteLock.lock();
try {
- if (latestView == null) {
- // Not sure how this would happen, but see bug 38460.
- // No view?? Not found!
- } else if (latestView.contains(remoteId)) {
- // ARB: check if remoteId is already in membership view.
- // If not, then create a latch if needed and wait for the latch to open.
- foundRemoteId = true;
- } else if ((currentLatch = this.memberLatch.get(remoteId)) == null) {
- currentLatch = new CountDownLatch(1);
- this.memberLatch.put(remoteId, currentLatch);
+ if (latestView != null) {
+ if (latestView.contains(remoteId)) {
+ // check if remoteId is already in membership view.
+ // If not, then create a latch if needed and wait for the latch to open.
+ foundRemoteId = true;
+ } else if ((currentLatch = this.memberLatch.get(remoteId)) == null) {
+ currentLatch = new CountDownLatch(1);
+ this.memberLatch.put(remoteId, currentLatch);
+ }
}
} finally {
latestViewWriteLock.unlock();
@@ -1716,18 +1655,13 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
if (currentLatch != null
&& currentLatch.await(membershipCheckTimeout, TimeUnit.MILLISECONDS)) {
foundRemoteId = true;
- // @todo
- // ARB: remove latch from memberLatch map if this is the last thread waiting on latch.
}
} catch (InterruptedException ex) {
- // ARB: latch attempt was interrupted.
+ // latch attempt was interrupted.
Thread.currentThread().interrupt();
logger.warn("The membership check was terminated with an exception.");
}
}
-
- // ARB: postconditions
- // (foundRemoteId == true) ==> (currentLatch is non-null ==> currentLatch is open)
return foundRemoteId;
}
@@ -1889,29 +1823,18 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
/* Service interface */
@Override
- public void started() throws MemberStartupException {
+ public void started() {
startCleanupTimer();
}
/* Service interface */
@Override
public void stop() {
- // [bruce] Do not null out the channel w/o adding appropriate synchronization
-
logger.debug("Membership closing");
if (lifecycleListener.disconnect(null)) {
-
if (address != null) {
- // Make sure that channel information is consistent
- // Probably not important in this particular case, but just
- // to be consistent...
- latestViewWriteLock.lock();
- try {
- destroyMember(address, "orderly shutdown");
- } finally {
- latestViewWriteLock.unlock();
- }
+ destroyMember(address, "orderly shutdown");
}
}