accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Christopher Tubbs" <ctubbsii...@apache.org>
Subject Re: Review Request 23393: ACCUMULO-2423 Clean up shell scripts
Date Thu, 10 Jul 2014 20:14:33 GMT


> On July 10, 2014, 11:16 a.m., Christopher Tubbs wrote:
> > assemble/bin/bootstrap_hdfs.sh, line 81
> > <https://reviews.apache.org/r/23393/diff/1/?file=627707#file627707line81>
> >
> >     Should probably
> >     s/cat/grep -v '^#'/
> >     
> >     and double-quote the arg (if necessary, as is seems to be done elsewhere)
> 
> Mike Drob wrote:
>     I'd like to handle that in a follow on issue.

Works for me.


> On July 10, 2014, 11:16 a.m., Christopher Tubbs wrote:
> > assemble/bin/build_native_library.sh, line 20
> > <https://reviews.apache.org/r/23393/diff/1/?file=627708#file627708line20>
> >
> >     Don't need to quote bash vars in [[ ]]
> 
> Mike Drob wrote:
>     While this is true, extra quoting never hurts so I'm inclined to leave it.

That's fine with me, but you're forcing bash to evaluate it after it has first expanded the
string expression, instead of directly comparing against the value of the variable. I was
under the impression that was the main optimization between single braces [ ] and double [[
]], and one of the biggest reasons to make this kind of switch.


> On July 10, 2014, 11:16 a.m., Christopher Tubbs wrote:
> > assemble/bin/build_native_library.sh, line 59
> > <https://reviews.apache.org/r/23393/diff/1/?file=627708#file627708line59>
> >
> >     Don't need to quote assignments.
> 
> Mike Drob wrote:
>     $@ is an array, and thus needs to be quoted.

I did some quick testing and it did not seem like it made a difference.


> On July 10, 2014, 11:16 a.m., Christopher Tubbs wrote:
> > assemble/bin/start-all.sh, line 67
> > <https://reviews.apache.org/r/23393/diff/1/?file=627710#file627710line67>
> >
> >     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.
> 
> Mike Drob wrote:
>     No, it's necessary because there could be a blank line in the file.
> 
> Christopher Tubbs wrote:
>     Doesn't matter. for loops split on all space and don't iterate over empty results.
That is, unless you change IFS to make it split only on newlines (e.g. IFS=$'\n'; for x in
$(); do ...; done)
> 
> Mike Drob wrote:
>     shellcheck warns:
>     
>     for f in $(grep -v '^#' "$ACCUMULO_CONF_DIR/slaves")
>              ^-- SC2013: To read lines rather than words, pipe/redirect to a 'while read'
loop.
>     
>     I think we can leave it for a follow-on issue.

I'm surprised it's not making those complaints about cat also. Either way, I think you'd have
to modify IFS to deal with this properly. (otherwise, we're essentially saying that it's okay
to put extra hosts on the same line, space-separated.


- Christopher


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23393/#review47566
-----------------------------------------------------------


On July 10, 2014, 4:04 p.m., Mike Drob wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23393/
> -----------------------------------------------------------
> 
> (Updated July 10, 2014, 4:04 p.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 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 double quotes to prevent word splitting
> 
> 
> Diffs
> -----
> 
>   assemble/bin/LogForwarder.sh 5fe7871fc4b8b6aab9dbfc0227aa2e36f4a43aba 
>   assemble/bin/accumulo bdd742d7aee81dc82872acee1b4333ee6e8348c7 
>   assemble/bin/bootstrap_config.sh fb643daf2f98f325bf041a3f75e179df74dfef59 
>   assemble/bin/bootstrap_hdfs.sh 6f38f633f4789ac7bc09639eeee58495ee4c3ea2 
>   assemble/bin/build_native_library.sh 907ce2c946ba1b6c4ddcc889011f899fc1d96575 
>   assemble/bin/config.sh 76634731abd2e51dc924804f76c1b73a9f5086d6 
>   assemble/bin/generate_monitor_certificate.sh edd415912bb0c67e203236122f5117fad88bf34e

>   assemble/bin/start-all.sh b71ba705889f6c157194ae4a816f8e21e9377b8b 
>   assemble/bin/start-here.sh 9abdb387273c7e7fed24928da7ba11183e4f5986 
>   assemble/bin/start-server.sh 62ad91d57731d7e32b3ce6e51a961b11c64bf163 
>   assemble/bin/stop-all.sh 3bd30bd758a9226d7e1e8e0b66cd36704e052d0e 
>   assemble/bin/stop-here.sh 4d5533acc6b4b4fc4aadde97ae95906ece347492 
>   assemble/bin/stop-server.sh 6fbe0afde2b881cb7ded6b66d6fcaad35102d7cd 
>   assemble/bin/tool.sh 8126b3b1a5753a395a21f47fe7849de79af05135 
>   assemble/bin/tup.sh b26def5790cf322cbd203fd53faa337d3cec617d 
>   assemble/build.sh 5f65419dcff6120dc6e7ca46905420936b439953 
>   assemble/conf/templates/accumulo-env.sh e136a3f41a86ad710e1f93f15dc1dfadf3a4de03 
>   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/analyze-missing.sh 62ad15b6e7a6a1fe5b707e9f3eced64763b002c1

>   test/system/continuous/continuous-env.sh.example d3fdd62768afafc496d4e94ecadfe26c0dc84454

>   test/system/continuous/mapred-setup.sh e6bd2caa8cb2557c23b17740f89102482f4300b0 
>   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/stress/reader.sh 11386850e45073b2620ae19d11d1930b529f7fff 
>   test/system/stress/start-readers.sh 709c46e27adbee35c137da837f48ea5de2508545 
>   test/system/stress/start-writers.sh b9464ccf8a39945cc1fe09cc5246df3f5bc4af0b 
>   test/system/stress/stop-readers.sh 1c5997185a66b952bc3eb2c95e2a96bbb31b151e 
>   test/system/stress/stop-writers.sh d925cb331118d0446728f849a5fab5e6e1fd6389 
>   test/system/stress/writer.sh ef26fed1c0761aadd2d69b9cd14970c3298f2969 
>   test/system/upgrade_test.sh 6259e1c3f267947facd7c712c5fa7ae3b114c242 
> 
> Diff: https://reviews.apache.org/r/23393/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Mike Drob
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message