fengnanli commented on a change in pull request #3346:
URL: https://github.com/apache/hadoop/pull/3346#discussion_r697812403
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/NNHAServiceTarget.java
##########
@@ -56,9 +56,25 @@
private final String nnId;
private final String nsId;
private final boolean autoFailoverEnabled;
-
+
public NNHAServiceTarget(Configuration conf,
String nsId, String nnId) {
+ this(conf, nsId, nnId, null, null, null);
+ }
+
+ /**
+ * Create a NNHAServiceTarget for a namenode.
+ *
+ * @param conf HDFS configuration.
+ * @param nsId nsId of this nn.
+ * @param nnId nnId of this nn.
+ * @param serviceAddr Provided service address.
+ * @param addr Provided service address.
+ * @param lifelineAddr Provided service address.
+ */
+ public NNHAServiceTarget(Configuration conf,
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
