Skip to content

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

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
ayushtkn 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 2 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
31 changes: 29 additions & 2 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 @@ -52,6 +52,8 @@ public static class Builder {
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 +78,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 +111,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 +198,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
67 changes: 67 additions & 0 deletions ...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 @@ -31,6 +31,7 @@


public class TestMiniJournalCluster {

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

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

try {
new MiniJournalCluster.Builder(conf).setHttpPorts(8481).build();
fail("Should not reach here");
} catch (IllegalArgumentException e) {
assertEquals("Num of http ports (1) should match num of JournalNodes (3)", e.getMessage());
}

try {
new MiniJournalCluster.Builder(conf).setRpcPorts(8481, 8482)
.build();
fail("Should not reach here");
} catch (IllegalArgumentException e) {
assertEquals("Num of rpc ports (2) should match num of JournalNodes (3)", e.getMessage());
}

try {
new MiniJournalCluster.Builder(conf).setHttpPorts(800, 9000, 10000).setRpcPorts(8481)
.build();
fail("Should not reach here");
} catch (IllegalArgumentException e) {
assertEquals("Num of rpc ports (1) should match num of JournalNodes (3)", e.getMessage());
}

try {
new MiniJournalCluster.Builder(conf).setHttpPorts(800, 9000, 1000, 2000)
.setRpcPorts(8481, 8482, 8483)
.build();
fail("Should not reach here");
} catch (IllegalArgumentException e) {
assertEquals("Num of http ports (4) should match num of JournalNodes (3)", e.getMessage());
}

MiniJournalCluster miniJournalCluster =
new MiniJournalCluster.Builder(conf).setHttpPorts(8481, 8482, 8483)
.setRpcPorts(8491, 8492, 8493).build();
Copy link
Contributor Author

@virajjasani virajjasani Mar 1, 2022

Choose a reason for hiding this comment

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

@tomscut this is the only UT that will use setHttpPorts and setRpcPorts to set custom ports and then asserts that correct ports are used, hence this UT should not collide with any other UTs. Besides, we must have some UT to ensure both setHttpPorts and setRpcPorts are working as expected, correct?
Thanks

Copy link
Member

@ayushtkn ayushtkn Mar 1, 2022

Choose a reason for hiding this comment

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

hence this UT should not collide with any other UTs.

I have similar concerns, We are hard coding the ports here, this test can fail in specific environments where these ports are already occupied. We have seen such cases in past..
--> Just had a cursory look

Copy link
Contributor Author

@virajjasani virajjasani Mar 1, 2022

Choose a reason for hiding this comment

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

Thanks. @ayushtkn I just wanted to see if this UT with hardcoded ports pass in Jenkins build and since it did, is it good enough for us to keep? If not, it's fine and I can try to randomize the ports generation (similar to default case) and assert that randomly chosen port is the one being used by MiniJournal cluster.

Copy link
Member

@ayushtkn ayushtkn Mar 1, 2022

Choose a reason for hiding this comment

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

We can not rely completely on Jenkins, I remember seeing an issue with a RBF test, where we had a port hard-coded, and for some organisation, there internal build was failing for that test, because they had something running on that port.

May be first finding some free ports, and then putting them into this conf should do. Or if that isn't possible, second option is try some randomisation, get a set of some random ports in a specified range, if they work great, if not loop back find another set and so on for some specified amount of iterations.

If above two doesn't work, we can try some skipping mechanism, like ports are occupied so skip the test or so, But this would be the last and the worst thing to do.

And in Jenkins the tests run in parallel, so results might change depending on what tests are running together & what ports they randomly choose.

My general experience so far, such controversial tests don't fail in the actual PR, but in other folks PR. :-P
Better we play safe. :)

Copy link
Contributor Author

@virajjasani virajjasani Mar 1, 2022

Choose a reason for hiding this comment

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

We can not rely completely on Jenkins, I remember seeing an issue with a RBF test, where we had a port hard-coded, and for some organisation, there internal build was failing for that test, because they had something running on that port.

Valid point!

try {
miniJournalCluster.waitActive();
URI uri = miniJournalCluster.getQuorumJournalURI("myjournal");
String[] addrs = uri.getAuthority().split(";");
assertEquals(3, addrs.length);

assertEquals(8481, miniJournalCluster.getJournalNode(0).getHttpAddress().getPort());
assertEquals(8482, miniJournalCluster.getJournalNode(1).getHttpAddress().getPort());
assertEquals(8483, miniJournalCluster.getJournalNode(2).getHttpAddress().getPort());

assertEquals(8491,
miniJournalCluster.getJournalNode(0).getRpcServer().getAddress().getPort());
assertEquals(8492,
miniJournalCluster.getJournalNode(1).getRpcServer().getAddress().getPort());
assertEquals(8493,
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);
} finally {
miniJournalCluster.shutdown();
}
}

}