----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23393/#review47566 -----------------------------------------------------------
Overall, the patch is moving in a good direction, but there's still a lot of inconsistencies. In particular, the use of quoting for args that could contain spaces, the use of grep v. cat v. egrep to read hosts files, and extra quotes in assignments. assemble/bin/accumulo <https://reviews.apache.org/r/23393/#comment83573> This part can probably be removed since ACCUMULO-2606 happened, but it doesn't matter that much. assemble/bin/bootstrap_hdfs.sh <https://reviews.apache.org/r/23393/#comment83574> Should probably s/cat/grep -v '^#'/ and double-quote the arg (if necessary, as is seems to be done elsewhere) assemble/bin/bootstrap_hdfs.sh <https://reviews.apache.org/r/23393/#comment83575> dirname arg in bin/accumulo was quoted. Should these args also be quoted? assemble/bin/build_native_library.sh <https://reviews.apache.org/r/23393/#comment83576> Don't need to quote bash vars in [[ ]] assemble/bin/build_native_library.sh <https://reviews.apache.org/r/23393/#comment83577> unnecessary outer quote. bin=$() is sufficient assemble/bin/build_native_library.sh <https://reviews.apache.org/r/23393/#comment83578> Don't need to quote assignments. assemble/bin/config.sh <https://reviews.apache.org/r/23393/#comment83579> More assignment quotes not needed. (there's probably more, but I'll stop checking at this one) assemble/bin/config.sh <https://reviews.apache.org/r/23393/#comment83580> quote dirname args? assemble/bin/start-all.sh <https://reviews.apache.org/r/23393/#comment83581> not sure it's necessary to grep -v blank lines like this, since they shouldn't show up as elements of the list. I believe a grep -v '^#' is sufficient (and less confusing). This should be verified and applied consistently in the other scripts that cat/grep these files. assemble/bin/start-server.sh <https://reviews.apache.org/r/23393/#comment83582> I'm partial to the more succinct: [[ -x $IFCONFIG ]] && IFCONFIG='/bin/netstat -ie' || IFCONFIG=/sbin/ifconfig There's other places where this could be done also (LONGNAME above, for instance). I don't feel strongly about this, but if we're going for consistency, it might be good to be consistently succinct also. assemble/bin/start-server.sh <https://reviews.apache.org/r/23393/#comment83583> Drop the braces on the variables. They aren't needed in the [[ ]] block. There's no ambiguity here. assemble/bin/start-server.sh <https://reviews.apache.org/r/23393/#comment83584> unneeded quotes on bash vars in block assemble/bin/stop-all.sh <https://reviews.apache.org/r/23393/#comment83586> more grep filtering inconsistency on host files assemble/bin/stop-server.sh <https://reviews.apache.org/r/23393/#comment83587> missed conversion from [] to [[]] core/src/main/scripts/generate-protobuf.sh <https://reviews.apache.org/r/23393/#comment83589> might as well drop trailing whitespace before committing test/system/randomwalk/bin/reset-cluster.sh <https://reviews.apache.org/r/23393/#comment83597> Use -z test/system/randomwalk/bin/start-all.sh <https://reviews.apache.org/r/23393/#comment83598> Use -z test/system/upgrade_test.sh <https://reviews.apache.org/r/23393/#comment83591> Not sure I agree it makes sense to do arithmetic syntax for conditionals. We don't want a numerical result, we want an evaluation to true or false. While this probably achieves the same, it's harder to read. Conditionals should probably use [[ ]]. I noticed this elsewhere also. Might want to go back and check. test/system/upgrade_test.sh <https://reviews.apache.org/r/23393/#comment83593> It's probably good practice to use single quotes for string literals when they don't contain embedded variables. - Christopher Tubbs On July 10, 2014, 10:07 a.m., Mike Drob wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23393/ > ----------------------------------------------------------- > > (Updated July 10, 2014, 10:07 a.m.) > > > Review request for accumulo and Alex Moundalexis. > > > Bugs: ACCUMULO-2423 > https://issues.apache.org/jira/browse/ACCUMULO-2423 > > > Repository: accumulo > > > Description > ------- > > ACCUMULO-2423 Clean up (most?) shell scripts > > Standardize interpreter line on '#! /usr/bin/env foo' > Replace `..` with $(..) > Replace [..] with [[..]] > Replace = with == in [[..]] > Replace arithmetic [[..]] with ((..)) > Remove useless invocations of echo > Add some double quotes to prevent word splitting > > > Diffs > ----- > > assemble/bin/accumulo bdd742d7aee81dc82872acee1b4333ee6e8348c7 > assemble/bin/bootstrap_hdfs.sh 6f38f633f4789ac7bc09639eeee58495ee4c3ea2 > assemble/bin/build_native_library.sh > 907ce2c946ba1b6c4ddcc889011f899fc1d96575 > assemble/bin/config.sh 76634731abd2e51dc924804f76c1b73a9f5086d6 > assemble/bin/start-all.sh b71ba705889f6c157194ae4a816f8e21e9377b8b > assemble/bin/start-server.sh 62ad91d57731d7e32b3ce6e51a961b11c64bf163 > assemble/bin/stop-all.sh 3bd30bd758a9226d7e1e8e0b66cd36704e052d0e > assemble/bin/stop-server.sh 6fbe0afde2b881cb7ded6b66d6fcaad35102d7cd > assemble/bin/tool.sh 8126b3b1a5753a395a21f47fe7849de79af05135 > assemble/bin/tup.sh b26def5790cf322cbd203fd53faa337d3cec617d > assemble/build.sh 5f65419dcff6120dc6e7ca46905420936b439953 > core/src/main/scripts/generate-protobuf.sh > 9a3b08a34b4eed50b74bf502de1bc64daa80418a > core/src/main/scripts/generate-thrift.sh > ee51b11a921205e6164bf6c5303d874fd4278ee4 > proxy/examples/python/TestClient.py > 5509ded0946a16455eea8b570a96a71ba15d4ebf > start/src/test/shell/makeTestJars.sh > 13018b1973fbddf1b8885f156d0c6d7196574d14 > test/src/test/scripts/run-test.sh 02a5090e622fdd22b0239cd4a48117067878f94c > test/system/continuous/run-moru.sh 3133b0518c9153c14d8cb1a8e78adc7ab1dc0333 > test/system/continuous/run-verify.sh > 72ecc14fecf18ab8e8f14c8e2420e8f0102701f3 > test/system/continuous/start-agitator.sh > e82e5ab66866122161a0b6e1be132114b42e94da > test/system/continuous/start-batchwalkers.sh > b3dd7becf677b28d81349f3622cb18d612d786d0 > test/system/continuous/start-ingest.sh > 468482935dcddf63a87b23f2695c0b6c057f3ffb > test/system/continuous/start-scanners.sh > 63b7fb318ddb3ae9d1c396bee493083f6403d1b1 > test/system/continuous/start-stats.sh > a9013c44f942eaf3bc54096059b02ae9dfe6e503 > test/system/continuous/start-walkers.sh > 32fbd515b71a75f6ebb36e437be8babe1c3a1504 > test/system/continuous/stop-agitator.sh > 136b451f2303b5900cdfd746956752ff4d96fa54 > test/system/continuous/stop-batchwalkers.sh > 0250656291572bbb9c10faa15bcee6872aada587 > test/system/continuous/stop-ingest.sh > 15f1715ebfc128a825ae65abf8262f5af6abd442 > test/system/continuous/stop-scanners.sh > 8b1852b39df4dad803ee9c04ac8a08699e06d83c > test/system/continuous/stop-stats.sh > a806f2d30e13bae929678318b075a1899f8cef26 > test/system/continuous/stop-walkers.sh > 7e759698833e04b54592ad201c702cdfe3df10a8 > test/system/randomwalk/bin/apocalypse.sh > 899256c8521bd7c319533ed8fd24676a367b65d6 > test/system/randomwalk/bin/copy-config.sh > 01a90e0579c31200fca8b91673cf865e36d6daf0 > test/system/randomwalk/bin/kill-all.sh > 75e73af8d5b5294a98e01ab96bb246bb8f95f89c > test/system/randomwalk/bin/kill-local.sh > 1d5af9b0545c640cad2e258e549099b4ec5ea199 > test/system/randomwalk/bin/reset-cluster.sh > d3942220181a9e150177818c2dad447f620acaad > test/system/randomwalk/bin/start-all.sh > 48219c6ea72cff08811ef5fadbc074c51c99f8e2 > test/system/randomwalk/bin/start-local.sh > 524a80b1ec0e4696332dd607be2f57354e5887dc > test/system/upgrade_test.sh 6259e1c3f267947facd7c712c5fa7ae3b114c242 > > Diff: https://reviews.apache.org/r/23393/diff/ > > > Testing > ------- > > > Thanks, > > Mike Drob > >
