null check added

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

updated patch with testcase

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12528034/HADOOP-8362-1.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 passed unit tests in hadoop-common-project/hadoop-common.

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1007//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1007//console

This message is automatically generated.

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.

updated the patch according to harsh's comments.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12528070/HADOOP-8362-2.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.io.TestMapFile
org.apache.hadoop.fs.TestFilterFileSystem
org.apache.hadoop.fs.viewfs.TestViewFileSystemDelegation
org.apache.hadoop.http.lib.TestStaticUserWebFilter
org.apache.hadoop.fs.viewfs.TestViewFsTrash
org.apache.hadoop.io.file.tfile.TestTFileStreams
org.apache.hadoop.util.TestGenericOptionsParser
org.apache.hadoop.ipc.TestSaslRPC
org.apache.hadoop.ipc.TestMultipleProtocolServer
org.apache.hadoop.conf.TestReconfiguration
org.apache.hadoop.ipc.TestSocketFactory
org.apache.hadoop.fs.viewfs.TestViewfsFileStatus
org.apache.hadoop.io.file.tfile.TestTFile
org.apache.hadoop.ipc.TestProtoBufRpc
org.apache.hadoop.fs.TestFsShellReturnCode
org.apache.hadoop.conf.TestConfigurationDeprecation
org.apache.hadoop.io.serializer.TestWritableSerialization
org.apache.hadoop.io.TestGenericWritable
org.apache.hadoop.io.file.tfile.TestTFileSplit
org.apache.hadoop.fs.s3native.TestInMemoryNativeS3FileSystemContract
org.apache.hadoop.fs.viewfs.TestViewFsWithAuthorityLocalFs
org.apache.hadoop.net.TestScriptBasedMapping
org.apache.hadoop.fs.TestS3_LocalFileContextURI
org.apache.hadoop.security.TestSecurityUtil
org.apache.hadoop.util.TestGenericsUtil
org.apache.hadoop.fs.viewfs.TestFcPermissionsLocalFs
org.apache.hadoop.fs.TestTruncatedInputBug
org.apache.hadoop.ipc.TestRPCCallBenchmark
org.apache.hadoop.http.TestHttpServerLifecycle
org.apache.hadoop.fs.TestTrash
org.apache.hadoop.security.TestGroupsCaching
org.apache.hadoop.io.TestDefaultStringifier
org.apache.hadoop.security.TestUserGroupInformation
org.apache.hadoop.log.TestLogLevel
org.apache.hadoop.fs.viewfs.TestFSMainOperationsLocalFileSystem
org.apache.hadoop.io.file.tfile.TestTFileNoneCodecsByteArrays
org.apache.hadoop.conf.TestGetInstances
org.apache.hadoop.ha.TestZKFailoverController
org.apache.hadoop.io.file.tfile.TestTFileSeqFileComparison
org.apache.hadoop.ha.TestHealthMonitor
org.apache.hadoop.fs.permission.TestFsPermission
org.apache.hadoop.io.compress.TestCodec
org.apache.hadoop.fs.TestFileSystemCanonicalization
org.apache.hadoop.net.TestTableMapping
org.apache.hadoop.security.TestLdapGroupsMapping
org.apache.hadoop.fs.shell.TestPathData
org.apache.hadoop.ipc.TestServer
org.apache.hadoop.fs.viewfs.TestFcMainOperationsLocalFs
org.apache.hadoop.io.file.tfile.TestTFileNoneCodecsJClassComparatorByteArrays
org.apache.hadoop.http.TestHttpServerWebapps
org.apache.hadoop.fs.viewfs.TestViewFileSystemLocalFileSystem
org.apache.hadoop.ipc.TestRPC
org.apache.hadoop.http.TestGlobalFilter
org.apache.hadoop.security.authorize.TestProxyUsers
org.apache.hadoop.io.file.tfile.TestTFileSeek
org.apache.hadoop.fs.viewfs.TestViewFsConfig
org.apache.hadoop.fs.viewfs.TestFcCreateMkdirLocalFs
org.apache.hadoop.fs.TestLocal_S3FileContextURI
org.apache.hadoop.io.file.tfile.TestTFileByteArrays
org.apache.hadoop.ha.TestSshFenceByTcpPort
org.apache.hadoop.io.file.tfile.TestTFileUnsortedByteArrays
org.apache.hadoop.ipc.TestMiniRPCBenchmark
org.apache.hadoop.fs.s3.TestInMemoryS3FileSystemContract
org.apache.hadoop.ipc.TestIPC
org.apache.hadoop.conf.TestConfServlet
org.apache.hadoop.cli.TestCLI
org.apache.hadoop.io.file.tfile.TestTFileComparator2
org.apache.hadoop.security.TestAuthenticationFilter
org.apache.hadoop.fs.viewfs.TestViewFileSystemDelegationTokenSupport
org.apache.hadoop.fs.viewfs.TestViewFsLocalFs
org.apache.hadoop.conf.TestDeprecatedKeys
org.apache.hadoop.conf.TestConfiguration
org.apache.hadoop.io.serializer.avro.TestAvroSerialization
org.apache.hadoop.fs.TestLocalDirAllocator
org.apache.hadoop.net.TestSwitchMapping
org.apache.hadoop.ha.TestShellCommandFencer
org.apache.hadoop.jmx.TestJMXJsonServlet
org.apache.hadoop.io.TestBloomMapFile
org.apache.hadoop.io.compress.TestCodecFactory
org.apache.hadoop.io.TestSequenceFileSerialization
org.apache.hadoop.ipc.TestRPCCompatibility
org.apache.hadoop.fs.viewfs.TestViewFileSystemWithAuthorityLocalFileSystem
org.apache.hadoop.io.file.tfile.TestTFileJClassComparatorByteArrays
org.apache.hadoop.http.TestPathFilter
org.apache.hadoop.net.TestStaticMapping
org.apache.hadoop.ipc.TestIPCServerResponder
org.apache.hadoop.security.TestDoAsEffectiveUser
org.apache.hadoop.http.TestServletFilter
org.apache.hadoop.http.TestHttpServer

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1008//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1008//console

