geode-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From bschucha...@apache.org
Subject [1/2] geode git commit: GEODE-2497 surprise members are never timed out during startup
Date Tue, 21 Feb 2017 23:26:38 GMT
Repository: geode
Updated Branches:
  refs/heads/feature/GEODE-2497 8d45ca227 -> 69e3523ec


GEODE-2497 surprise members are never timed out during startup

Moved the creation of the timer to GMSMembershipManager.started()

Removed write-lock in timer-creation method since it's only called from
one place now

Altered the way that the timer-creation method finds the
InternalDistributedSystem.  The old way of using getAnyInstance() was
the primary source of the problem since it returns null until startup
is completed.

Altered the surprise-member unit test to ensure that it's using the
timer and not relying on installation of a new membership view to clean
things up.

Altered the surprise-member unit test to run faster.  It now completes in
under 10 seconds.


Project: http://git-wip-us.apache.org/repos/asf/geode/repo
Commit: http://git-wip-us.apache.org/repos/asf/geode/commit/a6dfa4ca
Tree: http://git-wip-us.apache.org/repos/asf/geode/tree/a6dfa4ca
Diff: http://git-wip-us.apache.org/repos/asf/geode/diff/a6dfa4ca

Branch: refs/heads/feature/GEODE-2497
Commit: a6dfa4ca630a82fcf92942a834f8255e86d2bfcb
Parents: dbea592
Author: Bruce Schuchardt <bschuchardt@pivotal.io>
Authored: Fri Feb 17 10:17:21 2017 -0800
Committer: Bruce Schuchardt <bschuchardt@pivotal.io>
Committed: Tue Feb 21 15:07:00 2017 -0800

----------------------------------------------------------------------
 .../gms/mgr/GMSMembershipManager.java           | 76 ++++++++++----------
 .../internal/DistributionManagerDUnitTest.java  | 66 +++++++----------
 2 files changed, 62 insertions(+), 80 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/geode/blob/a6dfa4ca/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 cf17025..9652ad5 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
@@ -608,7 +608,6 @@ public class GMSMembershipManager implements MembershipManager, Manager
{
       }
       try {
         listener.viewInstalled(latestView);
-        startCleanupTimer();
       } catch (DistributedSystemDisconnectedException se) {
       }
     } finally {
@@ -616,6 +615,10 @@ public class GMSMembershipManager implements MembershipManager, Manager
{
     }
   }
 
