Xiao Chen added a comment - 30/Apr/16 06:40 Patch 1 to do a similar fix to httpfs.sh. Tested the same way as HADOOP-13030 , works. The function is small and I think we'd better no bother extracting that for reuse. Searched for similar patterns and I think httpfs.sh and kms.sh are the only 2 scripts doing this thing. Could you please review Allen Wittenauer ? Thanks!

Hadoop QA added a comment - 30/Apr/16 07:00 -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 12s Docker mode activated. 0 shelldocs 0m 3s Shelldocs was not available. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s 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 mvninstall 7m 15s trunk passed +1 mvnsite 0m 32s trunk passed +1 mvnsite 0m 29s the patch passed +1 shellcheck 0m 11s The patch generated 0 new + 93 unchanged - 3 fixed = 93 total (was 96) +1 whitespace 0m 0s The patch has no whitespace issues. +1 unit 0m 16s hadoop-hdfs-httpfs in the patch passed with JDK v1.8.0_92. +1 unit 0m 17s hadoop-hdfs-httpfs in the patch passed with JDK v1.7.0_95. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 11m 3s Subsystem Report/Notes Docker Image:yetus/hadoop:cf2ee45 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12801584/HADOOP-13077.01.patch JIRA Issue HADOOP-13077 Optional Tests asflicense mvnsite unit shellcheck shelldocs uname Linux d61449cb62fe 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 2753185 shellcheck v0.4.3 JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9239/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs-httpfs U: hadoop-hdfs-project/hadoop-hdfs-httpfs Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9239/console Powered by Apache Yetus 0.3.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.

Allen Wittenauer added a comment - 30/Apr/16 16:37 Thanks. This gives us a chance to clean this up: any reason why this is using multiple seds instead of just one using -e? multiple seds are expensive... this should be in hadoop-functions.sh and have both kms.sh and httpfs.sh call it. this removes some code redundancy and allows us to write a unit test, thus eliminating the need to have a tar ball to reproduce when it gets moved to hadoop-functions, probably better to rename it to something like hadoop_xml_passescape or whatever. (hadoop_escape is pretty generic)

Allen Wittenauer added a comment - 30/Apr/16 22:43 Also, the use of ret and the echo aren't actually doing anything here, given ret just contains the stdout of the sed. In other words, this function should really just be one big sed statement.

Xiao Chen added a comment - 01/May/16 19:43 Thanks Allen Wittenauer for the detailed reviews! Patch 2 addresses most of your comments. The only difference is that I extracted 2 functions, to escape sed and xml separately. They could be of general use. I manually verified the scripts work by just executing the scripts with correct env vars. (Didn't find any example in our code or the docs about how shell unit tests are done in hadoop... Please feel free to share your thoughts)

Hadoop QA added a comment - 01/May/16 20:05 -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 9s Docker mode activated. 0 shelldocs 0m 3s Shelldocs was not available. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s 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. 0 mvndep 0m 15s Maven dependency ordering for branch +1 mvninstall 6m 25s trunk passed +1 mvnsite 1m 54s trunk passed 0 mvndep 0m 15s Maven dependency ordering for patch +1 mvnsite 1m 43s the patch passed +1 shellcheck 0m 10s The patch generated 0 new + 94 unchanged - 2 fixed = 94 total (was 96) +1 whitespace 0m 0s The patch has no whitespace issues. +1 unit 1m 46s hadoop-common in the patch passed with JDK v1.8.0_92. +1 unit 0m 13s hadoop-kms in the patch passed with JDK v1.8.0_92. +1 unit 0m 14s hadoop-hdfs-httpfs in the patch passed with JDK v1.8.0_92. +1 unit 1m 51s hadoop-common in the patch passed with JDK v1.7.0_95. +1 unit 0m 15s hadoop-kms in the patch passed with JDK v1.7.0_95. +1 unit 0m 16s hadoop-hdfs-httpfs in the patch passed with JDK v1.7.0_95. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 16m 6s Subsystem Report/Notes Docker Image:yetus/hadoop:cf2ee45 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12801674/HADOOP-13077.02.patch JIRA Issue HADOOP-13077 Optional Tests asflicense mvnsite unit shellcheck shelldocs uname Linux b2923dc2dc41 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 971af60 shellcheck v0.4.3 JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9245/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-common-project/hadoop-kms hadoop-hdfs-project/hadoop-hdfs-httpfs U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9245/console Powered by Apache Yetus 0.3.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.

Allen Wittenauer added a comment - 01/May/16 20:09 Didn't find any example in our code Take a look in hadoop-common-project/hadoop-common/src/test/scripts for the script unit tests.

Xiao Chen added a comment - 01/May/16 21:34 Thanks again Allen, unit test added.

Hadoop QA added a comment - 01/May/16 21:55 +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 9s Docker mode activated. 0 shelldocs 0m 4s Shelldocs was not available. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. 0 mvndep 0m 15s Maven dependency ordering for branch +1 mvninstall 6m 27s trunk passed +1 mvnsite 1m 52s trunk passed 0 mvndep 0m 16s Maven dependency ordering for patch +1 mvnsite 1m 42s the patch passed +1 shellcheck 0m 10s The patch generated 0 new + 94 unchanged - 2 fixed = 94 total (was 96) +1 whitespace 0m 0s The patch has no whitespace issues. +1 unit 1m 46s hadoop-common in the patch passed with JDK v1.8.0_92. +1 unit 0m 13s hadoop-kms in the patch passed with JDK v1.8.0_92. +1 unit 0m 14s hadoop-hdfs-httpfs in the patch passed with JDK v1.8.0_92. +1 unit 1m 51s hadoop-common in the patch passed with JDK v1.7.0_95. +1 unit 0m 15s hadoop-kms in the patch passed with JDK v1.7.0_95. +1 unit 0m 16s hadoop-hdfs-httpfs in the patch passed with JDK v1.7.0_95. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 16m 6s Subsystem Report/Notes Docker Image:yetus/hadoop:cf2ee45 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12801681/HADOOP-13077.03.patch JIRA Issue HADOOP-13077 Optional Tests asflicense mvnsite unit shellcheck shelldocs uname Linux 95e32d01e5ca 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 971af60 shellcheck v0.4.3 JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9246/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-common-project/hadoop-kms hadoop-hdfs-project/hadoop-hdfs-httpfs U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9246/console Powered by Apache Yetus 0.3.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.

Allen Wittenauer added a comment - 05/May/16 18:33 looks fine. committed to trunk. thanks!

Hudson added a comment - 05/May/16 19:08 FAILURE: Integrated in Hadoop-trunk-Commit #9723 (See https://builds.apache.org/job/Hadoop-trunk-Commit/9723/ ) HADOOP-13077 . Handle special characters in passwords in httpfs.sh (Xiao (aw: rev 35cf503149d68d33ee4e20e3e57f9afa69aef7f5) hadoop-common-project/hadoop-common/src/test/scripts/hadoop_escape_chars.bats hadoop-hdfs-project/hadoop-hdfs-httpfs/src/main/sbin/httpfs.sh hadoop-common-project/hadoop-common/src/main/bin/hadoop-functions.sh hadoop-common-project/hadoop-kms/src/main/sbin/kms.sh