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 2020/08/12 23:46:04 UTC
[geode] 03/04: GEODE-8298: Fix multicast version detection (#5370)
This is an automated email from the ASF dual-hosted git repository.
burcham pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git
commit b5e2b566c4574bb41db4ed588fd79674f084e813
Author: Kamilla Aslami <ka...@vmware.com>
AuthorDate: Mon Jul 20 08:30:37 2020 -0700
GEODE-8298: Fix multicast version detection (#5370)
Membership decides whether or not use multicast based on the versions
of view members and surprise members. Surprise member version comparison
was backward. Correct version comparison by unifying view and surprise
member processing.
Co-authored-by: Kamilla Aslami <ka...@gmail.com>
Co-authored-by: Bill Burcham <bi...@gmail.com>
(cherry picked from commit fd76cc0b7dbf97dfb84d11e67b37e33c0a9e7fb2)
---
.../membership/InternalDistributedMember.java | 2 +-
.../gms/GMSMemberDataVersionJUnitTest.java | 7 --
.../membership/gms/GMSMembershipJUnitTest.java | 120 ++++++++++++++++++---
.../internal/membership/api/MemberData.java | 5 +-
.../internal/membership/api/MemberIdentifier.java | 3 +-
.../internal/membership/gms/GMSMemberData.java | 11 +-
.../internal/membership/gms/GMSMembership.java | 42 ++++----
.../membership/gms/MemberIdentifierImpl.java | 2 +-
8 files changed, 129 insertions(+), 63 deletions(-)
diff --git a/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java b/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java
index 70d6979..3e7db6d 100755
--- a/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java
+++ b/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java
@@ -545,7 +545,7 @@ public class InternalDistributedMember
return memberIdentifier.getUniqueId();
}
- public void setVersionForTest(KnownVersion v) {
+ public void setVersionForTest(Version v) {
memberIdentifier.setVersionForTest(v);
}
diff --git a/geode-membership/src/integrationTest/java/org/apache/geode/distributed/internal/membership/gms/GMSMemberDataVersionJUnitTest.java b/geode-membership/src/integrationTest/java/org/apache/geode/distributed/internal/membership/gms/GMSMemberDataVersionJUnitTest.java
index 7a6895b..af27e49 100644
--- a/geode-membership/src/integrationTest/java/org/apache/geode/distributed/internal/membership/gms/GMSMemberDataVersionJUnitTest.java
+++ b/geode-membership/src/integrationTest/java/org/apache/geode/distributed/internal/membership/gms/GMSMemberDataVersionJUnitTest.java
@@ -85,13 +85,6 @@ public class GMSMemberDataVersionJUnitTest {
validate(newMember);
}
- @Test
- public void testSetVersionOrdinal() {
- final GMSMemberData memberData = new GMSMemberData();
- memberData.setVersionOrdinal(unknownVersionOrdinal);
- validate(memberData);
- }
-
private AbstractShortAssert<?> validate(final MemberData memberData) {
return assertThat(memberData.getVersionOrdinal()).isEqualTo(unknownVersionOrdinal);
}
diff --git a/geode-membership/src/integrationTest/java/org/apache/geode/distributed/internal/membership/gms/GMSMembershipJUnitTest.java b/geode-membership/src/integrationTest/java/org/apache/geode/distributed/internal/membership/gms/GMSMembershipJUnitTest.java
index 49b8b3a..e7a2edd 100644
--- a/geode-membership/src/integrationTest/java/org/apache/geode/distributed/internal/membership/gms/GMSMembershipJUnitTest.java
+++ b/geode-membership/src/integrationTest/java/org/apache/geode/distributed/internal/membership/gms/GMSMembershipJUnitTest.java
@@ -59,12 +59,21 @@ import org.apache.geode.distributed.internal.membership.gms.interfaces.HealthMon
import org.apache.geode.distributed.internal.membership.gms.interfaces.JoinLeave;
import org.apache.geode.distributed.internal.membership.gms.interfaces.Messenger;
import org.apache.geode.internal.serialization.DSFIDSerializer;
+import org.apache.geode.internal.serialization.KnownVersion;
+import org.apache.geode.internal.serialization.Version;
+import org.apache.geode.internal.serialization.Versioning;
import org.apache.geode.internal.serialization.internal.DSFIDSerializerImpl;
import org.apache.geode.test.junit.categories.MembershipTest;
@Category({MembershipTest.class})
public class GMSMembershipJUnitTest {
+ private static final Version OLDER_THAN_CURRENT_VERSION =
+ Versioning.getVersion((short) (KnownVersion.CURRENT_ORDINAL - 1));
+ private static final Version NEWER_THAN_CURRENT_VERSION =
+ Versioning.getVersion((short) (KnownVersion.CURRENT_ORDINAL + 1));;
+ private static final int DEFAULT_PORT = 8888;
+
private Services services;
private MembershipConfig mockConfig;
private Authenticator authenticator;
@@ -139,7 +148,7 @@ public class GMSMembershipJUnitTest {
Random r = new Random();
mockMembers = new MemberIdentifier[5];
for (int i = 0; i < mockMembers.length; i++) {
- mockMembers[i] = createMemberID(8888 + i);
+ mockMembers[i] = createMemberID(DEFAULT_PORT + i);
uuid = new UUID(r.nextLong(), r.nextLong());
mockMembers[i].setUUID(uuid);
}
@@ -175,7 +184,7 @@ public class GMSMembershipJUnitTest {
MemberIdentifier myGMSMemberId = myMemberId;
List<MemberIdentifier> gmsMembers =
members.stream().map(x -> ((MemberIdentifier) x)).collect(Collectors.toList());
- manager.getGMSManager().installView(new GMSMembershipView(myGMSMemberId, 1, gmsMembers));
+ manager.getGMSManager().installView(new GMSMembershipView<>(myGMSMemberId, 1, gmsMembers));
MemberIdentifier[] destinations = new MemberIdentifier[] {mockMembers[0]};
Set<MemberIdentifier> failures =
manager.send(destinations, m);
@@ -199,9 +208,9 @@ public class GMSMembershipJUnitTest {
manager.getGMSManager().started();
manager.isJoining = true;
- List<MemberIdentifier> viewmembers =
+ List<MemberIdentifier> viewMembers =
Arrays.asList(new MemberIdentifier[] {mockMembers[0], myMemberId});
- manager.getGMSManager().installView(createView(myMemberId, 2, viewmembers));
+ manager.getGMSManager().installView(createView(myMemberId, 2, viewMembers));
// add a surprise member that will be shunned due to it's having
// an old view ID
@@ -219,7 +228,7 @@ public class GMSMembershipJUnitTest {
// suspect a member
MemberIdentifier suspectMember = mockMembers[1];
manager.handleOrDeferSuspect(
- new SuspectMember(mockMembers[0], suspectMember, "testing"));
+ new SuspectMember<>(mockMembers[0], suspectMember, "testing"));
// suspect messages aren't queued - they're ignored before joining the system
assertEquals(2, manager.getStartupEvents().size());
verify(listener, never()).memberSuspect(suspectMember, mockMembers[0], "testing");
@@ -232,9 +241,9 @@ public class GMSMembershipJUnitTest {
assertEquals(3, manager.getStartupEvents().size());
// this view officially adds surpriseMember2
- viewmembers = Arrays
+ viewMembers = Arrays
.asList(new MemberIdentifier[] {mockMembers[0], myMemberId, surpriseMember2});
- manager.handleOrDeferViewEvent(new MembershipView(myMemberId, 3, viewmembers));
+ manager.handleOrDeferViewEvent(new MembershipView<>(myMemberId, 3, viewMembers));
assertEquals(4, manager.getStartupEvents().size());
// add a surprise member that will be shunned due to it's having
@@ -247,13 +256,13 @@ public class GMSMembershipJUnitTest {
// process a new view after we finish joining but before event processing has started
manager.isJoining = false;
mockMembers[4].setVmViewId(4);
- viewmembers = Arrays.asList(new MemberIdentifier[] {mockMembers[0], myMemberId,
+ viewMembers = Arrays.asList(new MemberIdentifier[] {mockMembers[0], myMemberId,
surpriseMember2, mockMembers[4]});
- manager.handleOrDeferViewEvent(new MembershipView(myMemberId, 4, viewmembers));
+ manager.handleOrDeferViewEvent(new MembershipView<>(myMemberId, 4, viewMembers));
assertEquals(6, manager.getStartupEvents().size());
// exercise the toString methods for code coverage
- for (StartupEvent ev : manager.getStartupEvents()) {
+ for (StartupEvent<MemberIdentifier> ev : manager.getStartupEvents()) {
ev.toString();
}
@@ -271,14 +280,14 @@ public class GMSMembershipJUnitTest {
// for code coverage also install a view after we finish joining but before
// event processing has started. This should notify the distribution manager
// with a LocalViewMessage to process the view
- manager.handleOrDeferViewEvent(new MembershipView(myMemberId, 5, viewmembers));
+ manager.handleOrDeferViewEvent(new MembershipView<>(myMemberId, 5, viewMembers));
await().untilAsserted(() -> assertEquals(manager.getView().getViewId(), 5));
// process a suspect now - it will be passed to the listener
reset(listener);
suspectMember = mockMembers[1];
manager.handleOrDeferSuspect(
- new SuspectMember(mockMembers[0], suspectMember, "testing"));
+ new SuspectMember<>(mockMembers[0], suspectMember, "testing"));
verify(listener).memberSuspect(suspectMember, mockMembers[0], "testing");
}
@@ -292,15 +301,15 @@ public class GMSMembershipJUnitTest {
manager.getGMSManager().started();
manager.isJoining = true;
- List<MemberIdentifier> viewmembers =
+ List<MemberIdentifier> viewMembers =
Arrays.asList(new MemberIdentifier[] {mockMembers[0], mockMembers[1], myMemberId});
- GMSMembershipView view = createView(myMemberId, 2, viewmembers);
+ GMSMembershipView view = createView(myMemberId, 2, viewMembers);
manager.getGMSManager().installView(view);
when(services.getJoinLeave().getView()).thenReturn(view);
- MemberIdentifier[] destinations = new MemberIdentifier[viewmembers.size()];
+ MemberIdentifier[] destinations = new MemberIdentifier[viewMembers.size()];
for (int i = 0; i < destinations.length; i++) {
- MemberIdentifier id = viewmembers.get(i);
+ MemberIdentifier id = viewMembers.get(i);
destinations[i] = createMemberID(id.getMembershipPort());
}
manager.checkAddressesForUUIDs(destinations);
@@ -328,4 +337,83 @@ public class GMSMembershipJUnitTest {
assertThat(spy.getStartupEvents()).isEmpty();
}
+ @Test
+ public void testIsMulticastAllowedWithOldVersionSurpriseMember() {
+ MembershipView<MemberIdentifier> view = createMembershipView();
+ manager.addSurpriseMember(createSurpriseMember(OLDER_THAN_CURRENT_VERSION));
+
+ manager.processView(view);
+
+ assertThat(manager.getGMSManager().isMulticastAllowed()).isFalse();
+ }
+
+ @Test
+ public void testIsMulticastAllowedWithCurrentVersionSurpriseMember() {
+ MembershipView<MemberIdentifier> view = createMembershipView();
+ manager.addSurpriseMember(createSurpriseMember(KnownVersion.CURRENT));
+
+ manager.processView(view);
+
+ assertThat(manager.getGMSManager().isMulticastAllowed()).isTrue();
+ }
+
+ @Test
+ public void testIsMulticastAllowedWithNewVersionSurpriseMember() {
+ MembershipView<MemberIdentifier> view = createMembershipView();
+ manager.addSurpriseMember(createSurpriseMember(NEWER_THAN_CURRENT_VERSION));
+
+ manager.processView(view);
+
+ assertThat(manager.getGMSManager().isMulticastAllowed()).isTrue();
+ }
+
+ @Test
+ public void testIsMulticastAllowedWithOldVersionViewMember() {
+ MembershipView<MemberIdentifier> view = createMembershipView();
+ view.getMembers().get(0).setVersionForTest(OLDER_THAN_CURRENT_VERSION);
+
+ manager.processView(view);
+
+ assertThat(manager.getGMSManager().isMulticastAllowed()).isFalse();
+ }
+
+ @Test
+ public void testMulticastAllowedWithCurrentVersionViewMember() {
+ MembershipView<MemberIdentifier> view = createMembershipView();
+
+ manager.processView(view);
+
+ assertThat(manager.getGMSManager().isMulticastAllowed()).isTrue();
+ }
+
+ @Test
+ public void testMulticastAllowedWithNewVersionViewMember() {
+ MembershipView<MemberIdentifier> view = createMembershipView();
+ view.getMembers().get(0).setVersionForTest(NEWER_THAN_CURRENT_VERSION);
+
+ manager.processView(view);
+
+ assertThat(manager.getGMSManager().isMulticastAllowed()).isTrue();
+ }
+
+ private MemberIdentifier createSurpriseMember(Version version) {
+ MemberIdentifier surpriseMember = createMemberID(DEFAULT_PORT + 5);
+ surpriseMember.setVmViewId(3);
+ surpriseMember.setVersionForTest(version);
+ return surpriseMember;
+ }
+
+ private MembershipView<MemberIdentifier> createMembershipView() {
+ List<MemberIdentifier> viewMembers = createMemberIdentifiers();
+ return new MembershipView<>(myMemberId, 2, viewMembers);
+ }
+
+ private List<MemberIdentifier> createMemberIdentifiers() {
+ List<MemberIdentifier> viewMembers = new ArrayList<>();
+ for (int i = 0; i < 2; ++i) {
+ MemberIdentifier memberIdentifier = createMemberID(DEFAULT_PORT + 6 + i);
+ viewMembers.add(memberIdentifier);
+ }
+ return viewMembers;
+ }
}
diff --git a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/MemberData.java b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/MemberData.java
index 3c44b39..a8d038e 100644
--- a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/MemberData.java
+++ b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/MemberData.java
@@ -21,7 +21,6 @@ import java.net.InetAddress;
import org.jgroups.util.UUID;
import org.apache.geode.internal.serialization.DeserializationContext;
-import org.apache.geode.internal.serialization.KnownVersion;
import org.apache.geode.internal.serialization.SerializationContext;
import org.apache.geode.internal.serialization.Version;
@@ -50,8 +49,6 @@ public interface MemberData {
String getUniqueTag();
- void setVersionOrdinal(short versionOrdinal);
-
void setUUID(UUID u);
UUID getUUID();
@@ -92,7 +89,7 @@ public interface MemberData {
void setVmKind(int vmKind);
- void setVersion(KnownVersion v);
+ void setVersion(Version v);
void setDirectChannelPort(int directPort);
diff --git a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/MemberIdentifier.java b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/MemberIdentifier.java
index 30cc8dd..6b4546c 100644
--- a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/MemberIdentifier.java
+++ b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/MemberIdentifier.java
@@ -27,7 +27,6 @@ import org.jgroups.util.UUID;
import org.apache.geode.internal.serialization.DataSerializableFixedID;
import org.apache.geode.internal.serialization.DeserializationContext;
-import org.apache.geode.internal.serialization.KnownVersion;
import org.apache.geode.internal.serialization.SerializationContext;
import org.apache.geode.internal.serialization.Version;
@@ -191,7 +190,7 @@ public interface MemberIdentifier extends DataSerializableFixedID {
String getUniqueId();
- void setVersionForTest(KnownVersion v);
+ void setVersionForTest(Version v);
void setUniqueTag(String tag);
diff --git a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMemberData.java b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMemberData.java
index 76195d4..9f0ba77 100644
--- a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMemberData.java
+++ b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMemberData.java
@@ -236,11 +236,6 @@ public class GMSMemberData implements MemberData, Comparable<GMSMemberData> {
}
@Override
- public void setVersionOrdinal(short versionOrdinal) {
- this.version = Versioning.getVersion(versionOrdinal);
- }
-
- @Override
public void setUUID(UUID u) {
if (u == null) {
this.uuidLSBs = 0;
@@ -507,8 +502,8 @@ public class GMSMemberData implements MemberData, Comparable<GMSMemberData> {
@Override
- public void setVersion(KnownVersion v) {
- setVersionOrdinal(v.ordinal());
+ public void setVersion(Version version) {
+ this.version = version;
}
@Override
@@ -583,7 +578,7 @@ public class GMSMemberData implements MemberData, Comparable<GMSMemberData> {
@Override
public void readEssentialData(DataInput in,
DeserializationContext context) throws IOException, ClassNotFoundException {
- setVersionOrdinal(VersioningIO.readOrdinal(in));
+ setVersion(Versioning.getVersion(VersioningIO.readOrdinal(in)));
int flags = in.readShort();
this.networkPartitionDetectionEnabled = (flags & NPD_ENABLED_BIT) != 0;
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 33c3104..b4c77c2 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
@@ -39,6 +39,7 @@ import java.util.concurrent.locks.ReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.function.Supplier;
import java.util.stream.Collectors;
+import java.util.stream.Stream;
import org.apache.logging.log4j.Logger;
@@ -61,7 +62,6 @@ import org.apache.geode.distributed.internal.membership.api.QuorumChecker;
import org.apache.geode.distributed.internal.membership.api.StopShunningMarker;
import org.apache.geode.distributed.internal.membership.gms.interfaces.Manager;
import org.apache.geode.internal.serialization.KnownVersion;
-import org.apache.geode.internal.serialization.Version;
import org.apache.geode.logging.internal.executors.LoggingExecutors;
import org.apache.geode.logging.internal.executors.LoggingThread;
import org.apache.geode.util.internal.GeodeGlossary;
@@ -359,7 +359,7 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
/**
* Analyze a given view object, generate events as appropriate
*/
- public void processView(long newViewId, MembershipView<ID> newView) {
+ public void processView(MembershipView<ID> newView) {
// Sanity check...
if (logger.isDebugEnabled()) {
StringBuilder msg = new StringBuilder(200);
@@ -377,35 +377,17 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
// incoming events will not be lost in terms of our global view.
latestViewWriteLock.lock();
try {
- // first determine the version for multicast message serialization
- Version version = KnownVersion.CURRENT;
- for (final Entry<ID, Long> internalIDLongEntry : surpriseMembers
- .entrySet()) {
- ID mbr = internalIDLongEntry.getKey();
- final Version itsVersion = mbr.getVersion();
- if (itsVersion != null && version.compareTo(itsVersion) < 0) {
- version = itsVersion;
- }
- }
- for (ID mbr : newView.getMembers()) {
- final Version itsVersion = mbr.getVersion();
- if (itsVersion != null && itsVersion.compareTo(version) < 0) {
- version = mbr.getVersion();
- }
- }
- disableMulticastForRollingUpgrade = !version.equals(KnownVersion.CURRENT);
-
+ setIsMulticastAllowedFrom(newView, surpriseMembers);
// Save previous view, for delta analysis
MembershipView<ID> priorView = latestView;
- if (newViewId < priorView.getViewId()) {
+ if (newView.getViewId() < priorView.getViewId()) {
// ignore this view since it is old news
return;
}
// update the view to reflect our changes, so that
// callbacks will see the new (updated) view.
- long newlatestViewId = newViewId;
MembershipView<ID> newlatestView = new MembershipView<>(newView, newView.getViewId());
// look for additions
@@ -531,6 +513,18 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
}
}
+ private void setIsMulticastAllowedFrom(final MembershipView<ID> newView,
+ final Map<ID, Long> surpriseMembers) {
+ disableMulticastForRollingUpgrade =
+ anyMemberHasOlderVersion(
+ Stream.concat(surpriseMembers.keySet().stream(), newView.getMembers().stream()));
+ }
+
+ private boolean anyMemberHasOlderVersion(final Stream<ID> members) {
+ return members
+ .anyMatch(member -> KnownVersion.CURRENT.isNewerThan(member.getVersion()));
+ }
+
@Override
public <V> V doWithViewLocked(Supplier<V> function) {
latestViewReadLock.lock();
@@ -987,7 +981,7 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
}
}
- viewExecutor.submit(() -> processView(viewArg.getViewId(), viewArg));
+ viewExecutor.submit(() -> processView(viewArg));
} finally {
latestViewWriteLock.unlock();
@@ -1046,7 +1040,7 @@ public class GMSMembership<ID extends MemberIdentifier> implements Membership<ID
// message from non-member - ignore
}
} else if (o.isGmsView()) { // view event
- processView(o.gmsView.getViewId(), o.gmsView);
+ processView(o.gmsView);
} else if (o.isSurpriseConnect()) { // connect
processSurpriseConnect(o.member);
} else {
diff --git a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/MemberIdentifierImpl.java b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/MemberIdentifierImpl.java
index daa1a20..f227a97 100644
--- a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/MemberIdentifierImpl.java
+++ b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/MemberIdentifierImpl.java
@@ -982,7 +982,7 @@ public class MemberIdentifierImpl implements MemberIdentifier, DataSerializableF
return sb.toString();
}
- public void setVersionForTest(KnownVersion v) {
+ public void setVersionForTest(Version v) {
memberData.setVersion(v);
cachedToString = null;
}