+  public boolean isCleanupTimerStarted() {
+    return this.cleanupTimer != null;
+  }
+
   /**
    * the timer used to perform periodic tasks
    * 
@@ -767,7 +770,9 @@ public class GMSMembershipManager implements MembershipManager, Manager
{
   }
 
   @Override
-  public void started() {}
+  public void started() {
+    startCleanupTimer();
+  }
 
 
   /** this is invoked by JoinLeave when there is a loss of quorum in the membership system
*/
@@ -942,12 +947,6 @@ public class GMSMembershipManager implements MembershipManager, Manager
{
         surpriseMembers.remove(member);
       } else {
 
-        // Now that we're sure the member is new, add them.
-        // make sure the surprise-member cleanup task is running
-        if (this.cleanupTimer == null) {
-          startCleanupTimer();
-        } // cleanupTimer == null
-
         // Ensure that the member is accounted for in the view
         // Conjure up a new view including the new member. This is necessary
         // because we are about to tell the listener about a new member, so
@@ -978,40 +977,39 @@ public class GMSMembershipManager implements MembershipManager, Manager
{
 
   /** starts periodic task to perform cleanup chores such as expire surprise members */
   private void startCleanupTimer() {
+    if (this.dcReceiver == null) {
+      // junit tests don't provide a direct-channel receiver
+      return;
+    }
+    DistributedSystem ds = this.dcReceiver.getDM().getSystem();
+    this.cleanupTimer = new SystemTimer(ds, true);
+    SystemTimer.SystemTimerTask st = new SystemTimer.SystemTimerTask() {
+      @Override
+      public void run2() {
+        cleanUpSurpriseMembers();
+      }
+    };
+    this.cleanupTimer.scheduleAtFixedRate(st, surpriseMemberTimeout, surpriseMemberTimeout
/ 3);
+  }
+
+  // invoked from the cleanupTimer task
+  private void cleanUpSurpriseMembers() {
     latestViewWriteLock.lock();
     try {
-      if (this.cleanupTimer != null) {
-        return;
+      long oldestAllowed = System.currentTimeMillis() - surpriseMemberTimeout;
+      for (Iterator it = surpriseMembers.entrySet().iterator(); it.hasNext();) {
+        Map.Entry entry = (Map.Entry) it.next();
+        Long birthtime = (Long) entry.getValue();
+        if (birthtime.longValue() < oldestAllowed) {
+          it.remove();
+          InternalDistributedMember m = (InternalDistributedMember) entry.getKey();
+          logger.info(LocalizedMessage.create(
+              LocalizedStrings.GroupMembershipService_MEMBERSHIP_EXPIRING_MEMBERSHIP_OF_SURPRISE_MEMBER_0,
+              m));
+          removeWithViewLock(m, true,
+              "not seen in membership view in " + surpriseMemberTimeout + "ms");
+        }
       }
-      DistributedSystem ds = InternalDistributedSystem.getAnyInstance();
-      if (ds != null && ds.isConnected()) {
-        this.cleanupTimer = new SystemTimer(ds, true);
-        SystemTimer.SystemTimerTask st = new SystemTimer.SystemTimerTask() {
-          @Override
-          public void run2() {
-            latestViewWriteLock.lock();
-            try {
-              long oldestAllowed = System.currentTimeMillis() - surpriseMemberTimeout;
-              for (Iterator it = surpriseMembers.entrySet().iterator(); it.hasNext();) {
-                Map.Entry entry = (Map.Entry) it.next();
-                Long birthtime = (Long) entry.getValue();
-                if (birthtime.longValue() < oldestAllowed) {
-                  it.remove();
-                  InternalDistributedMember m = (InternalDistributedMember) entry.getKey();
-                  logger.info(LocalizedMessage.create(
-                      LocalizedStrings.GroupMembershipService_MEMBERSHIP_EXPIRING_MEMBERSHIP_OF_SURPRISE_MEMBER_0,
-                      m));
-                  removeWithViewLock(m, true,
-                      "not seen in membership view in " + surpriseMemberTimeout + "ms");
-                }
-              }
-            } finally {
-              latestViewWriteLock.unlock();
-            }
-          }
-        };
-        this.cleanupTimer.scheduleAtFixedRate(st, surpriseMemberTimeout, surpriseMemberTimeout
/ 3);
-      } // ds != null && ds.isConnected()
     } finally {
       latestViewWriteLock.unlock();
     }

http://git-wip-us.apache.org/repos/asf/geode/blob/a6dfa4ca/geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionManagerDUnitTest.java
----------------------------------------------------------------------
diff --git a/geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionManagerDUnitTest.java
b/geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionManagerDUnitTest.java
index ccce013..c795261 100644
--- a/geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionManagerDUnitTest.java
+++ b/geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionManagerDUnitTest.java
@@ -17,6 +17,7 @@ package org.apache.geode.distributed.internal;
 import static org.apache.geode.distributed.ConfigurationProperties.*;
 import static org.apache.geode.test.dunit.Assert.*;
 
+import org.apache.geode.test.dunit.rules.DistributedRestoreSystemProperties;
 import org.awaitility.Awaitility;
 
 import java.net.InetAddress;
@@ -28,6 +29,7 @@ import org.apache.geode.test.junit.categories.MembershipTest;
 import org.apache.logging.log4j.Logger;
 import org.junit.Assert;
 import org.junit.Ignore;
+import org.junit.Rule;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
 
@@ -74,6 +76,10 @@ public class DistributionManagerDUnitTest extends JUnit4DistributedTestCase
{
 
   public static DistributedSystem ds;
 
+  @Rule
+  public DistributedRestoreSystemProperties restoreSystemProperties =
+      new DistributedRestoreSystemProperties();
+
   /**
    * Clears the exceptionInThread flag in the given distribution manager.
    */
@@ -137,18 +143,14 @@ public class DistributionManagerDUnitTest extends JUnit4DistributedTestCase
{
     InternalDistributedMember idm = mgr.getLocalMember();
     // TODO GMS needs to have a system property allowing the bind-port to be set
     System.setProperty(DistributionConfig.GEMFIRE_PREFIX + "jg-bind-port", "" + idm.getPort());
-    try {
-      sys.disconnect();
-      sys = getSystem();
-      mgr = MembershipManagerHelper.getMembershipManager(sys);
-      sys.disconnect();
-      InternalDistributedMember idm2 = mgr.getLocalMember();
-      org.apache.geode.test.dunit.LogWriterUtils.getLogWriter()
-          .info("original ID=" + idm + " and after connecting=" + idm2);
-      assertTrue("should not have used a different udp port", idm.getPort() == idm2.getPort());
-    } finally {
-      System.getProperties().remove(DistributionConfig.GEMFIRE_PREFIX + "jg-bind-port");
-    }
+    sys.disconnect();
+    sys = getSystem();
+    mgr = MembershipManagerHelper.getMembershipManager(sys);
+    sys.disconnect();
+    InternalDistributedMember idm2 = mgr.getLocalMember();
+    org.apache.geode.test.dunit.LogWriterUtils.getLogWriter()
+        .info("original ID=" + idm + " and after connecting=" + idm2);
+    assertTrue("should not have used a different udp port", idm.getPort() == idm2.getPort());
   }
 
   /**
@@ -158,11 +160,12 @@ public class DistributionManagerDUnitTest extends JUnit4DistributedTestCase
{
    * should be gone and force more view processing to have it scrubbed from the set.
    **/
   @Test
-  public void testSurpriseMemberHandling() {
-    VM vm0 = Host.getHost(0).getVM(0);
+  public void testSurpriseMemberHandling() throws Exception {
 
+    System.setProperty(DistributionConfig.GEMFIRE_PREFIX + "surprise-member-timeout", "3000");
     InternalDistributedSystem sys = getSystem();
     MembershipManager mgr = MembershipManagerHelper.getMembershipManager(sys);
+    assertTrue(((GMSMembershipManager) mgr).isCleanupTimerStarted());
 
     try {
       InternalDistributedMember mbr =
@@ -180,19 +183,13 @@ public class DistributionManagerDUnitTest extends JUnit4DistributedTestCase
{
           .info("current membership view is " + mgr.getView());
       org.apache.geode.test.dunit.LogWriterUtils.getLogWriter()
           .info("created ID " + mbr + " with view ID " + mbr.getVmViewId());
-      sys.getLogWriter()
-          .info("<ExpectedException action=add>attempt to add old member</ExpectedException>");
-      sys.getLogWriter()
-          .info("<ExpectedException action=add>Removing shunned GemFire node</ExpectedException>");
-      try {
-        boolean accepted = mgr.addSurpriseMember(mbr);
-        Assert.assertTrue("member with old ID was not rejected (bug #44566)", !accepted);
-      } finally {
-        sys.getLogWriter()
-            .info("<ExpectedException action=remove>attempt to add old member</ExpectedException>");
-        sys.getLogWriter().info(
-            "<ExpectedException action=remove>Removing shunned GemFire node</ExpectedException>");
-      }
+
+      IgnoredException.addIgnoredException("attempt to add old member");
+      IgnoredException.addIgnoredException("Removing shunned GemFire node");
+
+      boolean accepted = mgr.addSurpriseMember(mbr);
+      Assert.assertTrue("member with old ID was not rejected (bug #44566)", !accepted);
+
       mbr.setVmViewId(oldViewId);
 
       // now forcibly add it as a surprise member and show that it is reaped
@@ -203,27 +200,14 @@ public class DistributionManagerDUnitTest extends JUnit4DistributedTestCase
{
       MembershipManagerHelper.addSurpriseMember(sys, mbr, birthTime);
       assertTrue("Member was not a surprise member", mgr.isSurpriseMember(mbr));
 
-      // force a real view change
-      SerializableRunnable connectDisconnect = new SerializableRunnable() {
-        public void run() {
-          getSystem().disconnect();
-        }
-      };
-      vm0.invoke(connectDisconnect);
-
       if (birthTime < (System.currentTimeMillis() - timeout)) {
         return; // machine is too busy and we didn't get enough CPU to perform more assertions
       }
       assertTrue("Member was incorrectly removed from surprise member set",
           mgr.isSurpriseMember(mbr));
 
-      try {
-        Thread.sleep(gracePeriod);
-      } catch (InterruptedException e) {
-        fail("test was interrupted", e);
-      }
+      Thread.sleep((timeout / 3) + gracePeriod);
 
-      vm0.invoke(connectDisconnect);
       assertTrue("Member was not removed from surprise member set", !mgr.isSurpriseMember(mbr));
 
     } finally {


Mime
View raw message