Description

1. Catching Exception too low. The wrapper around setKeys should only catch IOException.
2. InterruptedException is ignored. It should be caught at the top level and exit run.
3. Throwable is not caught. It should be caught at the top level and kill the Balancer server process.

-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 javadoc. The javadoc tool did not generate any warning messages.

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

Hadoop QA
added a comment - 01/Dec/11 21:19 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12505799/HDFS-776.patch
against trunk revision .
+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 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
-1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
-1 core tests. The patch failed these unit tests:
org.apache.hadoop.hdfs.server.datanode.TestMulitipleNNDataBlockScanner
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/1625//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/1625//artifact/trunk/hadoop-hdfs-project/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1625//console
This message is automatically generated.

-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 javadoc. The javadoc tool did not generate any warning messages.

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

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

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

Hadoop QA
added a comment - 03/Dec/11 15:45 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12505987/HDFS-776.patch
against trunk revision .
+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 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
-1 core tests. The patch failed these unit tests:
org.apache.hadoop.hdfs.TestDatanodeDeath
org.apache.hadoop.hdfs.TestFileAppend2
org.apache.hadoop.hdfs.TestDFSClientRetries
org.apache.hadoop.hdfs.TestInjectionForSimulatedStorage
org.apache.hadoop.hdfs.TestSetrepIncreasing
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/1631//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1631//console
This message is automatically generated.

Tsz Wo Nicholas Sze
added a comment - 10/Feb/12 02:43 > 3. Throwable is not caught. It should be caught at the top level and kill the Balancer server process.
I disagree since balancer supports multiple namenodes. If a balancer is working with NN1, then failing updating the keys with NN2 should not stop NN1's work.
How about throwing an exception in getAccessToken(..) when it is called? We may set shouldRun to false in run() and the check shouldRun in getAccessToken(..).

Uma Maheswara Rao G
added a comment - 10/Feb/12 04:14 Yep, considering multiple Namenodes support in Balancer today, your points make sense to me.
How about throwing an exception in getAccessToken(..) when it is called? We may set shouldRun to false in run() and the check shouldRun in getAccessToken(..).
this proposal sounds good. Let me take a look.

Attached the patch. getAccessToken will check about shouldRun flag and handle throwable outside and sets the shouldRun flag to false instead of shutdown.
Test: ran the all balancer tests to check this is not effecting.

Uma Maheswara Rao G
added a comment - 10/Feb/12 17:25 Attached the patch. getAccessToken will check about shouldRun flag and handle throwable outside and sets the shouldRun flag to false instead of shutdown.
Test: ran the all balancer tests to check this is not effecting.

Hadoop QA
added a comment - 10/Feb/12 18:32 +1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12514120/HDFS-776.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 3 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) 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 .
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/1862//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1862//console
This message is automatically generated.

Tsz Wo Nicholas Sze
added a comment - 10/Feb/12 19:54
The first one should be IOException, not Exception.
For InterruptedException, it is better to use LOG.info(..) since interrupting a thread is a normal behavior.

Uma Maheswara Rao G
added a comment - 10/Feb/12 23:25
The first one should be IOException, not Exception.
Oops. its my mistake. sorry, In previous patch i made that as IOException. In current one only forgot to change . Thanks a lot for noticing.I have updated it.
For InterruptedException, it is better to use LOG.info(..) since interrupting a thread is a normal behavior.
done.
Thanks a lot for review.

Hadoop QA
added a comment - 11/Feb/12 00:21 +1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12514178/HDFS-776.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 3 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) 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 .
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/1863//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1863//console
This message is automatically generated.