From 865fe772309df750ecd136a4ab194f596ad98d52 Mon Sep 17 00:00:00 2001 From: Bharath Vissapragada Date: Sat, 9 Nov 2019 15:47:43 -0800 Subject: [PATCH 1/2] HBASE-23275: Track active master's address in ActiveMasterManager Currently we just track whether an active master exists. It helps to also track the address of the active master in all the masters to help serve the client RPC requests to know which master is active. --- .../hbase/master/ActiveMasterManager.java | 65 +++++++++++++++---- .../apache/hadoop/hbase/master/HMaster.java | 3 + .../hbase/master/TestActiveMasterManager.java | 10 +++ 3 files changed, 66 insertions(+), 12 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java index 50798ed60a0a..b0a68c0864e6 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java @@ -17,25 +17,24 @@ * limitations under the License. */ package org.apache.hadoop.hbase.master; - import java.io.IOException; import java.util.concurrent.atomic.AtomicBoolean; - -import org.apache.hadoop.hbase.zookeeper.MasterAddressTracker; -import org.apache.hadoop.hbase.zookeeper.ZKUtil; -import org.apache.hadoop.hbase.zookeeper.ZKWatcher; -import org.apache.hadoop.hbase.zookeeper.ZNodePaths; -import org.apache.yetus.audience.InterfaceAudience; +import java.util.concurrent.atomic.AtomicReference; import org.apache.hadoop.hbase.Server; import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.ZNodeClearer; import org.apache.hadoop.hbase.exceptions.DeserializationException; import org.apache.hadoop.hbase.monitoring.MonitoredTask; -import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil; +import org.apache.hadoop.hbase.zookeeper.MasterAddressTracker; import org.apache.hadoop.hbase.zookeeper.ZKListener; +import org.apache.hadoop.hbase.zookeeper.ZKUtil; +import org.apache.hadoop.hbase.zookeeper.ZKWatcher; +import org.apache.hadoop.hbase.zookeeper.ZNodePaths; +import org.apache.yetus.audience.InterfaceAudience; import org.apache.zookeeper.KeeperException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil; /** * Handles everything on master-side related to master election. @@ -57,12 +56,18 @@ public class ActiveMasterManager extends ZKListener { final AtomicBoolean clusterHasActiveMaster = new AtomicBoolean(false); final AtomicBoolean clusterShutDown = new AtomicBoolean(false); + // This server's information. private final ServerName sn; private int infoPort; private final Server master; + // Active master's server name. Invalidated anytime active master changes (based on ZK + // notifications) and lazily fetched on-demand. + // ServerName is immutable, so we don't need heavy synchronization around it. + private final AtomicReference activeMasterServerName; + /** - * @param watcher + * @param watcher ZK watcher * @param sn ServerName * @param master In an instance of a Master. */ @@ -71,6 +76,7 @@ public class ActiveMasterManager extends ZKListener { watcher.registerListener(this); this.sn = sn; this.master = master; + activeMasterServerName = new AtomicReference<>(); } // will be set after jetty server is started @@ -106,6 +112,33 @@ void handle(final String path) { } } + /* + * Fetches the active master's ServerName from zookeeper. + */ + private void fetchAndSetActiveMasterServerName() { + LOG.debug("Attempting to fetch active master sn from zk"); + try { + activeMasterServerName.set(MasterAddressTracker.getMasterAddress(watcher)); + } catch (IOException | KeeperException e) { + // Log and ignore for now and re-fetch later if needed. + LOG.error("Error fetching active master information", e); + } + } + + public ServerName getActiveMasterServerName() { + if (!clusterHasActiveMaster.get()) { + return null; + } + ServerName sname = activeMasterServerName.get(); + if (sname != null) { + return sname; + } + // This happens if the data was not fetched earlier for some reason. + fetchAndSetActiveMasterServerName(); + // It could still be null, but return whatever we have. + return activeMasterServerName.get(); + } + /** * Handle a change in the master node. Doesn't matter whether this was called * from a nodeCreated or nodeDeleted event because there are no guarantees @@ -134,6 +167,9 @@ private void handleMasterNodeChange() { // Notify any thread waiting to become the active master clusterHasActiveMaster.notifyAll(); } + // Reset the active master sn. Will be re-fetched later if needed. + // We don't want to make a synchronous RPC under a monitor. + activeMasterServerName.set(null); } } catch (KeeperException ke) { master.abort("Received an unexpected KeeperException, aborting", ke); @@ -151,8 +187,8 @@ private void handleMasterNodeChange() { * @param checkInterval the interval to check if the master is stopped * @param startupStatus the monitor status to track the progress * @return True if no issue becoming active master else false if another - * master was running or if some other problem (zookeeper, stop flag has been - * set on this Master) + * master was running or if some other problem (zookeeper, stop flag has been + * set on this Master) */ boolean blockUntilBecomingActiveMaster( int checkInterval, MonitoredTask startupStatus) { @@ -178,10 +214,14 @@ boolean blockUntilBecomingActiveMaster( // We are the master, return startupStatus.setStatus("Successfully registered as active master."); this.clusterHasActiveMaster.set(true); + activeMasterServerName.set(sn); LOG.info("Registered as active master=" + this.sn); return true; } + // Invalidate the active master name so that subsequent requests do not get any stale + // master information. Will be re-fetched if needed. + activeMasterServerName.set(null); // There is another active master running elsewhere or this is a restart // and the master ephemeral node has not expired yet. this.clusterHasActiveMaster.set(true); @@ -208,7 +248,8 @@ boolean blockUntilBecomingActiveMaster( ZKUtil.deleteNode(this.watcher, this.watcher.getZNodePaths().masterAddressZNode); // We may have failed to delete the znode at the previous step, but - // we delete the file anyway: a second attempt to delete the znode is likely to fail again. + // we delete the file anyway: a second attempt to delete the znode is likely to fail + // again. ZNodeClearer.deleteMyEphemeralNodeOnDisk(); } else { msg = "Another master is the active master, " + currentMaster + diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java index 43da8c10bee3..c9fd56fdf5b2 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java @@ -3869,4 +3869,7 @@ public String getClusterId() { return cachedClusterId.getFromCacheOrFetch(); } + public ServerName getActiveMaster() { + return activeMasterManager.getActiveMasterServerName(); + } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestActiveMasterManager.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestActiveMasterManager.java index 4649eea5ce06..255da7de1a9e 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestActiveMasterManager.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestActiveMasterManager.java @@ -17,6 +17,7 @@ */ package org.apache.hadoop.hbase.master; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; @@ -91,6 +92,7 @@ public static void tearDownAfterClass() throws Exception { ActiveMasterManager activeMasterManager = dummyMaster.getActiveMasterManager(); assertFalse(activeMasterManager.clusterHasActiveMaster.get()); + assertEquals(null, activeMasterManager.getActiveMasterServerName()); // First test becoming the active master uninterrupted MonitoredTask status = Mockito.mock(MonitoredTask.class); @@ -99,6 +101,7 @@ public static void tearDownAfterClass() throws Exception { activeMasterManager.blockUntilBecomingActiveMaster(100, status); assertTrue(activeMasterManager.clusterHasActiveMaster.get()); assertMaster(zk, master); + assertMaster(zk, activeMasterManager.getActiveMasterServerName()); // Now pretend master restart DummyMaster secondDummyMaster = new DummyMaster(zk,master); @@ -108,6 +111,8 @@ public static void tearDownAfterClass() throws Exception { activeMasterManager.blockUntilBecomingActiveMaster(100, status); assertTrue(activeMasterManager.clusterHasActiveMaster.get()); assertMaster(zk, master); + assertMaster(zk, activeMasterManager.getActiveMasterServerName()); + assertMaster(zk, secondActiveMasterManager.getActiveMasterServerName()); } /** @@ -135,6 +140,7 @@ public void testActiveMasterManagerFromZK() throws Exception { ActiveMasterManager activeMasterManager = ms1.getActiveMasterManager(); assertFalse(activeMasterManager.clusterHasActiveMaster.get()); + assertEquals(activeMasterManager.getActiveMasterServerName(), null); // First test becoming the active master uninterrupted ClusterStatusTracker clusterStatusTracker = @@ -144,6 +150,7 @@ public void testActiveMasterManagerFromZK() throws Exception { Mockito.mock(MonitoredTask.class)); assertTrue(activeMasterManager.clusterHasActiveMaster.get()); assertMaster(zk, firstMasterAddress); + assertMaster(zk, activeMasterManager.getActiveMasterServerName()); // New manager will now try to become the active master in another thread WaitToBeMasterThread t = new WaitToBeMasterThread(zk, secondMasterAddress); @@ -161,6 +168,8 @@ public void testActiveMasterManagerFromZK() throws Exception { assertTrue(t.manager.clusterHasActiveMaster.get()); // But secondary one should not be the active master assertFalse(t.isActiveMaster); + // Verify the active master ServerName is populated in standby master. + assertEquals(firstMasterAddress, t.manager.getActiveMasterServerName()); // Close the first server and delete it's master node ms1.stop("stopping first server"); @@ -189,6 +198,7 @@ public void testActiveMasterManagerFromZK() throws Exception { assertTrue(t.manager.clusterHasActiveMaster.get()); assertTrue(t.isActiveMaster); + assertEquals(secondMasterAddress, t.manager.getActiveMasterServerName()); LOG.info("Deleting master node"); From 84a82c5056225046d5c51d95645cf693f96b01e1 Mon Sep 17 00:00:00 2001 From: Bharath Vissapragada Date: Tue, 12 Nov 2019 09:11:13 -0800 Subject: [PATCH 2/2] Address Nick's comments. - Use volatile instead of AtomicReference - Switch to Optional for better Nullability semantics. --- .../hbase/master/ActiveMasterManager.java | 30 ++++++++----------- .../apache/hadoop/hbase/master/HMaster.java | 2 +- .../hbase/master/TestActiveMasterManager.java | 16 +++++----- 3 files changed, 22 insertions(+), 26 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java index b0a68c0864e6..99cab625b85c 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java @@ -1,4 +1,4 @@ -/** +/* * * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file @@ -18,8 +18,8 @@ */ package org.apache.hadoop.hbase.master; import java.io.IOException; +import java.util.Optional; import java.util.concurrent.atomic.AtomicBoolean; -import java.util.concurrent.atomic.AtomicReference; import org.apache.hadoop.hbase.Server; import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.ZNodeClearer; @@ -64,7 +64,7 @@ public class ActiveMasterManager extends ZKListener { // Active master's server name. Invalidated anytime active master changes (based on ZK // notifications) and lazily fetched on-demand. // ServerName is immutable, so we don't need heavy synchronization around it. - private final AtomicReference activeMasterServerName; + private volatile ServerName activeMasterServerName; /** * @param watcher ZK watcher @@ -76,7 +76,6 @@ public class ActiveMasterManager extends ZKListener { watcher.registerListener(this); this.sn = sn; this.master = master; - activeMasterServerName = new AtomicReference<>(); } // will be set after jetty server is started @@ -112,31 +111,28 @@ void handle(final String path) { } } - /* + /** * Fetches the active master's ServerName from zookeeper. */ private void fetchAndSetActiveMasterServerName() { LOG.debug("Attempting to fetch active master sn from zk"); try { - activeMasterServerName.set(MasterAddressTracker.getMasterAddress(watcher)); + activeMasterServerName = MasterAddressTracker.getMasterAddress(watcher); } catch (IOException | KeeperException e) { // Log and ignore for now and re-fetch later if needed. LOG.error("Error fetching active master information", e); } } - public ServerName getActiveMasterServerName() { + public Optional getActiveMasterServerName() { if (!clusterHasActiveMaster.get()) { - return null; + return Optional.empty(); } - ServerName sname = activeMasterServerName.get(); - if (sname != null) { - return sname; + if (activeMasterServerName == null) { + fetchAndSetActiveMasterServerName(); } - // This happens if the data was not fetched earlier for some reason. - fetchAndSetActiveMasterServerName(); // It could still be null, but return whatever we have. - return activeMasterServerName.get(); + return Optional.ofNullable(activeMasterServerName); } /** @@ -169,7 +165,7 @@ private void handleMasterNodeChange() { } // Reset the active master sn. Will be re-fetched later if needed. // We don't want to make a synchronous RPC under a monitor. - activeMasterServerName.set(null); + activeMasterServerName = null; } } catch (KeeperException ke) { master.abort("Received an unexpected KeeperException, aborting", ke); @@ -214,14 +210,14 @@ boolean blockUntilBecomingActiveMaster( // We are the master, return startupStatus.setStatus("Successfully registered as active master."); this.clusterHasActiveMaster.set(true); - activeMasterServerName.set(sn); + activeMasterServerName = sn; LOG.info("Registered as active master=" + this.sn); return true; } // Invalidate the active master name so that subsequent requests do not get any stale // master information. Will be re-fetched if needed. - activeMasterServerName.set(null); + activeMasterServerName = null; // There is another active master running elsewhere or this is a restart // and the master ephemeral node has not expired yet. this.clusterHasActiveMaster.set(true); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java index c9fd56fdf5b2..5c2a3ab1f674 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java @@ -3869,7 +3869,7 @@ public String getClusterId() { return cachedClusterId.getFromCacheOrFetch(); } - public ServerName getActiveMaster() { + public Optional getActiveMaster() { return activeMasterManager.getActiveMasterServerName(); } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestActiveMasterManager.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestActiveMasterManager.java index 255da7de1a9e..84837f93965a 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestActiveMasterManager.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestActiveMasterManager.java @@ -92,7 +92,7 @@ public static void tearDownAfterClass() throws Exception { ActiveMasterManager activeMasterManager = dummyMaster.getActiveMasterManager(); assertFalse(activeMasterManager.clusterHasActiveMaster.get()); - assertEquals(null, activeMasterManager.getActiveMasterServerName()); + assertFalse(activeMasterManager.getActiveMasterServerName().isPresent()); // First test becoming the active master uninterrupted MonitoredTask status = Mockito.mock(MonitoredTask.class); @@ -101,7 +101,7 @@ public static void tearDownAfterClass() throws Exception { activeMasterManager.blockUntilBecomingActiveMaster(100, status); assertTrue(activeMasterManager.clusterHasActiveMaster.get()); assertMaster(zk, master); - assertMaster(zk, activeMasterManager.getActiveMasterServerName()); + assertMaster(zk, activeMasterManager.getActiveMasterServerName().get()); // Now pretend master restart DummyMaster secondDummyMaster = new DummyMaster(zk,master); @@ -111,8 +111,8 @@ public static void tearDownAfterClass() throws Exception { activeMasterManager.blockUntilBecomingActiveMaster(100, status); assertTrue(activeMasterManager.clusterHasActiveMaster.get()); assertMaster(zk, master); - assertMaster(zk, activeMasterManager.getActiveMasterServerName()); - assertMaster(zk, secondActiveMasterManager.getActiveMasterServerName()); + assertMaster(zk, activeMasterManager.getActiveMasterServerName().get()); + assertMaster(zk, secondActiveMasterManager.getActiveMasterServerName().get()); } /** @@ -140,7 +140,7 @@ public void testActiveMasterManagerFromZK() throws Exception { ActiveMasterManager activeMasterManager = ms1.getActiveMasterManager(); assertFalse(activeMasterManager.clusterHasActiveMaster.get()); - assertEquals(activeMasterManager.getActiveMasterServerName(), null); + assertFalse(activeMasterManager.getActiveMasterServerName().isPresent()); // First test becoming the active master uninterrupted ClusterStatusTracker clusterStatusTracker = @@ -150,7 +150,7 @@ public void testActiveMasterManagerFromZK() throws Exception { Mockito.mock(MonitoredTask.class)); assertTrue(activeMasterManager.clusterHasActiveMaster.get()); assertMaster(zk, firstMasterAddress); - assertMaster(zk, activeMasterManager.getActiveMasterServerName()); + assertMaster(zk, activeMasterManager.getActiveMasterServerName().get()); // New manager will now try to become the active master in another thread WaitToBeMasterThread t = new WaitToBeMasterThread(zk, secondMasterAddress); @@ -169,7 +169,7 @@ public void testActiveMasterManagerFromZK() throws Exception { // But secondary one should not be the active master assertFalse(t.isActiveMaster); // Verify the active master ServerName is populated in standby master. - assertEquals(firstMasterAddress, t.manager.getActiveMasterServerName()); + assertEquals(firstMasterAddress, t.manager.getActiveMasterServerName().get()); // Close the first server and delete it's master node ms1.stop("stopping first server"); @@ -198,7 +198,7 @@ public void testActiveMasterManagerFromZK() throws Exception { assertTrue(t.manager.clusterHasActiveMaster.get()); assertTrue(t.isActiveMaster); - assertEquals(secondMasterAddress, t.manager.getActiveMasterServerName()); + assertEquals(secondMasterAddress, t.manager.getActiveMasterServerName().get()); LOG.info("Deleting master node");