accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Mike Drob" <md...@mdrob.com>
Subject Re: Review Request 23393: ACCUMULO-2423 Clean up (most?) shell scripts
Date Thu, 10 Jul 2014 16:53:08 GMT


> On July 10, 2014, 3:16 p.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)

I'd like to handle that in a follow on issue.


> On July 10, 2014, 3:16 p.m., Christopher Tubbs wrote:
> > assemble/bin/bootstrap_hdfs.sh, line 83
> > <https://reviews.apache.org/r/23393/diff/1/?file=627707#file627707line83>
> >
> >     dirname arg in bin/accumulo was quoted. Should these args also be quoted?

Yep.


> On July 10, 2014, 3:16 p.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 [[ ]]

While this is true, extra quoting never hurts so I'm inclined to leave it.


> On July 10, 2014, 3:16 p.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.

No, it's necessary because there could be a blank line in the file.


> On July 10, 2014, 3:16 p.m., Christopher Tubbs wrote:
> > assemble/bin/stop-all.sh, line 48
> > <https://reviews.apache.org/r/23393/diff/1/?file=627712#file627712line48>
> >
> >     more grep filtering inconsistency on host files

Follow on issue, see above.


> On July 10, 2014, 3:16 p.m., Christopher Tubbs wrote:
> > test/system/upgrade_test.sh, line 19
> > <https://reviews.apache.org/r/23393/diff/1/?file=627743#file627743line19>
> >
> >     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.

I don't understand the rationale behind the suggestion here.


> On July 10, 2014, 3:16 p.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.

$@ is an array, and thus needs to be quoted.


> On July 10, 2014, 3:16 p.m., Christopher Tubbs wrote:
> > assemble/bin/accumulo, lines 32-36
> > <https://reviews.apache.org/r/23393/diff/1/?file=627706#file627706line32>
> >
> >     This part can probably be removed since ACCUMULO-2606 happened, but it doesn't
matter that much.

Killed it.


- Mike


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


On July 10, 2014, 2:07 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, 2:07 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 (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