[GitHub] twill pull request #65: (TWILL-254) Update to use ContainerId.fromString

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/twill/pull/65


---

[GitHub] twill pull request #65: (TWILL-254) Update to use ContainerId.fromString

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/65#discussion_r166160088
  
--- Diff: pom.xml ---
@@ -680,9 +680,9 @@
 
 
 
-hadoop-2.5
+hadoop-2.6
 
-2.5.1
+2.6.5
--- End diff --

Yes, that's correct.


---

[GitHub] twill pull request #65: (TWILL-254) Update to use ContainerId.fromString

[cbaenziger]

Github user cbaenziger commented on a diff in the pull request:

https://github.com/apache/twill/pull/65#discussion_r166133525
  
--- Diff: pom.xml ---
@@ -680,9 +680,9 @@
 
 
 
-hadoop-2.5
+hadoop-2.6
 
-2.5.1
+2.6.5
--- End diff --

@chtyim You are asking me to leave the 2.5 profile alone and add the 2.6 
profile?


---

[GitHub] twill pull request #65: (TWILL-254) Update to use ContainerId.fromString

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/65#discussion_r166078366
  
--- Diff: pom.xml ---
@@ -680,9 +680,9 @@
 
 
 
-hadoop-2.5
+hadoop-2.6
 
-2.5.1
+2.6.5
--- End diff --

The profile for different versions of Hadoop is both for version specific 
code as well as for running unit-tests. It is better to keep it and add the 
hadoop-2.6


---

[GitHub] twill pull request #65: (TWILL-254) Update to use ContainerId.fromString

[serranom]

Github user serranom commented on a diff in the pull request:

https://github.com/apache/twill/pull/65#discussion_r165965620
  
--- Diff: 
twill-yarn/src/main/hadoop22/org/apache/twill/internal/yarn/Hadoop22YarnAMClient.java
 ---
@@ -26,7 +26,7 @@
 /**
  * Wrapper class for AMRMClient for Hadoop version 2.2 or greater.
  */
