You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@twill.apache.org by piaosama <gi...@git.apache.org> on 2018/09/29 06:51:51 UTC
[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...
GitHub user piaosama opened a pull request:
https://github.com/apache/twill/pull/71
TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is removed from DFSUtils from hadoop-2.8
TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is removed from DFSUtils from hadoop-2.8
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/piaosama/twill master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/twill/pull/71.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #71
----
commit e1e58009c7a40d665ed80b78ebe3e1c3d2aa0f3b
Author: lihongyuan <li...@...>
Date: 2018-09-29T06:49:47Z
TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is removed from DFSUtils from hadoop-2.8
----
---
[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/71#discussion_r221660324
--- Diff: twill-yarn/src/main/java/org/apache/twill/internal/yarn/YarnUtils.java ---
@@ -202,6 +223,28 @@ public static void cloneHaNnCredentials(Configuration config) throws IOException
}
}
+ /***
+ * When hadoop_version > 2.8.0, class DFSUtils has no method getHaNnRpcAddresses(Configuration config)
+ * @param config
+ * @return
+ */
+ private static Set<Map.Entry<String, Map<String, InetSocketAddress>>> getEntries(Configuration config) {
--- End diff --
Please name this method with a more specific name, such as `getHaNnRpcAddress`.
---
[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/71#discussion_r221661290
--- Diff: twill-yarn/src/main/java/org/apache/twill/internal/yarn/YarnUtils.java ---
@@ -202,6 +223,28 @@ public static void cloneHaNnCredentials(Configuration config) throws IOException
}
}
+ /***
+ * When hadoop_version > 2.8.0, class DFSUtils has no method getHaNnRpcAddresses(Configuration config)
+ * @param config
+ * @return
+ */
+ private static Set<Map.Entry<String, Map<String, InetSocketAddress>>> getEntries(Configuration config) {
+ return iDFSUtilClientExists ? invoke(config) :
+ DFSUtil.getHaNnRpcAddresses(config).entrySet();
+ }
+
+ private static Set<Map.Entry<String, Map<String, InetSocketAddress>>> invoke(Configuration config) {
+ try {
+ return ((Map) getHaNnRpcAddressesMethod.invoke(null, config)).entrySet();
--- End diff --
Why casting to `Map` while the return type of this method is `Set`??
---
[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...
Posted by piaosama <gi...@git.apache.org>.
Github user piaosama commented on a diff in the pull request:
https://github.com/apache/twill/pull/71#discussion_r223181337
--- Diff: twill-yarn/src/main/java/org/apache/twill/internal/yarn/YarnUtils.java ---
@@ -72,7 +77,23 @@
HADOOP_26
}
- private static final Logger LOG = LoggerFactory.getLogger(YarnUtils.class);
+ private static boolean iDFSUtilClientExists = false; // use this to judge if the hadoop version is above 2.8
--- End diff --
ok， suddenly find HAUtil class does not exists as the DFSUtils class. i will add the two judge into the code and make the code more nice as you commented.
---
[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...
Posted by piaosama <gi...@git.apache.org>.
Github user piaosama commented on a diff in the pull request:
https://github.com/apache/twill/pull/71#discussion_r225817747
--- Diff: twill-yarn/src/main/java/org/apache/twill/internal/yarn/YarnUtils.java ---
@@ -202,6 +223,28 @@ public static void cloneHaNnCredentials(Configuration config) throws IOException
}
}
+ /***
+ * When hadoop_version > 2.8.0, class DFSUtils has no method getHaNnRpcAddresses(Configuration config)
--- End diff --
https://github.com/apache/hadoop/commit/d12a0a25b4da1957ba0f658694729d2f2d400c5f#diff-f42f0d7039ba6a4ce427c67ae74dd7ec
HDFS-11538. Move ClientProtocol HA proxies into hadoop-hdfs-client said this.
---
[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...
Posted by piaosama <gi...@git.apache.org>.
Github user piaosama commented on a diff in the pull request:
https://github.com/apache/twill/pull/71#discussion_r223181489
--- Diff: twill-yarn/src/main/java/org/apache/twill/internal/yarn/YarnUtils.java ---
@@ -202,6 +223,28 @@ public static void cloneHaNnCredentials(Configuration config) throws IOException
}
}
+ /***
+ * When hadoop_version > 2.8.0, class DFSUtils has no method getHaNnRpcAddresses(Configuration config)
+ * @param config
+ * @return
+ */
+ private static Set<Map.Entry<String, Map<String, InetSocketAddress>>> getEntries(Configuration config) {
+ return iDFSUtilClientExists ? invoke(config) :
+ DFSUtil.getHaNnRpcAddresses(config).entrySet();
+ }
+
+ private static Set<Map.Entry<String, Map<String, InetSocketAddress>>> invoke(Configuration config) {
+ try {
+ return ((Map) getHaNnRpcAddressesMethod.invoke(null, config)).entrySet();
--- End diff --
getHaNnRpcAddressesMethod invoke return Map instance.
---
[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...
Posted by piaosama <gi...@git.apache.org>.
Github user piaosama commented on a diff in the pull request:
https://github.com/apache/twill/pull/71#discussion_r225824325
--- Diff: twill-yarn/src/main/java/org/apache/twill/internal/yarn/YarnUtils.java ---
@@ -202,6 +223,28 @@ public static void cloneHaNnCredentials(Configuration config) throws IOException
}
}
+ /***
+ * When hadoop_version > 2.8.0, class DFSUtils has no method getHaNnRpcAddresses(Configuration config)
--- End diff --
https://github.com/apache/hadoop/commit/f02ca4ab158aa2257e839a1f74bc8254e1a3d61b#diff-f42f0d7039ba6a4ce427c67ae74dd7ec
HDFS-8185 Separate client related routines in HAUtil into a new clas also said this.
---
[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/71#discussion_r221661663
--- Diff: twill-yarn/src/main/java/org/apache/twill/internal/yarn/YarnUtils.java ---
@@ -345,6 +388,28 @@ private static FileSystem getFileSystem(LocationFactory locationFactory) throws
return null;
}
+ private static void handleLogAction(Exception e) {
--- End diff --
This method is unnecessary, better log it in place. E.g. in the static initializer above:
```java
static {
try {
Class dfsUtilsClientClazz = Class.forName("org.apache.hadoop.hdfs.DFSUtilClient");
getHaNnRpcAddressesMethod = dfsUtilsClientClazz.getMethod("getHaNnRpcAddresses",
Configuration.class);
hasDFSUtilClient = true;
} catch (ClassNotFoundException e) {
// Expected for Hadoop version < 2.8, hence log it as debug only to no polluting the logs
LOG.debug("No DFSUtilClient found", e);
} catch (NoSuchMethodException e) {
// This is unexpected for not founding the getHaNnRpcAddresses method if the DFSUtilClient class exists
LOG.warn("No DFSUtilClient.getHaNnRpcAddresses method found. Getting HA NameNode address might fail.", e);
}
}
```
---
[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...
Posted by piaosama <gi...@git.apache.org>.
Github user piaosama commented on a diff in the pull request:
https://github.com/apache/twill/pull/71#discussion_r223182267
--- Diff: twill-yarn/src/main/java/org/apache/twill/internal/yarn/YarnUtils.java ---
@@ -345,6 +388,28 @@ private static FileSystem getFileSystem(LocationFactory locationFactory) throws
return null;
}
+ private static void handleLogAction(Exception e) {
--- End diff --
add new code, modify the code as you suggested and add the HAutils method judgement.
---
[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/twill/pull/71
---
[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/71#discussion_r221660130
--- Diff: twill-yarn/src/main/java/org/apache/twill/internal/yarn/YarnUtils.java ---
@@ -202,6 +223,28 @@ public static void cloneHaNnCredentials(Configuration config) throws IOException
}
}
+ /***
+ * When hadoop_version > 2.8.0, class DFSUtils has no method getHaNnRpcAddresses(Configuration config)
+ * @param config
+ * @return
+ */
+ private static Set<Map.Entry<String, Map<String, InetSocketAddress>>> getEntries(Configuration config) {
+ return iDFSUtilClientExists ? invoke(config) :
+ DFSUtil.getHaNnRpcAddresses(config).entrySet();
+ }
+
+ private static Set<Map.Entry<String, Map<String, InetSocketAddress>>> invoke(Configuration config) {
--- End diff --
Please name this method with a more appropriate name, rather than a generic name `invoke`.
---
[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/71#discussion_r221663573
--- Diff: twill-yarn/src/main/java/org/apache/twill/internal/yarn/YarnUtils.java ---
@@ -72,7 +77,23 @@
HADOOP_26
}
- private static final Logger LOG = LoggerFactory.getLogger(YarnUtils.class);
+ private static boolean iDFSUtilClientExists = false; // use this to judge if the hadoop version is above 2.8
--- End diff --
What does the `i` prefix mean? Better just call this `hasDFSUtilClient`.
---
[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...
Posted by piaosama <gi...@git.apache.org>.
Github user piaosama commented on a diff in the pull request:
https://github.com/apache/twill/pull/71#discussion_r223181498
--- Diff: twill-yarn/src/main/java/org/apache/twill/internal/yarn/YarnUtils.java ---
@@ -202,6 +223,28 @@ public static void cloneHaNnCredentials(Configuration config) throws IOException
}
}
+ /***
+ * When hadoop_version > 2.8.0, class DFSUtils has no method getHaNnRpcAddresses(Configuration config)
+ * @param config
+ * @return
+ */
+ private static Set<Map.Entry<String, Map<String, InetSocketAddress>>> getEntries(Configuration config) {
+ return iDFSUtilClientExists ? invoke(config) :
+ DFSUtil.getHaNnRpcAddresses(config).entrySet();
+ }
+
+ private static Set<Map.Entry<String, Map<String, InetSocketAddress>>> invoke(Configuration config) {
--- End diff --
ok
---
[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...
Posted by piaosama <gi...@git.apache.org>.
Github user piaosama commented on a diff in the pull request:
https://github.com/apache/twill/pull/71#discussion_r223181503
--- Diff: twill-yarn/src/main/java/org/apache/twill/internal/yarn/YarnUtils.java ---
@@ -202,6 +223,28 @@ public static void cloneHaNnCredentials(Configuration config) throws IOException
}
}
+ /***
+ * When hadoop_version > 2.8.0, class DFSUtils has no method getHaNnRpcAddresses(Configuration config)
+ * @param config
+ * @return
+ */
+ private static Set<Map.Entry<String, Map<String, InetSocketAddress>>> getEntries(Configuration config) {
--- End diff --
ok
---
[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/71#discussion_r221661448
--- Diff: twill-yarn/src/main/java/org/apache/twill/internal/yarn/YarnUtils.java ---
@@ -202,6 +223,28 @@ public static void cloneHaNnCredentials(Configuration config) throws IOException
}
}
+ /***
+ * When hadoop_version > 2.8.0, class DFSUtils has no method getHaNnRpcAddresses(Configuration config)
--- End diff --
This is not a very clear javadoc that describe what this method does (it should have the same javadoc as the `DFSUtil.getHaNnRpcAddresses` method).
---
[GitHub] twill issue #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUtil#getHa...
Posted by piaosama <gi...@git.apache.org>.
Github user piaosama commented on the issue:
https://github.com/apache/twill/pull/71
add new code, modify the code as you suggested and add the HAutils method judgement.
---
[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/71#discussion_r221660462
--- Diff: twill-yarn/src/main/java/org/apache/twill/internal/yarn/YarnUtils.java ---
@@ -202,6 +223,28 @@ public static void cloneHaNnCredentials(Configuration config) throws IOException
}
}
+ /***
--- End diff --
Use two asterisks instead of three `/**`
---