This message is automatically generated.

fixed patch for the core-tests

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

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!

updated patch to have two separate pre-conditions.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12530034/HADOOP-8362-4.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 patch. The patch command could not apply the patch.

Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1046//console

This message is automatically generated.

fixed apply patch issue

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12530036/HADOOP-8362-5.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 patch appears to cause the build to fail.

Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1047//console

This message is automatically generated.

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

Hi Harsh,
Sorry for too many patches ..was not sure about when to use submit patch button yeah will use the test-patch shell script going forward ..thank you.

No need to be sorry! I just wanted to note that. Thanks for your continuing contributions! Let us know if you need help with test-patch/etc.

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

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/)

updated patch with style fixes

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12532650/HADOOP-8362-7.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 patch. The patch command could not apply the patch.

Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1124//console

This message is automatically generated.

fixed patch issues

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12532674/HADOOP-8362-8.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 13 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 passed unit tests in hadoop-common-project/hadoop-common.

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1125//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1125//console

This message is automatically generated.

Minor changes to fix indentation issues.

Madhukara, in future, please follow the coding guidelines as Colin had suggested.

Committed the patch. Thank you Madhukara.

Hi Suresh,

Looks like this wasn't committed? I'm going ahead and committing it in. Reopening for until it is done.

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.

Committed to branch-2 and trunk. Thanks Madhukara and Suresh!

Integrated in Hadoop-Mapreduce-trunk-Commit #2492 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2492/)
HADOOP-8362. Improve exception message when Configuration.set() is called with a null key or value. Contributed by Madhukara Phatak and Suresh Srinivas (harsh) (Revision 1361712)

Result = FAILURE
harsh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361712
Files :

• /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java

Integrated in Hadoop-Hdfs-trunk #1105 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1105/)
HADOOP-8362. Improve exception message when Configuration.set() is called with a null key or value. Contributed by Madhukara Phatak and Suresh Srinivas (harsh) (Revision 1361712)

Result = FAILURE
harsh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361712
Files :

• /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java

Integrated in Hadoop-Mapreduce-trunk #1138 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1138/)
HADOOP-8362. Improve exception message when Configuration.set() is called with a null key or value. Contributed by Madhukara Phatak and Suresh Srinivas (harsh) (Revision 1361712)

Result = FAILURE
harsh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361712
Files :

• /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java