You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@twill.apache.org by cbaenziger <gi...@git.apache.org> on 2018/02/04 06:27:19 UTC
[GitHub] twill pull request #65: (TWILL-254) Update to use ContainerId.fromString
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 <cb...@...>
Date: 2018-02-03T03:35:37Z
(TWILL-254) Update to use ContainerId.fromString
----
---
[GitHub] twill issue #65: (TWILL-254) Update to use ContainerId.fromString
Posted by cbaenziger <gi...@git.apache.org>.
Github user cbaenziger commented on the issue:
https://github.com/apache/twill/pull/65
Hi @chtyim, thank you for the review. Could you please expand on what you see concerning in the `pom.xml`? For reference, I need to provide a 2.6 target as it is the first version to provide the `ContainerId.fromString()` API.
---
[GitHub] twill issue #65: (TWILL-254) Update to use ContainerId.fromString
Posted by cbaenziger <gi...@git.apache.org>.
Github user cbaenziger commented on the issue:
https://github.com/apache/twill/pull/65
Thank you for the super fast code review @serranom! I think all comments have been addressed and the code should be complete now. Please let me know if you see anything else necessary. It is great working with Twill! Hopefully I can provide some other ideas as I work with it.
---
[GitHub] twill issue #65: (TWILL-254) Update to use ContainerId.fromString
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on the issue:
https://github.com/apache/twill/pull/65
Changes LGTM.
---
[GitHub] twill pull request #65: (TWILL-254) Update to use ContainerId.fromString
Posted by cbaenziger <gi...@git.apache.org>.
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 @@
</build>
</profile>
<profile>
- <id>hadoop-2.5</id>
+ <id>hadoop-2.6</id>
<properties>
- <hadoop.version>2.5.1</hadoop.version>
+ <hadoop.version>2.6.5</hadoop.version>
--- End diff --
I didn't see any code specific to 2.5 that 2.3 did not provide?
---
[GitHub] twill issue #65: (TWILL-254) Update to use ContainerId.fromString
Posted by cbaenziger <gi...@git.apache.org>.
Github user cbaenziger commented on the issue:
https://github.com/apache/twill/pull/65
I added a few omissions and corrected the parent `pom.xml` from my original submission. Now the new code is actually being used and I have updated the default target to Hadoop 2.6 and removed the generic `hadoop.version` as suggested.
---
[GitHub] twill pull request #65: (TWILL-254) Update to use ContainerId.fromString
Posted by cbaenziger <gi...@git.apache.org>.
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 @@
</build>
</profile>
<profile>
- <id>hadoop-2.5</id>
+ <id>hadoop-2.6</id>
<properties>
- <hadoop.version>2.5.1</hadoop.version>
+ <hadoop.version>2.6.5</hadoop.version>
--- 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
Posted by serranom <gi...@git.apache.org>.
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 @@
</build>
</profile>
<profile>
- <id>hadoop-2.5</id>
+ <id>hadoop-2.6</id>
<properties>
- <hadoop.version>2.5.1</hadoop.version>
+ <hadoop.version>2.6.5</hadoop.version>
--- End diff --
why are you eliminating the hadoop-2.5 profile?
---
[GitHub] twill pull request #65: (TWILL-254) Update to use ContainerId.fromString
Posted by asfgit <gi...@git.apache.org>.
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
Posted by cbaenziger <gi...@git.apache.org>.
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
Posted by cbaenziger <gi...@git.apache.org>.
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
Posted by serranom <gi...@git.apache.org>.
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
Posted by chtyim <gi...@git.apache.org>.
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 @@
</build>
</profile>
<profile>
- <id>hadoop-2.5</id>
+ <id>hadoop-2.6</id>
<properties>
- <hadoop.version>2.5.1</hadoop.version>
+ <hadoop.version>2.6.5</hadoop.version>
--- 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
Posted by serranom <gi...@git.apache.org>.
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 @@
<junit.version>4.11</junit.version>
<jopt-simple.version>3.2</jopt-simple.version>
<commons-compress.version>1.5</commons-compress.version>
- <hadoop.version>[2.0.2-alpha,2.3.0]</hadoop.version>
+ <hadoop.version>2.7.2</hadoop.version>
<hadoop20.output.dir>target/hadoop20-classes</hadoop20.output.dir>
--- 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
Posted by serranom <gi...@git.apache.org>.
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
Posted by serranom <gi...@git.apache.org>.
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
Posted by chtyim <gi...@git.apache.org>.
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 @@
</build>
</profile>
<profile>
- <id>hadoop-2.5</id>
+ <id>hadoop-2.6</id>
<properties>
- <hadoop.version>2.5.1</hadoop.version>
+ <hadoop.version>2.6.5</hadoop.version>
--- End diff --
Yes, that's correct.
---
[GitHub] twill issue #65: (TWILL-254) Update to use ContainerId.fromString
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on the issue:
https://github.com/apache/twill/pull/65
The change looks mostly ok to me, except for the change in the pom.xml profile.
---