-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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.
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
HDFS-16481. Provide support to set Http and Rpc ports in MiniJournalCluster #4028
Conversation
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
MiniJournalCluster miniJournalCluster = | ||
new MiniJournalCluster.Builder(conf).setHttpPorts(8481, 8482, 8483) | ||
.setRpcPorts(8491, 8492, 8493).build(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
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!
@ayushtkn could you please take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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,
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Done, thanks.
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.
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());
}
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎊 +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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let me take care of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 Pending Jenkins
Will commit by tomorrow EOD, if no objections
for (int i = 0; i < 3; i++) { | ||
assertNotEquals(0, rpcPorts[i]); | ||
assertNotEquals(0, httpPorts[i]); | ||
} |
There was a problem hiding this comment.
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));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, Thanks.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
|
Thanx @tomscut & @virajjasani |
…luster (apache#4028). Contributed by Viraj Jasani.
…luster (apache#4028). Contributed by Viraj Jasani.
…luster (apache#4028). Contributed by Viraj Jasani.
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: