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

Hadoop QA
added a comment - 14/May/12 08:44 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12526719/HADOOP-8362.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 javac. The patch appears to cause the build to fail.
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/993//console
This message is automatically generated.

Harsh J
added a comment - 18/May/12 15:36 Hi,
Thanks for the patches!
Some comments from my side:
Better to use IllegalArgumentException instead of the raw RuntimeException.
Perhaps we can check for this very early (first step) into the set() call. Perhaps better to be done via Guava's http://google-collections.googlecode.com/svn/trunk/javadoc/com/google/common/base/Preconditions.html#checkArgument(boolean,%20java.lang.Object ) call, rather than duplicating code. We already have Guava added in as a dependency, makes sense to leverage it where we can for its free benefits.
Please also document the behavior of sending in nulls inside the set call's javadocs, so users can benefit from it (by knowing what to expect).
Test is good, but also good to assert that the exception object is the right one, and that the message is proper. Helps avoid regressions in future.

Hadoop QA
added a comment - 18/May/12 17:32 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12528093/HADOOP-8362-3.patch
against trunk revision .
+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. The javadoc tool appears to have generated 2 warning messages.
+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 failed these unit tests in hadoop-common-project/hadoop-common:
org.apache.hadoop.fs.viewfs.TestViewFsTrash
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1009//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1009//console
This message is automatically generated.

Sorry for the late response here. I failed to add another comment earlier:

Can you separate the key==null and value==null checks? We shouldn't have the users determine which one of them being null caused the exception as is currently the case anyway. Two preconditions would be good (same for tests).

Harsh J
added a comment - 29/May/12 12:37 Hi,
Sorry for the late response here. I failed to add another comment earlier:
Can you separate the key==null and value==null checks? We shouldn't have the users determine which one of them being null caused the exception as is currently the case anyway. Two preconditions would be good (same for tests).
Thanks!

I do appreciate (and love) your enthusiasm in providing new, review-fixing patches but please do test your patch locally for simple things such as compiler errors and test runs before you send it across. Surely a local compile can help us save a lot of time.

You can also run a test-patch.sh run locally to test your patch yourself first, instead of relying on the QA queue, which is for reviewed patches alone.

Harsh J
added a comment - 29/May/12 14:59 Hi Madhukara,
I do appreciate (and love) your enthusiasm in providing new, review-fixing patches but please do test your patch locally for simple things such as compiler errors and test runs before you send it across. Surely a local compile can help us save a lot of time.
You can also run a test-patch.sh run locally to test your patch yourself first, instead of relying on the QA queue, which is for reviewed patches alone.
Thanks and please keep contributing!
Harsh

Hadoop QA
added a comment - 30/May/12 07:33 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12530159/HADOOP-8362-6.patch
against trunk revision .
+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. The javadoc tool did not generate any warning messages.
+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 failed these unit tests in hadoop-common-project/hadoop-common:
org.apache.hadoop.fs.viewfs.TestViewFsTrash
org.apache.hadoop.ha.TestZKFailoverController
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1055//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1055//console
This message is automatically generated.

Colin Patrick McCabe
added a comment - 19/Jun/12 18:48 Hi Madhukara,
The patch looks pretty good. Just one small criticism: you should put the braces on the same line as the 'if' or 'else' statement.
See http://wiki.apache.org/hadoop/CodeReviewChecklist for details-- basically, we try to follow Sun's coding conventions, except with 2-space indentation instead of 4.
(See http://java.sun.com/docs/codeconv/ )

Harsh J
added a comment - 15/Jul/12 16:03 Had to rebase this after HADOOP-8525 made some changes.
Also fixed an extra space in midst of a javadoc sentence, and made the error more assertive, and descriptive of the context.
Added tests were unmodified pass as before:
Running org.apache.hadoop.conf.TestConfiguration
Tests run: 50, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2.544 sec
Committing this version.