[jira] [Commented] (TWILL-262) YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is removed from DFSUtils from hadoop-2.8

2018-10-01 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/TWILL-262?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16634233#comment-16634233
 ] 

ASF GitHub Bot commented on TWILL-262:
--

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).


> YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is 
> removed from DFSUtils from hadoop-2.8 
> 
>
> Key: TWILL-262
> URL: https://issues.apache.org/jira/browse/TWILL-262
> Project: Apache Twill
>  Issue Type: Bug
>  Components: yarn
>Affects Versions: 0.8.0, 0.9.0, 0.11.0, 0.12.0, 0.13.0
>Reporter: Hongyuan Li
>Priority: Major
> Attachments: errors.txt
>
>
> YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is 
> removed from DFSUtils from hadoop-2.8 
> current code
> {code}
>   public static void cloneHaNnCredentials(Configuration config) throws 
> IOException {
> ……
> // Loop through all name services. Each name service could have multiple 
> name node associated with it.
> for (Map.Entry> entry : 
> DFSUtil.getHaNnRpcAddresses(config).entrySet()) {
>   String nsId = entry.getKey();
>   Map addressesInNN = entry.getValue();
>   if (!HAUtil.isHAEnabled(config, nsId) || addressesInNN == null || 
> addressesInNN.isEmpty()) {
> continue;
>   }
>  ……
>   }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)


[jira] [Commented] (TWILL-262) YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is removed from DFSUtils from hadoop-2.8

2018-10-01 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/TWILL-262?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16634235#comment-16634235
 ] 

ASF GitHub Bot commented on TWILL-262:
--

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>> 
getEntries(Configuration config) {
+return iDFSUtilClientExists ? invoke(config) :
+DFSUtil.getHaNnRpcAddresses(config).entrySet();
+  }
+
+  private static Set>> 
invoke(Configuration config) {
--- End diff --

Please name this method with a more appropriate name, rather than a generic 
name `invoke`.


> YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is 
> removed from DFSUtils from hadoop-2.8 
> 
>
> Key: TWILL-262
> URL: https://issues.apache.org/jira/browse/TWILL-262
> Project: Apache Twill
>  Issue Type: Bug
>  Components: yarn
>Affects Versions: 0.8.0, 0.9.0, 0.11.0, 0.12.0, 0.13.0
>Reporter: Hongyuan Li
>Priority: Major
> Attachments: errors.txt
>
>
> YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is 
> removed from DFSUtils from hadoop-2.8 
> current code
> {code}
>   public static void cloneHaNnCredentials(Configuration config) throws 
> IOException {
> ……
> // Loop through all name services. Each name service could have multiple 
> name node associated with it.
> for (Map.Entry> entry : 
> DFSUtil.getHaNnRpcAddresses(config).entrySet()) {
>   String nsId = entry.getKey();
>   Map addressesInNN = entry.getValue();
>   if (!HAUtil.isHAEnabled(config, nsId) || addressesInNN == null || 
> addressesInNN.isEmpty()) {
> continue;
>   }
>  ……
>   }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)


[jira] [Commented] (TWILL-262) YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is removed from DFSUtils from hadoop-2.8

2018-10-01 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/TWILL-262?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16634234#comment-16634234
 ] 

ASF GitHub Bot commented on TWILL-262:
--

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 `/**`


> YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is 
> removed from DFSUtils from hadoop-2.8 
> 
>
> Key: TWILL-262
> URL: https://issues.apache.org/jira/browse/TWILL-262
> Project: Apache Twill
>  Issue Type: Bug
>  Components: yarn
>Affects Versions: 0.8.0, 0.9.0, 0.11.0, 0.12.0, 0.13.0
>Reporter: Hongyuan Li
>Priority: Major
> Attachments: errors.txt
>
>
> YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is 
> removed from DFSUtils from hadoop-2.8 
> current code
> {code}
>   public static void cloneHaNnCredentials(Configuration config) throws 
> IOException {
> ……
> // Loop through all name services. Each name service could have multiple 
> name node associated with it.
> for (Map.Entry> entry : 
> DFSUtil.getHaNnRpcAddresses(config).entrySet()) {
>   String nsId = entry.getKey();
>   Map addressesInNN = entry.getValue();
>   if (!HAUtil.isHAEnabled(config, nsId) || addressesInNN == null || 
> addressesInNN.isEmpty()) {
> continue;
>   }
>  ……
>   }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)


[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...

2018-10-01 Thread chtyim
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);
  }
}
```


---


[jira] [Commented] (TWILL-262) YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is removed from DFSUtils from hadoop-2.8

2018-10-01 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/TWILL-262?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16634238#comment-16634238
 ] 

ASF GitHub Bot commented on TWILL-262:
--

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);
  }
}
```


> YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is 
> removed from DFSUtils from hadoop-2.8 
> 
>
> Key: TWILL-262
> URL: https://issues.apache.org/jira/browse/TWILL-262
> Project: Apache Twill
>  Issue Type: Bug
>  Components: yarn
>Affects Versions: 0.8.0, 0.9.0, 0.11.0, 0.12.0, 0.13.0
>Reporter: Hongyuan Li
>Priority: Major
> Attachments: errors.txt
>
>
> YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is 
> removed from DFSUtils from hadoop-2.8 
> current code
> {code}
>   public static void cloneHaNnCredentials(Configuration config) throws 
> IOException {
> ……
> // Loop through all name services. Each name service could have multiple 
> name node associated with it.
> for (Map.Entry> entry : 
> DFSUtil.getHaNnRpcAddresses(config).entrySet()) {
>   String nsId = entry.getKey();
>   Map addressesInNN = entry.getValue();
>   if (!HAUtil.isHAEnabled(config, nsId) || addressesInNN == null || 
> addressesInNN.isEmpty()) {
> continue;
>   }
>  ……
>   }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)


