Skip to content
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

Jump to bottom

HBASE-23259: Ability to start minicluster with pre-determined master ports #807

Merged
merged 1 commit into from Nov 20, 2019
Merged

HBASE-23259: Ability to start minicluster with pre-determined master ports #807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java
View file
Open in desktop
Expand Up @@ -171,6 +171,11 @@ public enum OperationStatusCode {
/** Configuration key for master web API port */
public static final String MASTER_INFO_PORT = "hbase.master.info.port";

/** Configuration key for the list of master host:ports **/
public static final String MASTER_ADDRS_KEY = "hbase.master.addrs";
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure parsing can handle both and : comma separated values. The other configuration setting like this is the zk quorum, which accepts a comma-separated list of : or . Let's keep things consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to confirm my understanding is correct, you mean the port number is optional right ? Something like "foo.com,bar.com:1234,baz.com:5678,temp.com"

Copy link
Member

Choose a reason for hiding this comment

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

Looking at our zookeeper connection string parsing code. We depend on the Hadoop Configuration object to handle parsing of the comma-delimited configuration value. We then manually check for ':' characters to split out ports.

If we're making a habit of doing this kind of parsing (lists of socket addresses), it's probably work encapsulating the logic into a single place.

hbase/hbase-common/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKConfig.java

Lines 97 to 111 in 5e84997

final String[] serverHosts = conf.getStrings(HConstants.ZOOKEEPER_QUORUM,
HConstants.LOCALHOST);
String serverHost;
String address;
String key;
for (int i = 0; i < serverHosts.length; ++i) {
if (serverHosts[i].contains(":")) {
serverHost = serverHosts[i].substring(0, serverHosts[i].indexOf(':'));
} else {
serverHost = serverHosts[i];
}
address = serverHost + ":" + peerPort + ":" + leaderPort;
key = "server." + i;
zkProperties.put(key, address);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I'll consolidate the parsing logic. It will come in the next patch.

Copy link
Member

Choose a reason for hiding this comment

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

You plan to address this last nit? I'm +1 with this bit handled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ndimiduk The parsing logic will come as a part of PR for HBASE-23305. There is nothing in this patch that consumes the content of "hbase.master.addrs".


public static final String MASTER_ADDRS_DEFAULT = "localhost:" + DEFAULT_MASTER_PORT;

/** Parameter name for the master type being backup (waits for primary to go inactive). */
public static final String MASTER_TYPE_BACKUP = "hbase.master.backup";

Expand Down
10 changes: 9 additions & 1 deletion hbase-server/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java
View file
Open in desktop
Expand Up @@ -173,6 +173,14 @@ public LocalHBaseCluster(final Configuration conf, final int noMasters,
for (int i = 0; i < noMasters; i++) {
addMaster(new Configuration(conf), i);
}

// Populate the master address host ports in the config. This is needed if a master based
// registry is configured for client metadata services (HBASE-18095)
List<String> masterHostPorts = new ArrayList<>();
getMasters().forEach(masterThread ->
bharathv marked this conversation as resolved.
Show resolved Hide resolved
masterHostPorts.add(masterThread.getMaster().getServerName().getAddress().toString()));
conf.set(HConstants.MASTER_ADDRS_KEY, String.join(",", masterHostPorts));

// Start the HRegionServers.
this.regionServerClass =
(Class<? extends HRegionServer>)conf.getClass(HConstants.REGION_SERVER_IMPL,
Expand Down Expand Up @@ -220,7 +228,7 @@ public JVMClusterUtil.MasterThread addMaster() throws IOException {
}

public JVMClusterUtil.MasterThread addMaster(Configuration c, final int index)
throws IOException {
throws IOException {
// Create each master with its own Configuration instance so each has
// its Connection instance rather than share (see HBASE_INSTANCES down in
// the guts of ConnectionManager.
Expand Down
12 changes: 10 additions & 2 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/TestHBaseTestingUtility.java
View file
Open in desktop
@@ -1,4 +1,4 @@
/**
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
Expand Down Expand Up @@ -447,10 +447,14 @@ public void testOverridingOfDefaultPorts() throws Exception {
HBaseTestingUtility htu = new HBaseTestingUtility(defaultConfig);
try {
MiniHBaseCluster defaultCluster = htu.startMiniCluster();
final String masterHostPort =
defaultCluster.getMaster().getServerName().getAddress().toString();
assertNotEquals(HConstants.DEFAULT_MASTER_INFOPORT,
defaultCluster.getConfiguration().getInt(HConstants.MASTER_INFO_PORT, 0));
assertNotEquals(HConstants.DEFAULT_REGIONSERVER_INFOPORT,
defaultCluster.getConfiguration().getInt(HConstants.REGIONSERVER_INFO_PORT, 0));
assertEquals(masterHostPort,
defaultCluster.getConfiguration().get(HConstants.MASTER_ADDRS_KEY));
} finally {
htu.shutdownMiniCluster();
}
Expand All @@ -464,10 +468,14 @@ public void testOverridingOfDefaultPorts() throws Exception {
htu = new HBaseTestingUtility(altConfig);
try {
MiniHBaseCluster customCluster = htu.startMiniCluster();
final String masterHostPort =
customCluster.getMaster().getServerName().getAddress().toString();
assertEquals(nonDefaultMasterInfoPort,
customCluster.getConfiguration().getInt(HConstants.MASTER_INFO_PORT, 0));
customCluster.getConfiguration().getInt(HConstants.MASTER_INFO_PORT, 0));
assertEquals(nonDefaultRegionServerPort,
customCluster.getConfiguration().getInt(HConstants.REGIONSERVER_INFO_PORT, 0));
assertEquals(masterHostPort,
customCluster.getConfiguration().get(HConstants.MASTER_ADDRS_KEY));
} finally {
htu.shutdownMiniCluster();
}
Expand Down