-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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-16188. RBF: Router to support resolving monitored namenodes with DNS #3346
Conversation
💔 -1 overall
This message was automatically generated. |
* @param addr Provided service address. | ||
* @param lifelineAddr Provided service address. | ||
*/ | ||
public NNHAServiceTarget(Configuration conf, |
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.
Instead of overloading the constructor like this, can we have the common part extracted as a function and call it inside? Having a lot of null check for parameters feels not ideal:
NNHAServiceTarget(Configuration conf, String nsId, String nnId) {
// do addrs assignment from config
.....
assignAutofailoverAndFencer();
}
NNHAServiceTarget(Configuration conf, String nsId, String nnId, String serviceAddr, addr, lifelineAddr) {
// do assignment from param
....
assignAutofailoverAndFencer();
}
Also please add a simple test for it.
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.
Yeah this will look cleaner. I will need to remove the final key word from some vars as the assignment is out of the constructor.
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.
Added a simple test per comment.
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSUtilClient.java
Outdated
Show resolved
Hide resolved
hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSUtilClient.java
Outdated
Show resolved
Hide resolved
hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSUtilClient.java
Show resolved
Hide resolved
.../src/main/java/org/apache/hadoop/hdfs/server/federation/router/NamenodeHeartbeatService.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/hadoop/hdfs/server/federation/router/NamenodeHeartbeatService.java
Outdated
Show resolved
Hide resolved
...c/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterNamenodeHeartbeat.java
Outdated
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
} | ||
} catch (UnknownHostException e) { | ||
LOG.error("Failed to resolve address: " + address); | ||
ret.putAll(getResolvedAddressesForNnId( |
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 would extract the output of the method before doing putAll().
If something breaks, it is easier to debug if it points to the exact line too.
* This is mainly used when configured nn is | ||
* a domain record that has multiple hosts behind it. | ||
*/ | ||
static String getConcatNnId(String nsId, String nnId, String hostname, int port) { |
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.
private?
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.
Add the args to the javadoc too.
if (this.serviceAddress == null) { | ||
LOG.error("Cannot locate RPC service address for NN {}, " + | ||
"using RPC address {}", nnDesc, this.rpcAddress); | ||
this.serviceAddress = this.rpcAddress; | ||
} | ||
if (resolvedHost != null) { |
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 do the same thing over and over for the lifeline and the others.
Maybe do all of them in a single shot?
The way to extract the port might also be expensive to be honest; creating a socket address is usually bad.
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.
Sure, that will be more clear.
Maybe I will just do a String split with ":"? The format coming from config should be ok.
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.
There might be already a function to do the split and make it resilient without having to set a socket.
Take a look at NetUtils to see if there is something.
If there is not, please create a static function that does the split and adds some checks
💔 -1 overall
This message was automatically generated. |
if (resolvedHost != null) { | ||
// Get the addresses from resolvedHost plus the configured ports. | ||
rpcAddress = resolvedHost + ":" | ||
+ rpcAddress.split(":")[1]; |
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.
Try to use something from NetUtils that doesn't open a socket and if it's not there create a method for extractPort or something.
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.
Sure, will add a simple method there to parse the host:port string.
assertEquals(1002, NetUtils.getPortFromHostPortString("testHost:1002")); | ||
try { | ||
NetUtils.getPortFromHostPortString("testHost"); | ||
fail("Should throw exception for wrong format"); |
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.
LambdaTestUtils#intercept does 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.
thx for the pointer, its pretty handy!
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Thanks for the contribution from @LeonGao91 and review from @goiri ! |
thanks for the reviews @goiri @fengnanli ! |
… DNS (apache#3346) Contributed by Leon Gao * Router to support resolving monitored namenodes with DNS * Style * fix style and test failure * Add test for NNHAServiceTarget const * Resolve comments * Fix test * Comments and style * Create a simple function to extract port * Use LambdaTestUtils.intercept * fix javadoc * Trigger Build
Description of PR
How was this patch tested?
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?