GEODE-3043 surprise member added when the member is already in the cluster Modified InternalDistributedMember to disregard the "name" field in compareTo if partial IDs are being used in the comparison.
Modified GMSMembershipManager to attempt to replace partial IDs with the full IDs from the membership view. Typically these IDs will cause no problems and methods in GMSMembershipManager that send messages to a recipient will also attempt to replace the ID with a full ID from the membership view, but it's good to try to keep IDs canonical. Project: http://git-wip-us.apache.org/repos/asf/geode/repo Commit: http://git-wip-us.apache.org/repos/asf/geode/commit/e07591ca Tree: http://git-wip-us.apache.org/repos/asf/geode/tree/e07591ca Diff: http://git-wip-us.apache.org/repos/asf/geode/diff/e07591ca Branch: refs/heads/feature/GEODE-3023 Commit: e07591caa0a4857d144d25b9f71e093f38eab38c Parents: 4dce200 Author: Bruce Schuchardt <[email protected]> Authored: Thu Jun 8 16:59:48 2017 -0700 Committer: Udo Kohlmeyer <[email protected]> Committed: Fri Jun 9 13:12:04 2017 -0700 ---------------------------------------------------------------------- .../membership/InternalDistributedMember.java | 33 ++++-- .../internal/membership/gms/GMSMember.java | 24 +++++ .../gms/mgr/GMSMembershipManager.java | 24 +++++ .../distributed/DistributedMemberDUnitTest.java | 107 ++++++++++++------- 4 files changed, 141 insertions(+), 47 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/geode/blob/e07591ca/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java ---------------------------------------------------------------------- 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 6982d31..2060934 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 @@ -163,6 +163,7 @@ public class InternalDistributedMember implements DistributedMember, Externaliza this.versionObj = Version.CURRENT; } cachedToString = null; + this.isPartial = true; } /** @@ -558,16 +559,18 @@ public class InternalDistributedMember implements DistributedMember, Externaliza String myName = getName(); String otherName = other.getName(); - if (myName == null && otherName == null) { - // do nothing - } else if (myName == null) { - return -1; - } else if (otherName == null) { - return 1; - } else { - int i = myName.compareTo(otherName); - if (i != 0) { - return i; + if (!(other.isPartial || this.isPartial)) { + if (myName == null && otherName == null) { + // do nothing + } else if (myName == null) { + return -1; + } else if (otherName == null) { + return 1; + } else { + int i = myName.compareTo(otherName); + if (i != 0) { + return i; + } } } @@ -605,6 +608,16 @@ public class InternalDistributedMember implements DistributedMember, Externaliza // @todo Add durableClientAttributes to compare } + /** + * An InternalDistributedMember created for a test or via readEssentialData will be a Partial ID, + * possibly not having ancillary info like "name". + * + * @return true if this is a partial ID + */ + public boolean isPartial() { + return isPartial; + } + @Override public boolean equals(Object obj) { if (this == obj) { http://git-wip-us.apache.org/repos/asf/geode/blob/e07591ca/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMember.java ---------------------------------------------------------------------- diff --git a/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMember.java b/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMember.java index c82d97e..e368c9a 100755 --- a/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMember.java +++ b/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMember.java @@ -138,6 +138,30 @@ public class GMSMember implements NetMember, DataSerializableFixedID { this.vmViewId = viewId; } + + /** + * Clone a GMSMember + * + * @param other the member to create a copy of + */ + public GMSMember(GMSMember other) { + this.udpPort = other.udpPort; + this.preferredForCoordinator = other.preferredForCoordinator; + this.networkPartitionDetectionEnabled = other.networkPartitionDetectionEnabled; + this.memberWeight = other.memberWeight; + this.inetAddr = other.inetAddr; + this.processId = other.processId; + this.vmKind = other.vmKind; + this.vmViewId = other.vmViewId; + this.directPort = other.directPort; + this.name = other.name; + this.durableClientAttributes = other.durableClientAttributes; + this.groups = other.groups; + this.versionOrdinal = other.versionOrdinal; + this.uuidLSBs = other.uuidLSBs; + this.uuidMSBs = other.uuidMSBs; + } + public int getPort() { return this.udpPort; } http://git-wip-us.apache.org/repos/asf/geode/blob/e07591ca/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/mgr/GMSMembershipManager.java ---------------------------------------------------------------------- diff --git a/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/mgr/GMSMembershipManager.java b/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/mgr/GMSMembershipManager.java index fe560d9..639c6a8 100644 --- a/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/mgr/GMSMembershipManager.java +++ b/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/mgr/GMSMembershipManager.java @@ -1077,6 +1077,12 @@ public class GMSMembershipManager implements MembershipManager, Manager { InternalDistributedMember m = msg.getSender(); boolean shunned = false; + // UDP messages received from surprise members will have partial IDs. + // Attempt to replace these with full IDs from the MembershipManager's view. + if (msg.getSender().isPartial()) { + replacePartialIdentifierInMessage(msg); + } + // If this member is shunned or new, grab the latestViewWriteLock: update the appropriate data // structure. // synchronized (latestViewLock) { @@ -1117,6 +1123,24 @@ public class GMSMembershipManager implements MembershipManager, Manager { listener.messageReceived(msg); } + /** + * If the message's sender ID is a partial ID, which can happen if it's received in a JGroups + * message, replace it with an ID from the current membership view. + * + * @param msg the message holding the sender ID + */ + public void replacePartialIdentifierInMessage(DistributionMessage msg) { + InternalDistributedMember sender = msg.getSender(); + sender = this.services.getJoinLeave().getMemberID(sender.getNetMember()); + if (sender.isPartial()) { + // the DM's view also has surprise members, so let's check it as well + sender = this.dcReceiver.getDM().getCanonicalId(sender); + } + if (!sender.isPartial()) { + msg.setSender(sender); + } + } + /** * Process a new view object, or place on the startup queue http://git-wip-us.apache.org/repos/asf/geode/blob/e07591ca/geode-core/src/test/java/org/apache/geode/distributed/DistributedMemberDUnitTest.java ---------------------------------------------------------------------- diff --git a/geode-core/src/test/java/org/apache/geode/distributed/DistributedMemberDUnitTest.java b/geode-core/src/test/java/org/apache/geode/distributed/DistributedMemberDUnitTest.java index 0153ca6..eb0e658 100755 --- a/geode-core/src/test/java/org/apache/geode/distributed/DistributedMemberDUnitTest.java +++ b/geode-core/src/test/java/org/apache/geode/distributed/DistributedMemberDUnitTest.java @@ -17,20 +17,27 @@ package org.apache.geode.distributed; import org.apache.geode.IncompatibleSystemException; import org.apache.geode.distributed.internal.DM; import org.apache.geode.distributed.internal.DistributionConfig; +import org.apache.geode.distributed.internal.HighPriorityAckedMessage; import org.apache.geode.distributed.internal.InternalDistributedSystem; import org.apache.geode.distributed.internal.membership.InternalDistributedMember; +import org.apache.geode.distributed.internal.membership.gms.GMSMember; +import org.apache.geode.distributed.internal.membership.gms.MembershipManagerHelper; +import org.apache.geode.distributed.internal.membership.gms.mgr.GMSMembershipManager; import org.apache.geode.test.dunit.Host; import org.apache.geode.test.dunit.SerializableCallable; import org.apache.geode.test.dunit.SerializableRunnable; import org.apache.geode.test.dunit.VM; import org.apache.geode.test.dunit.internal.JUnit4DistributedTestCase; import org.apache.geode.test.junit.categories.DistributedTest; +import org.awaitility.Awaitility; +import org.jgroups.protocols.pbcast.GMS; import org.junit.Test; import org.junit.experimental.categories.Category; import java.net.InetAddress; import java.net.UnknownHostException; import java.util.*; +import java.util.concurrent.TimeUnit; import static org.apache.geode.distributed.ConfigurationProperties.*; import static org.apache.geode.test.dunit.Assert.*; @@ -38,19 +45,10 @@ import static org.apache.geode.test.dunit.Assert.*; /** * Tests the functionality of the {@link DistributedMember} class. * - * @since GemFire 5.0 */ @Category(DistributedTest.class) public class DistributedMemberDUnitTest extends JUnit4DistributedTestCase { - protected void sleep(long millis) { // TODO: replace with Awaitility - try { - Thread.sleep(millis); - } catch (InterruptedException e) { - fail("interrupted"); - } - } - /** * Tests default settings. */ @@ -209,20 +207,9 @@ public class DistributedMemberDUnitTest extends JUnit4DistributedTestCase { Role myRole = (Role) myRoles.iterator().next(); assertTrue(vmRoles[vm].equals(myRole.getName())); - Set members = null; - for (int i = 1; i <= 3; i++) { - try { - members = dm.getOtherNormalDistributionManagerIds(); - assertEquals(3, members.size()); - break; - } catch (AssertionError e) { // TODO: delete this - if (i < 3) { - sleep(200); - } else { - throw e; - } - } - } + Awaitility.await().atMost(10, TimeUnit.SECONDS) + .until(() -> dm.getOtherNormalDistributionManagerIds().size() == 3); + Set<DistributedMember> members = dm.getOtherNormalDistributionManagerIds(); for (Iterator iterMembers = members.iterator(); iterMembers.hasNext();) { InternalDistributedMember member = (InternalDistributedMember) iterMembers.next(); @@ -246,6 +233,62 @@ public class DistributedMemberDUnitTest extends JUnit4DistributedTestCase { } } + + private InternalDistributedMember connectAndSetUpPartialID() throws Exception { + Properties properties = new Properties(); + properties.put("name", "myName"); + InternalDistributedSystem system = getSystem(properties); + assertTrue(system == basicGetSystem()); // senders will use basicGetSystem() + InternalDistributedMember internalDistributedMember = system.getDistributedMember(); + + GMSMember gmsMember = new GMSMember((GMSMember) internalDistributedMember.getNetMember()); + assertTrue(gmsMember.equals(internalDistributedMember.getNetMember())); + gmsMember.setName(null); + InternalDistributedMember partialID = new InternalDistributedMember(gmsMember); + return partialID; + } + + /** + * GEODE-3043 surprise member added when member is already in the cluster + * + * This test ensures that a partial ID (with no "name") is equal to its equivalent non-partial ID. + * + * @throws Exception + */ + @Test + public void testMemberNameIgnoredInPartialID() throws Exception { + InternalDistributedMember partialID = connectAndSetUpPartialID(); + + InternalDistributedMember internalDistributedMember = basicGetSystem().getDistributedMember(); + assertTrue(partialID.isPartial()); + assertFalse(internalDistributedMember.isPartial()); + + assertEquals(internalDistributedMember, partialID); + assertEquals(partialID, internalDistributedMember); + } + + /** + * GEODE-3043 surprise member added when member is already in the cluster + * + * This test ensures that the membership manager can detect and replace a partial ID with one that + * is not partial + * + * @throws Exception + */ + @Test + public void testPartialIDInMessageReplacedWithFullID() throws Exception { + InternalDistributedMember partialID = connectAndSetUpPartialID(); + + HighPriorityAckedMessage message = new HighPriorityAckedMessage(); + message.setSender(partialID); + + GMSMembershipManager manager = + (GMSMembershipManager) MembershipManagerHelper.getMembershipManager(basicGetSystem()); + manager.replacePartialIdentifierInMessage(message); + + assertFalse(message.getSender().isPartial()); + } + private static String makeOddEvenString(int vm) { return ((vm % 2) == 0) ? "EVENS" : "ODDS"; } @@ -291,20 +334,10 @@ public class DistributedMemberDUnitTest extends JUnit4DistributedTestCase { assertEquals(Arrays.asList("" + vm, makeOddEvenString(vm)), myGroups); - Set<DistributedMember> members = null; - for (int i = 1; i <= 3; i++) { - try { - members = dm.getOtherNormalDistributionManagerIds(); - assertEquals(3, members.size()); - break; - } catch (AssertionError e) { // TODO: delete this - if (i < 3) { - sleep(200); - } else { - throw e; - } - } - } + Awaitility.await().atMost(10, TimeUnit.SECONDS) + .until(() -> dm.getOtherNormalDistributionManagerIds().size() == 3); + Set<DistributedMember> members = dm.getOtherNormalDistributionManagerIds(); + // Make sure getAllOtherMembers returns a set // containing our three peers plus an admin member. Set<DistributedMember> others = sys.getAllOtherMembers();
