HDFS-16481. Provide support to set Http and Rpc ports in MiniJournalCluster #4028
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
tomscut
reviewed
Feb 28, 2022
There was a problem hiding this comment.
Hi @virajjasani , will this fixed port increase the probability of port occupation, resulting in the interruption of unit test?
@tomscut only one UT is going to use this ability, rest of the UTs are still going to use default mode (i.e. random port usage). Hence this is more like feature for JN downstream applications. |
Thank you @virajjasani for your explanation. I'm not sure whether the UT you mentioned can be verified in other ways.
|
virajjasani
commented
Mar 1, 2022
Comment on lines
93
to
95
| MiniJournalCluster miniJournalCluster = | ||
| new MiniJournalCluster.Builder(conf).setHttpPorts(8481, 8482, 8483) | ||
| .setRpcPorts(8491, 8492, 8493).build(); |
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
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!
|
@ayushtkn could you please take a look? |
ayushtkn
reviewed
Mar 1, 2022
There was a problem hiding this comment.
In general the code looks good. One concern regarding the test going flaky since we are using hard-coded ports.
For other tests where you are asserting on the exceptions, Can you please switch to LambdaTestUtils.intercept.
For the case, where you are expecting to get the cluster started and doing miniJournalCluster.shutdown();, may be using try-with resources would be better,
ayushtkn
reviewed
Mar 1, 2022
Comment on lines
100
to
115
| final int[] httpPorts = new int[3]; | ||
| final int[] rpcPorts = new int[3]; | ||
| try (MiniJournalCluster miniJournalCluster = new MiniJournalCluster.Builder(conf).build()) { | ||
| miniJournalCluster.waitActive(); | ||
|
|
||
| for (int i = 0; i < 3; i++) { | ||
| httpPorts[i] = miniJournalCluster.getJournalNode(i).getHttpAddress().getPort(); | ||
| } | ||
|
|
||
| for (int i = 0; i < 3; i++) { | ||
| rpcPorts[i] = miniJournalCluster.getJournalNode(i).getRpcServer().getAddress().getPort(); | ||
| } | ||
| } | ||
|
|
||
| LOG.info("Http ports selected: {}", httpPorts); | ||
| LOG.info("Rpc ports selected: {}", rpcPorts); |
There was a problem hiding this comment.
Is this to just get the free ports?
Like start the cluster with 0 -> get the ports -> Shutdown the cluster(Ports are free)-> Now use them explicitly
If that is the case, You can explore using NetUtils.getFreeSocketPort(), that would fetch you free ports, which you can pass as arguments.
There was a problem hiding this comment.
Sounds good. Done, thanks.
ayushtkn
reviewed
Mar 1, 2022
Comment on lines
101
to
104
| final int[] httpPorts = new int[] { NetUtils.getFreeSocketPort(), NetUtils.getFreeSocketPort(), | ||
| NetUtils.getFreeSocketPort() }; | ||
| final int[] rpcPorts = new int[] { NetUtils.getFreeSocketPort(), NetUtils.getFreeSocketPort(), | ||
| NetUtils.getFreeSocketPort() }; |
There was a problem hiding this comment.
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());
}
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@ayushtkn @tomscut With the latest changes, here is the result of a stretch test on MacOS: The patch used: |
|
🎊 +1 overall
This message was automatically generated. |
ayushtkn
reviewed
Mar 3, 2022
| public static Set<Integer> getFreeSocketPorts(int numOfPorts) { | ||
| final Set<Integer> freePorts = new HashSet<>(numOfPorts); | ||
| for (int i = 0; i < numOfPorts * 5; i++) { | ||
| freePorts.add(getFreeSocketPort()); |
There was a problem hiding this comment.
getFreeSocketPort() as per code can return 0 as well, we should avoid that? We can't classify 0 as a unique free port.
if it returns 0, then your test would also fail. you would be expecting the port to be 0, but it would be different
There was a problem hiding this comment.
Yes, let me take care of this.
ayushtkn
approved these changes
Mar 3, 2022
Comment on lines
120
to
123
| for (int i = 0; i < 3; i++) { | ||
| assertNotEquals(0, rpcPorts[i]); | ||
| assertNotEquals(0, httpPorts[i]); | ||
| } |
There was a problem hiding this comment.
you could have asserted directly on httpAndRpcPorts. :-)
assertFalse(httpAndRpcPorts.contains(0));
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
|
|
Thanx @tomscut & @virajjasani |
1 task
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…luster (apache#4028). Contributed by Viraj Jasani.
LiuGuH
pushed a commit
to LiuGuH/hadoop
that referenced
this pull request
Mar 29, 2024
…luster (apache#4028). Contributed by Viraj Jasani.
LiuGuH
pushed a commit
to LiuGuH/hadoop
that referenced
this pull request
Jul 4, 2024
…luster (apache#4028). Contributed by Viraj Jasani.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
We should provide support for clients to set Http and Rpc ports of JournalNodes in MiniJournalCluster.
How was this patch tested?
Using a downstreamer application as well as using UT.
For code changes: