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

HDFS-16481. Provide support to set Http and Rpc ports in MiniJournalCluster #4028

Merged
merged 7 commits into from
Mar 4, 2022
Merged

HDFS-16481. Provide support to set Http and Rpc ports in MiniJournalCluster #4028

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
948305f
HDFS-16481. Provide support to set Http and Rpc ports in MiniJournalC…
virajjasani Feb 24, 2022
5833956
fix checkstyle
virajjasani Feb 24, 2022
e70c311
addressing reviews from @ayushtkn and @tomscut
virajjasani Mar 1, 2022
24fa464
addressing recent review
virajjasani Mar 1, 2022
bcf8a15
adding new utility to acquire free ports
virajjasani Mar 2, 2022
68e7e2e
exclude 0 in the free ports
virajjasani Mar 3, 2022
84e430e
assert all acquire ports
virajjasani Mar 3, 2022
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
41 changes: 38 additions & 3 deletions ...project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/qjournal/MiniJournalCluster.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import static org.apache.hadoop.hdfs.qjournal.QJMTestUtil.FAKE_NSINFO;
import static org.junit.Assert.fail;

import java.io.Closeable;
import java.io.File;
import java.io.IOException;
import java.net.InetSocketAddress;
Expand All @@ -45,13 +46,16 @@
import org.apache.hadoop.thirdparty.com.google.common.base.Joiner;
import org.apache.hadoop.test.GenericTestUtils;

