Details

Description

HADOOP-11492 upped the Curator version to 2.7.1, which makes the ChildReaper class use a method that only exists in newer versions of Guava (we have 11.0.2, and it needs 15+). As a workaround, we can copy the ChildReaper class into hadoop-common and make a minor modification to allow it to work with Guava 11.

The ChildReaper is used by Curator to cleanup old lock znodes. Curator locks are needed by YARN-2942.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The following test timeouts occurred in hadoop-common-project/hadoop-common:

Hadoop QA
added a comment - 19/Feb/15 02:11 -1 overall . Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12699584/HADOOP-11612.001.patch
against trunk revision 1c03376.
+1 @author . The patch does not contain any @author tags.
-1 tests included . The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.
+1 javac . The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc . There were no new javadoc warning messages.
+1 eclipse:eclipse . The patch built with eclipse:eclipse.
+1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings.
+1 release audit . The applied patch does not increase the total number of release audit warnings.
-1 core tests . The following test timeouts occurred in hadoop-common-project/hadoop-common:
org.apache.hadoop.http.TestHttpServerLifecycle
Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5739//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5739//console
This message is automatically generated.

Tsuyoshi Ozawa
added a comment - 19/Feb/15 06:57 Robert Kanter Thank you for taking this issue. I look over your patch. +1 for this change. I think we should add test code, like TestChildReaper, not to introduce bugs with modified ChildReaper.

Robert Kanter
added a comment - 19/Feb/15 08:06 Thanks for looking at the patch Tsuyoshi Ozawa . That's a good point. In the 002 patch, I've also copied TestChildReaper from Curator, and modified it slightly to work with JUnit.

Hadoop QA
added a comment - 19/Feb/15 09:06 +1 overall . Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12699625/HADOOP-11612.002.patch
against trunk revision 946456c.
+1 @author . The patch does not contain any @author tags.
+1 tests included . The patch appears to include 1 new or modified test files.
+1 javac . The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc . There were no new javadoc warning messages.
+1 eclipse:eclipse . The patch built with eclipse:eclipse.
+1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings.
+1 release audit . The applied patch does not increase the total number of release audit warnings.
+1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common.
Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5742//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5742//console
This message is automatically generated.

Steve Loughran
added a comment - 19/Feb/15 09:58 Interesting; in HADOOP-11102 I thought i'd rebuilt curator against guava < 15 and didn't see problems.
Guava is pain.
We are going to upgrade in trunk before long, though the price is jenkins isn't going to notice problems in pre-commits.
so we should plan to switch back to curator ChildReaper at some point in the future -this patch needs to reflect that.
if there is more JUnit support in this patch —why not try to get it into curator

I thought i'd rebuilt curator against guava < 15 and didn't see problems.

At the time, I don't think anything was using ChildReaper, so we didn't catch it. I'm planning on using it for something new in YARN-2942. Also, Curator 2.5.0's ChildReaper compiles fine against guava < 15.

so we should plan to switch back to curator ChildReaper at some point in the future -this patch needs to reflect that.

I agree. What's the best way to reflect that? I was just going to file a followup JIRA. Or is there an "Upgrade Guava JIRA"?

if there is more JUnit support in this patch —why not try to get it into curator

It was mainly changing some imports and not pulling in the iRetry stuff from the parent class. I don't think we can make the test compile against JUnit and what Curator is using (TestNG?). In any case, we want the test in Hadoop for now, to test the copied ChildReaper.

Robert Kanter
added a comment - 19/Feb/15 20:50 I thought i'd rebuilt curator against guava < 15 and didn't see problems.
At the time, I don't think anything was using ChildReaper , so we didn't catch it. I'm planning on using it for something new in YARN-2942 . Also, Curator 2.5.0's ChildReaper compiles fine against guava < 15.
so we should plan to switch back to curator ChildReaper at some point in the future -this patch needs to reflect that.
I agree. What's the best way to reflect that? I was just going to file a followup JIRA. Or is there an "Upgrade Guava JIRA"?
if there is more JUnit support in this patch —why not try to get it into curator
It was mainly changing some imports and not pulling in the iRetry stuff from the parent class. I don't think we can make the test compile against JUnit and what Curator is using (TestNG?). In any case, we want the test in Hadoop for now, to test the copied ChildReaper .

Robert Kanter
added a comment - 20/Feb/15 01:28 The 003 patch makes the regular comments I had on ChildReaper and TestChildReaper into Javadoc comments; it also adds Private and Unstable annotations to ChildReaper .

Hadoop QA
added a comment - 20/Feb/15 02:26 +1 overall . Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12699793/HADOOP-11612.003.patch
against trunk revision c0d9b93.
+1 @author . The patch does not contain any @author tags.
+1 tests included . The patch appears to include 1 new or modified test files.
+1 javac . The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc . There were no new javadoc warning messages.
+1 eclipse:eclipse . The patch built with eclipse:eclipse.
+1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings.
+1 release audit . The applied patch does not increase the total number of release audit warnings.
+1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common.
Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5745//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5745//console
This message is automatically generated.