-public final class Hadoop22YarnAMClient extends Hadoop21YarnAMClient {
+public class Hadoop22YarnAMClient extends Hadoop21YarnAMClient {
 
--- End diff --

Ah, okay then.  sorry, I missed that.


---

[GitHub] twill pull request #65: (TWILL-254) Update to use ContainerId.fromString

[cbaenziger]

Github user cbaenziger commented on a diff in the pull request:

https://github.com/apache/twill/pull/65#discussion_r165862926
  
--- Diff: 
twill-yarn/src/main/hadoop22/org/apache/twill/internal/yarn/Hadoop22YarnAMClient.java
 ---
@@ -26,7 +26,7 @@
 /**
  * Wrapper class for AMRMClient for Hadoop version 2.2 or greater.
  */
-public final class Hadoop22YarnAMClient extends Hadoop21YarnAMClient {
+public class Hadoop22YarnAMClient extends Hadoop21YarnAMClient {
 
--- End diff --

If marked `final`, I can not extend it for 
[`Hadoop26YarnAMClient`](https://github.com/apache/twill/pull/65/files#diff-8c254de8ae00c6007495979dcb66a986R30)?


---

[GitHub] twill pull request #65: (TWILL-254) Update to use ContainerId.fromString

[cbaenziger]

Github user cbaenziger commented on a diff in the pull request:

https://github.com/apache/twill/pull/65#discussion_r165862857
  
--- Diff: 
twill-yarn/src/main/hadoop20/org/apache/twill/internal/yarn/Hadoop20YarnAMClient.java
 ---
@@ -71,6 +73,11 @@ public Hadoop20YarnAMClient(Configuration conf) {
 this.nmClient = new Hadoop20YarnNMClient(YarnRPC.create(conf), conf);
   }
 
+  @Override
+  private ContainerId containerIdLookup(String containerIdStr) {
+return (ConverterUtils.toContainerId(containerIdStr));
--- End diff --

Thank you


---

[GitHub] twill pull request #65: (TWILL-254) Update to use ContainerId.fromString

[cbaenziger]

Github user cbaenziger commented on a diff in the pull request:

https://github.com/apache/twill/pull/65#discussion_r165862673
  
--- Diff: pom.xml ---
@@ -680,9 +680,9 @@
 
 
 
-hadoop-2.5
+hadoop-2.6
 
-2.5.1
+2.6.5
--- End diff --

I didn't see any code specific to 2.5 that 2.3 did not provide?


---

[GitHub] twill pull request #65: (TWILL-254) Update to use ContainerId.fromString

[serranom]

Github user serranom commented on a diff in the pull request:

https://github.com/apache/twill/pull/65#discussion_r165845875
  
--- Diff: pom.xml ---
@@ -680,9 +680,9 @@
 
 
 
-hadoop-2.5
+hadoop-2.6
 
-2.5.1
+2.6.5
--- End diff --

why are you eliminating the hadoop-2.5 profile?


---

[GitHub] twill pull request #65: (TWILL-254) Update to use ContainerId.fromString

[serranom]

Github user serranom commented on a diff in the pull request:

https://github.com/apache/twill/pull/65#discussion_r165845855
  
--- Diff: pom.xml ---
@@ -171,7 +171,7 @@
 4.11
 3.2
 1.5
-[2.0.2-alpha,2.3.0]
+2.7.2
 target/hadoop20-classes
--- End diff --

is there any reason to keep this property at all?  seems better to just 
have it defined in each profile as necessary.


---

[GitHub] twill pull request #65: (TWILL-254) Update to use ContainerId.fromString

[serranom]

Github user serranom commented on a diff in the pull request:

https://github.com/apache/twill/pull/65#discussion_r165845936
  
--- Diff: 
twill-yarn/src/main/hadoop20/org/apache/twill/internal/yarn/Hadoop20YarnAMClient.java
 ---
@@ -71,6 +73,11 @@ public Hadoop20YarnAMClient(Configuration conf) {
 this.nmClient = new Hadoop20YarnNMClient(YarnRPC.create(conf), conf);
   }
 
+  @Override
+  private ContainerId containerIdLookup(String containerIdStr) {
+return (ConverterUtils.toContainerId(containerIdStr));
--- End diff --

this should be protected not private


---

[GitHub] twill pull request #65: (TWILL-254) Update to use ContainerId.fromString

[serranom]

Github user serranom commented on a diff in the pull request:

https://github.com/apache/twill/pull/65#discussion_r165845929
  
--- Diff: 
twill-yarn/src/main/hadoop22/org/apache/twill/internal/yarn/Hadoop22YarnAMClient.java
 ---
@@ -26,7 +26,7 @@
 /**
  * Wrapper class for AMRMClient for Hadoop version 2.2 or greater.
  */
-public final class Hadoop22YarnAMClient extends Hadoop21YarnAMClient {
+public class Hadoop22YarnAMClient extends Hadoop21YarnAMClient {
 
--- End diff --

why remove final?


---

[GitHub] twill pull request #65: (TWILL-254) Update to use ContainerId.fromString

[cbaenziger]

GitHub user cbaenziger opened a pull request:

https://github.com/apache/twill/pull/65

(TWILL-254) Update to use ContainerId.fromString

This moves away from the deprecated `ConverterUitls.toContainerId` and 
updates the build to use Hadoop 2.7.2 to avoid folks grabbing master and having 
a failure trying to run on clusters with code newer than November 18th, 2014 
(release of Hadoop 2.6.0).

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/cbaenziger/twill master

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/twill/pull/65.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 #65


commit 391553308ccc0539b72fac6bf13cd819ce3bb5e1
Author: Clay Baenziger 
Date:   2018-02-03T03:35:37Z

(TWILL-254) Update to use ContainerId.fromString




---