[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...

2018-10-01 Thread chtyim
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>> 
getEntries(Configuration config) {
+return iDFSUtilClientExists ? invoke(config) :
+DFSUtil.getHaNnRpcAddresses(config).entrySet();
+  }
+
+  private static Set>> 
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...

2018-10-01 Thread chtyim
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).


---


[jira] [Commented] (TWILL-262) YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is removed from DFSUtils from hadoop-2.8

2018-10-01 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/TWILL-262?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16634237#comment-16634237
 ] 

ASF GitHub Bot commented on TWILL-262:
--

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`.


> YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is 
> removed from DFSUtils from hadoop-2.8 
> 
>
> Key: TWILL-262
> URL: https://issues.apache.org/jira/browse/TWILL-262
> Project: Apache Twill
>  Issue Type: Bug
>  Components: yarn
>Affects Versions: 0.8.0, 0.9.0, 0.11.0, 0.12.0, 0.13.0
>Reporter: Hongyuan Li
>Priority: Major
> Attachments: errors.txt
>
>
> YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is 
> removed from DFSUtils from hadoop-2.8 
> current code
> {code}
>   public static void cloneHaNnCredentials(Configuration config) throws 
> IOException {
> ……
> // Loop through all name services. Each name service could have multiple 
> name node associated with it.
> for (Map.Entry> entry : 
> DFSUtil.getHaNnRpcAddresses(config).entrySet()) {
>   String nsId = entry.getKey();
>   Map addressesInNN = entry.getValue();
>   if (!HAUtil.isHAEnabled(config, nsId) || addressesInNN == null || 
> addressesInNN.isEmpty()) {
> continue;
>   }
>  ……
>   }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)


[jira] [Commented] (TWILL-262) YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is removed from DFSUtils from hadoop-2.8

2018-10-01 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/TWILL-262?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16634239#comment-16634239
 ] 

ASF GitHub Bot commented on TWILL-262:
--

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>> 
getEntries(Configuration config) {
--- End diff --

Please name this method with a more specific name, such as 
`getHaNnRpcAddress`.


> YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is 
> removed from DFSUtils from hadoop-2.8 
> 
>
> Key: TWILL-262
> URL: https://issues.apache.org/jira/browse/TWILL-262
> Project: Apache Twill
>  Issue Type: Bug
>  Components: yarn
>Affects Versions: 0.8.0, 0.9.0, 0.11.0, 0.12.0, 0.13.0
>Reporter: Hongyuan Li
>Priority: Major
> Attachments: errors.txt
>
>
> YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is 
> removed from DFSUtils from hadoop-2.8 
> current code
> {code}
>   public static void cloneHaNnCredentials(Configuration config) throws 
> IOException {
> ……
> // Loop through all name services. Each name service could have multiple 
> name node associated with it.
> for (Map.Entry> entry : 
> DFSUtil.getHaNnRpcAddresses(config).entrySet()) {
>   String nsId = entry.getKey();
>   Map addressesInNN = entry.getValue();
>   if (!HAUtil.isHAEnabled(config, nsId) || addressesInNN == null || 
> addressesInNN.isEmpty()) {
> continue;
>   }
>  ……
>   }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)


[jira] [Commented] (TWILL-262) YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is removed from DFSUtils from hadoop-2.8

2018-10-01 Thread ASF GitHub Bot (JIRA)


[ 
https://issues.apache.org/jira/browse/TWILL-262?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16634236#comment-16634236
 ] 

ASF GitHub Bot commented on TWILL-262:
--

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>> 
getEntries(Configuration config) {
+return iDFSUtilClientExists ? invoke(config) :
+DFSUtil.getHaNnRpcAddresses(config).entrySet();
+  }
+
+  private static Set>> 
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`??


> YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is 
> removed from DFSUtils from hadoop-2.8 
> 
>
> Key: TWILL-262
> URL: https://issues.apache.org/jira/browse/TWILL-262
> Project: Apache Twill
>  Issue Type: Bug
>  Components: yarn
>Affects Versions: 0.8.0, 0.9.0, 0.11.0, 0.12.0, 0.13.0
>Reporter: Hongyuan Li
>Priority: Major
> Attachments: errors.txt
>
>
> YarnUtils#cloneHaNnCredentials uses DFSUtil#getHaNnRpcAddresses, which is 
> removed from DFSUtils from hadoop-2.8 
> current code
> {code}
>   public static void cloneHaNnCredentials(Configuration config) throws 
> IOException {
> ……
> // Loop through all name services. Each name service could have multiple 
> name node associated with it.
> for (Map.Entry> entry : 
> DFSUtil.getHaNnRpcAddresses(config).entrySet()) {
>   String nsId = entry.getKey();
>   Map addressesInNN = entry.getValue();
>   if (!HAUtil.isHAEnabled(config, nsId) || addressesInNN == null || 
> addressesInNN.isEmpty()) {
> continue;
>   }
>  ……
>   }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)


[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...

2018-10-01 Thread chtyim
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 `/**`


---


[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...

2018-10-01 Thread chtyim
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...

2018-10-01 Thread chtyim
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>> 
getEntries(Configuration config) {
+return iDFSUtilClientExists ? invoke(config) :
+DFSUtil.getHaNnRpcAddresses(config).entrySet();
+  }
+
+  private static Set>> 
invoke(Configuration config) {
--- End diff --

Please name this method with a more appropriate name, rather than a generic 
name `invoke`.


---