public class MiniJournalCluster {
public class MiniJournalCluster implements Closeable {

public static final String CLUSTER_WAITACTIVE_URI = "waitactive";
public static class Builder {
private String baseDir;
private int numJournalNodes = 3;
private boolean format = true;
private final Configuration conf;
private int[] httpPorts = null;
private int[] rpcPorts = null;

static {
DefaultMetricsSystem.setMiniClusterMode(true);
Expand All @@ -76,6 +80,16 @@ public Builder format(boolean f) {
return this;
}

public Builder setHttpPorts(int... ports) {
this.httpPorts = ports;
return this;
}

public Builder setRpcPorts(int... ports) {
this.rpcPorts = ports;
return this;
}

public MiniJournalCluster build() throws IOException {
return new MiniJournalCluster(this);
}
Expand All @@ -99,6 +113,19 @@ private JNInfo(JournalNode node) {
private final JNInfo[] nodes;

private MiniJournalCluster(Builder b) throws IOException {

if (b.httpPorts != null && b.httpPorts.length != b.numJournalNodes) {
throw new IllegalArgumentException(
"Num of http ports (" + b.httpPorts.length + ") should match num of JournalNodes ("
+ b.numJournalNodes + ")");
}

if (b.rpcPorts != null && b.rpcPorts.length != b.numJournalNodes) {
throw new IllegalArgumentException(
"Num of rpc ports (" + b.rpcPorts.length + ") should match num of JournalNodes ("
+ b.numJournalNodes + ")");
}

LOG.info("Starting MiniJournalCluster with " +
b.numJournalNodes + " journal nodes");

Expand Down Expand Up @@ -173,8 +200,10 @@ private Configuration createConfForNode(Builder b, int idx) {
Configuration conf = new Configuration(b.conf);
File logDir = getStorageDir(idx);
conf.set(DFSConfigKeys.DFS_JOURNALNODE_EDITS_DIR_KEY, logDir.toString());
conf.set(DFSConfigKeys.DFS_JOURNALNODE_RPC_ADDRESS_KEY, "localhost:0");
conf.set(DFSConfigKeys.DFS_JOURNALNODE_HTTP_ADDRESS_KEY, "localhost:0");
int httpPort = b.httpPorts != null ? b.httpPorts[idx] : 0;
int rpcPort = b.rpcPorts != null ? b.rpcPorts[idx] : 0;
conf.set(DFSConfigKeys.DFS_JOURNALNODE_RPC_ADDRESS_KEY, "localhost:" + rpcPort);
conf.set(DFSConfigKeys.DFS_JOURNALNODE_HTTP_ADDRESS_KEY, "localhost:" + httpPort);
return conf;
}

Expand Down Expand Up @@ -274,4 +303,10 @@ public void setNamenodeSharedEditsConf(String jid) {
.DFS_NAMENODE_SHARED_EDITS_DIR_KEY, quorumJournalURI.toString());
}
}

@Override
public void close() throws IOException {
this.shutdown();
}

}
87 changes: 86 additions & 1 deletion ...ect/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/qjournal/TestMiniJournalCluster.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,17 @@
import org.apache.hadoop.hdfs.DFSConfigKeys;
import org.apache.hadoop.hdfs.MiniDFSCluster;
import org.apache.hadoop.hdfs.qjournal.server.JournalNode;
import org.junit.Test;
import org.apache.hadoop.net.NetUtils;
import org.apache.hadoop.test.LambdaTestUtils;

import org.junit.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class TestMiniJournalCluster {

private static final Logger LOG = LoggerFactory.getLogger(TestMiniJournalCluster.class);

@Test
public void testStartStop() throws IOException {
Configuration conf = new Configuration();
Expand All @@ -52,4 +59,82 @@ public void testStartStop() throws IOException {
c.shutdown();
}
}

@Test
public void testStartStopWithPorts() throws Exception {
Configuration conf = new Configuration();

LambdaTestUtils.intercept(
IllegalArgumentException.class,
"Num of http ports (1) should match num of JournalNodes (3)",
"MiniJournalCluster port validation failed",
() -> {
new MiniJournalCluster.Builder(conf).setHttpPorts(8481).build();
});

LambdaTestUtils.intercept(
IllegalArgumentException.class,
"Num of rpc ports (2) should match num of JournalNodes (3)",
"MiniJournalCluster port validation failed",
() -> {
new MiniJournalCluster.Builder(conf).setRpcPorts(8481, 8482).build();
});

LambdaTestUtils.intercept(
IllegalArgumentException.class,
"Num of rpc ports (1) should match num of JournalNodes (3)",
"MiniJournalCluster port validation failed",
() -> {
new MiniJournalCluster.Builder(conf).setHttpPorts(800, 9000, 10000).setRpcPorts(8481)
.build();
});

LambdaTestUtils.intercept(
IllegalArgumentException.class,
"Num of http ports (4) should match num of JournalNodes (3)",
"MiniJournalCluster port validation failed",
() -> {
new MiniJournalCluster.Builder(conf).setHttpPorts(800, 9000, 1000, 2000)
.setRpcPorts(8481, 8482, 8483).build();
});

final int[] httpPorts = new int[] { NetUtils.getFreeSocketPort(), NetUtils.getFreeSocketPort(),
NetUtils.getFreeSocketPort() };
final int[] rpcPorts = new int[] { NetUtils.getFreeSocketPort(), NetUtils.getFreeSocketPort(),
NetUtils.getFreeSocketPort() };
Copy link
Member

Choose a reason for hiding this comment

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

This won't work in all cases, the same port can be returned, say you are calling NetUtils.getFreeSocketPort() 6 times, it is possible it won't return 6 different ports, it can have dupes.

Can have a util method added in NetUtils, Something like this(Just for idea, can improve/fix)

  /**
   * Return a set of free ports. There is no guarantee it will remain free, so
   *    * it should be used immediately.
   * @param numPorts number of free ports required.
   * @return a set of unique ports.
   * @throws IOException in case unable to find the required number of free ports.
   */
  public static Set<Integer> getFreeSocketPorts(int numPorts)
      throws IOException {
    Set<Integer> ports = new HashSet<>();
    for (int numAttempt = 0;
         ports.size() != numPorts && numAttempt < 10 * numPorts; numAttempt++) {
      int port = getFreeSocketPort();
      if (port > 0) {
        ports.add(port);
      }
    }
    if (ports.size() != numPorts) {
      throw new IOException("Couldn't find " + numPorts + " free ports");
    }
    return ports;
  }

And we can consume the output of this method.
If possible, Please extend a test in TestNetUtils as well. A simple test should do:


  @Test 
  public void testGetFreePorts() throws IOException {
    assertEquals(6, NetUtils.getFreeSocketPorts(6).size());
  }

Copy link
Contributor Author

@virajjasani virajjasani Mar 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I see the exact increments in the ports as per the logs:

2022-03-01 20:08:22,504 [Listener at localhost/58862] INFO  qjournal.TestMiniJournalCluster (TestMiniJournalCluster.java:testStartStopWithPorts(106)) - Http ports selected: [58873, 58874, 58875]
2022-03-01 20:08:22,504 [Listener at localhost/58862] INFO  qjournal.TestMiniJournalCluster (TestMiniJournalCluster.java:testStartStopWithPorts(107)) - Rpc ports selected: [58876, 58877, 58878]


2022-03-01 21:53:55,768 [Listener at localhost/59292] INFO  qjournal.TestMiniJournalCluster (TestMiniJournalCluster.java:testStartStopWithPorts(106)) - Http ports selected: [59302, 59303, 59304]
2022-03-01 21:53:55,768 [Listener at localhost/59292] INFO  qjournal.TestMiniJournalCluster (TestMiniJournalCluster.java:testStartStopWithPorts(107)) - Rpc ports selected: [59305, 59306, 59307]

And so on..

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure about it, would need a second opinion, from the javadoc of the comment, it feels like it can return same ports.
And logically say port X is the only free port, will it not return port X, if called again. Not sure how does this behave in different OS.
Tried something like this:

    HashSet<Integer> hs = new LinkedHashSet();
    while(true) {
      int port = NetUtils.getFreeSocketPort();
      if(!hs.add(port)) {
        throw new IOException("Dupe Port "+ port + " ports : " + hs + " size " + hs.size());
      }

it threw me an exception on MacOs with higher hs.size and on Ubuntu with lesser. not sure if there is some logic behind it or not, or just machine specific. Let me see if I can ask some N/W expert internally
@tomscut in case you have any opinion here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @ayushtkn for ping me and your advice.

NetUtils.getFreeSocketPort() returns the currently freeport, which may be occupied if not immediately used.

If we set the port to 0, the system will automatically assign a free port for us. Then we'll see which ports are actually being used and do assert. Is that ok? As @virajjasani mentioned here .

Please point out if I understand wrong. Thanks. :)

Copy link
Contributor Author

@virajjasani virajjasani Mar 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@ayushtkn @tomscut There is nothing wrong with providing the utility as mentioned by Ayush above. I believe it is better to fail the test if we can not get 6 free ports for this UT and once we do get free ports, we can go ahead with the UT. I will make the change.


LOG.info("Http ports selected: {}", httpPorts);
LOG.info("Rpc ports selected: {}", rpcPorts);

for (int i = 0; i < 3; i++) {
assertNotEquals(0, rpcPorts[i]);
assertNotEquals(0, httpPorts[i]);
}
Copy link
Member

Choose a reason for hiding this comment

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

you could have asserted directly on httpAndRpcPorts. :-)

    assertFalse(httpAndRpcPorts.contains(0));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, Thanks.


try (MiniJournalCluster miniJournalCluster = new MiniJournalCluster.Builder(conf)
.setHttpPorts(httpPorts)
.setRpcPorts(rpcPorts).build()) {
miniJournalCluster.waitActive();
URI uri = miniJournalCluster.getQuorumJournalURI("myjournal");
String[] addrs = uri.getAuthority().split(";");
assertEquals(3, addrs.length);

assertEquals(httpPorts[0], miniJournalCluster.getJournalNode(0).getHttpAddress().getPort());
assertEquals(httpPorts[1], miniJournalCluster.getJournalNode(1).getHttpAddress().getPort());
assertEquals(httpPorts[2], miniJournalCluster.getJournalNode(2).getHttpAddress().getPort());

assertEquals(rpcPorts[0],
miniJournalCluster.getJournalNode(0).getRpcServer().getAddress().getPort());
assertEquals(rpcPorts[1],
miniJournalCluster.getJournalNode(1).getRpcServer().getAddress().getPort());
assertEquals(rpcPorts[2],
miniJournalCluster.getJournalNode(2).getRpcServer().getAddress().getPort());

JournalNode node = miniJournalCluster.getJournalNode(0);
String dir = node.getConf().get(DFSConfigKeys.DFS_JOURNALNODE_EDITS_DIR_KEY);
assertEquals(new File(MiniDFSCluster.getBaseDirectory() + "journalnode-0").getAbsolutePath(),
dir);
}
}

}