Skip to content

HBASE-23275: Track active master's address in ActiveMasterManager #812

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ndimiduk merged 2 commits into from
Nov 20, 2019
Merged

HBASE-23275: Track active master's address in ActiveMasterManager #812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
865fe77
HBASE-23275: Track active master's address in ActiveMasterManager
bharathv Nov 9, 2019
84a82c5
Address Nick's comments.
bharathv Nov 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 50 additions & 13 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/**
/*
*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
Expand All @@ -17,25 +17,24 @@
* limitations under the License.
*/
package org.apache.hadoop.hbase.master;

import java.io.IOException;
import java.util.Optional;
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 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.
Expand All @@ -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.
Copy link
Member

@ndimiduk ndimiduk Nov 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think this field can replace the AtomicBoolean clusterHasActiveMaster. Seems your new information is a super-set of the existing information.

Copy link
Contributor Author

@bharathv bharathv Nov 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't do this because I implemented it as a lazy-fetch (don't fetch it unless some client asks for it). This way we also don't touch the battle-tested synchronization around 'clusterHasActiveMaster' and hence lesser risk of introducing any new bugs. Let me know if you feel strongly, I'll refactor the whole thing.

Copy link
Member

@ndimiduk ndimiduk Nov 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is a fair point. It would be nice to consolidate these concerns, but if we don't have the test coverage and you don't have the capacity for a manual test, better leave it as is.

private volatile ServerName activeMasterServerName;

/**
* @param watcher
* @param watcher ZK watcher
* @param sn ServerName
* @param master In an instance of a Master.
*/
Expand Down Expand Up @@ -106,6 +111,30 @@ 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 = 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 Optional<ServerName> getActiveMasterServerName() {
if (!clusterHasActiveMaster.get()) {
return Optional.empty();
}
if (activeMasterServerName == null) {
fetchAndSetActiveMasterServerName();
}
// It could still be null, but return whatever we have.
return Optional.ofNullable(activeMasterServerName);
}

/**
* 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
Expand Down Expand Up @@ -134,6 +163,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 = null;
}
} catch (KeeperException ke) {
master.abort("Received an unexpected KeeperException, aborting", ke);
Expand All @@ -151,8 +183,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) {
Expand All @@ -178,10 +210,14 @@ boolean blockUntilBecomingActiveMaster(
// We are the master, return
startupStatus.setStatus("Successfully registered as active master.");
this.clusterHasActiveMaster.set(true);
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 = 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);
Expand All @@ -208,7 +244,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 +
Expand Down
3 changes: 3 additions & 0 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3869,4 +3869,7 @@ public String getClusterId() {
return cachedClusterId.getFromCacheOrFetch();
}

public Optional<ServerName> getActiveMaster() {
return activeMasterManager.getActiveMasterServerName();
}
}
10 changes: 10 additions & 0 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestActiveMasterManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -91,6 +92,7 @@ public static void tearDownAfterClass() throws Exception {
ActiveMasterManager activeMasterManager =
dummyMaster.getActiveMasterManager();
assertFalse(activeMasterManager.clusterHasActiveMaster.get());
assertFalse(activeMasterManager.getActiveMasterServerName().isPresent());

// First test becoming the active master uninterrupted
MonitoredTask status = Mockito.mock(MonitoredTask.class);
Expand All @@ -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().get());

// Now pretend master restart
DummyMaster secondDummyMaster = new DummyMaster(zk,master);
Expand All @@ -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().get());
assertMaster(zk, secondActiveMasterManager.getActiveMasterServerName().get());
}

/**
Expand Down Expand Up @@ -135,6 +140,7 @@ public void testActiveMasterManagerFromZK() throws Exception {
ActiveMasterManager activeMasterManager =
ms1.getActiveMasterManager();
assertFalse(activeMasterManager.clusterHasActiveMaster.get());
assertFalse(activeMasterManager.getActiveMasterServerName().isPresent());

// First test becoming the active master uninterrupted
ClusterStatusTracker clusterStatusTracker =
Expand All @@ -144,6 +150,7 @@ public void testActiveMasterManagerFromZK() throws Exception {
Mockito.mock(MonitoredTask.class));
assertTrue(activeMasterManager.clusterHasActiveMaster.get());
assertMaster(zk, firstMasterAddress);
assertMaster(zk, activeMasterManager.getActiveMasterServerName().get());

// New manager will now try to become the active master in another thread
WaitToBeMasterThread t = new WaitToBeMasterThread(zk, secondMasterAddress);
Expand All @@ -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().get());

// Close the first server and delete it's master node
ms1.stop("stopping first server");
Expand Down Expand Up @@ -189,6 +198,7 @@ public void testActiveMasterManagerFromZK() throws Exception {

assertTrue(t.manager.clusterHasActiveMaster.get());
assertTrue(t.isActiveMaster);
assertEquals(secondMasterAddress, t.manager.getActiveMasterServerName().get());

LOG.info("Deleting master node");

Expand Down