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 (most?) shell scripts
Date Thu, 10 Jul 2014 15:16:50 GMT

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


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