You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "dlmarion (via GitHub)" <gi...@apache.org> on 2024/11/06 15:27:07 UTC
[PR] Added dry run option to accumulo-cluster script [accumulo]
dlmarion opened a new pull request, #5035:
URL: https://github.com/apache/accumulo/pull/5035
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added dry run option to accumulo-cluster script [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5035:
URL: https://github.com/apache/accumulo/pull/5035#discussion_r1831550873
##########
assemble/bin/accumulo-cluster:
##########
@@ -590,7 +626,22 @@ function main() {
accumulo_cmd="${bin}/accumulo"
SSH='ssh -qnf -o ConnectTimeout=2'
- case "$1" in
+ # Copy input arguments into new array
+ # removing any options
+ DEBUG=""
Review Comment:
Implemented this in 24b3356.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added dry run option to accumulo-cluster script [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5035:
URL: https://github.com/apache/accumulo/pull/5035#discussion_r1831551967
##########
assemble/bin/accumulo-cluster:
##########
@@ -157,16 +161,33 @@ function control_service() {
# using the value of $host
#
if [[ $# -gt 3 ]]; then
- ACCUMULO_SERVICE_INSTANCE="${ACCUMULO_SERVICE_INSTANCE}" "${bin}/accumulo-service" "$service" "$control_cmd" "-a" "$host" "${@:4}"
+ EXTRA_ARGS="${*:4}"
Review Comment:
I backed out this change in 24b3356.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added dry run option to accumulo-cluster script [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5035:
URL: https://github.com/apache/accumulo/pull/5035#discussion_r1831551599
##########
assemble/bin/accumulo-cluster:
##########
@@ -590,7 +626,22 @@ function main() {
accumulo_cmd="${bin}/accumulo"
SSH='ssh -qnf -o ConnectTimeout=2'
- case "$1" in
+ # Copy input arguments into new array
+ # removing any options
+ DEBUG=""
+ i=0
+ declare -a program_args
+ for arg in "$@"; do
+ if [[ $arg == "--dry-run" ]]; then
+ DEBUG="true"
+ else
+ program_args[i++]="$arg"
+ fi
+ done
Review Comment:
I left as is. I'm trying to allow `--dry-run` to be placed anywhere in the input.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added dry run option to accumulo-cluster script [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5035:
URL: https://github.com/apache/accumulo/pull/5035#discussion_r1833335507
##########
assemble/bin/accumulo-cluster:
##########
@@ -157,16 +161,33 @@ function control_service() {
# using the value of $host
#
if [[ $# -gt 3 ]]; then
- ACCUMULO_SERVICE_INSTANCE="${ACCUMULO_SERVICE_INSTANCE}" "${bin}/accumulo-service" "$service" "$control_cmd" "-a" "$host" "${@:4}"
+ EXTRA_ARGS="${*:4}"
+ if [[ -n $DEBUG ]]; then
+ echo "ACCUMULO_SERVICE_INSTANCE=\"${ACCUMULO_SERVICE_INSTANCE}\" \"${bin}/accumulo-service\" \"$service\" \"$control_cmd\" \"-a\" \"$host\" $EXTRA_ARGS"
+ else
+ ACCUMULO_SERVICE_INSTANCE="${ACCUMULO_SERVICE_INSTANCE}" "${bin}/accumulo-service" "$service" "$control_cmd" "-a" "$host" "$EXTRA_ARGS"
Review Comment:
I added an ignore for the shell check warning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added dry run option to accumulo-cluster script [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5035:
URL: https://github.com/apache/accumulo/pull/5035#discussion_r1831550295
##########
assemble/bin/accumulo-cluster:
##########
@@ -157,16 +161,33 @@ function control_service() {
# using the value of $host
#
if [[ $# -gt 3 ]]; then
- ACCUMULO_SERVICE_INSTANCE="${ACCUMULO_SERVICE_INSTANCE}" "${bin}/accumulo-service" "$service" "$control_cmd" "-a" "$host" "${@:4}"
+ EXTRA_ARGS="${*:4}"
+ if [[ -n $DEBUG ]]; then
+ echo "ACCUMULO_SERVICE_INSTANCE=\"${ACCUMULO_SERVICE_INSTANCE}\" \"${bin}/accumulo-service\" \"$service\" \"$control_cmd\" \"-a\" \"$host\" $EXTRA_ARGS"
+ else
+ ACCUMULO_SERVICE_INSTANCE="${ACCUMULO_SERVICE_INSTANCE}" "${bin}/accumulo-service" "$service" "$control_cmd" "-a" "$host" "$EXTRA_ARGS"
Review Comment:
shellcheck shows a warning with what I did in 24b3356, but it works. The issue is that the `ACCUMULO_SERVICE_INSTANCE="${ACCUMULO_SERVICE_INSTANCE}"` can't be enclosed in quotes when you execute the command.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added dry run option to accumulo-cluster script [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5035:
URL: https://github.com/apache/accumulo/pull/5035#discussion_r1831498178
##########
assemble/bin/accumulo-cluster:
##########
@@ -590,7 +626,22 @@ function main() {
accumulo_cmd="${bin}/accumulo"
SSH='ssh -qnf -o ConnectTimeout=2'
- case "$1" in
+ # Copy input arguments into new array
+ # removing any options
+ DEBUG=""
+ i=0
+ declare -a program_args
+ for arg in "$@"; do
+ if [[ $arg == "--dry-run" ]]; then
+ DEBUG="true"
+ else
+ program_args[i++]="$arg"
+ fi
+ done
Review Comment:
This parsing becomes a lot simpler if the dry-run flag is expected to be first. Then, you can just use `shift` to strip it off the front.
If the case block below were put into its own function, it would also simplify the use of the `program_args` array, allowing you to work directly with `$@`, `$1`, `$2`, etc. inside the called method.
##########
assemble/bin/accumulo-cluster:
##########
@@ -157,16 +161,33 @@ function control_service() {
# using the value of $host
#
if [[ $# -gt 3 ]]; then
- ACCUMULO_SERVICE_INSTANCE="${ACCUMULO_SERVICE_INSTANCE}" "${bin}/accumulo-service" "$service" "$control_cmd" "-a" "$host" "${@:4}"
+ EXTRA_ARGS="${*:4}"
+ if [[ -n $DEBUG ]]; then
+ echo "ACCUMULO_SERVICE_INSTANCE=\"${ACCUMULO_SERVICE_INSTANCE}\" \"${bin}/accumulo-service\" \"$service\" \"$control_cmd\" \"-a\" \"$host\" $EXTRA_ARGS"
+ else
+ ACCUMULO_SERVICE_INSTANCE="${ACCUMULO_SERVICE_INSTANCE}" "${bin}/accumulo-service" "$service" "$control_cmd" "-a" "$host" "$EXTRA_ARGS"
Review Comment:
It's easy for these two lines to diverge over time. It would probably be useful to create a function that passes the arguments to execute, and optionally does echo instead. That way, you don't have to worry about making sure they stay the same.
Something like:
```bash
function dryRunOrExec() {
if [[ -n $DEBUG ]]; then
echo "${@@Q}"
else
"$@"
fi
}
```
Then, you just use it like:
```bash
dryRunOrExec ACCUMULO_SERVICE_INSTANCE="${ACCUMULO_SERVICE_INSTANCE}" "${bin}/accumulo-service" "$service" "$control_cmd" "-a" "$host" "${@:4}"
```
If that `else` block doesn't work as-is, it might need to use `eval` in some way to handle the `ACCUMULO_SERVICE_INSTANCE="${ACCUMULO_SERVICE_INSTANCE}"` part. Also, this requires bash4 for the `@Q` quoting, so you can copy/paste the output for manual execution. If you prefer without quotes, for readability, and don't care about the quoting for copy/paste safety, then you can just use `echo "$*"`, which will keep things compatible with bash3. However, I'm pretty sure we require bash4 elsewhere in our scripts, and I don't think it's a goal to stay bash3 compatible (Mac OS users can just install bash4 using homebrew or similar).
##########
assemble/bin/accumulo-cluster:
##########
@@ -157,16 +161,33 @@ function control_service() {
# using the value of $host
#
if [[ $# -gt 3 ]]; then
- ACCUMULO_SERVICE_INSTANCE="${ACCUMULO_SERVICE_INSTANCE}" "${bin}/accumulo-service" "$service" "$control_cmd" "-a" "$host" "${@:4}"
+ EXTRA_ARGS="${*:4}"
Review Comment:
I'm not sure this `EXTRA_ARGS` is working the same as it did before. Here, it is flattened to a scalar and used as a single argument, whereas before, `"${@:4}"` was a sliced array and every extra argument would be considered its own argument. This will probably break things like using `-o key1=value1 -o key2=value2`, etc.
This is very different than the other place where `EXTRA_ARGS` is used in a very carefully quoted way when passing over SSH, so that each argument is treated as separate.
##########
assemble/bin/accumulo-cluster:
##########
@@ -590,7 +626,22 @@ function main() {
accumulo_cmd="${bin}/accumulo"
SSH='ssh -qnf -o ConnectTimeout=2'
- case "$1" in
+ # Copy input arguments into new array
+ # removing any options
+ DEBUG=""
Review Comment:
It's common to use variables like this by setting them to a numeric value, 0 or 1. Instead of checking that it is non-zero length (`-n $DEBUG`), it'd probably be better to check that it has been been explicitly set to `1` (`$DEBUG == 1`; this is string equality, not numeric equality, but that's fine, since it's just a toggle, and `==` looks better than `eq` for this comparison), and initializing it to `0` here.
```suggestion
DEBUG=0
```
You could also create another function to use to check the global variable instead of comparing the variable each time:
```bash
function isDebug() {
[[ $DEBUG == 1 ]]
}
isDebug && echo "..."
if ! isDebug; then
# do something if debug is not on
fi
```
This isn't a huge benefit, but might improve readability in a few places.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added dry run option to accumulo-cluster script [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion merged PR #5035:
URL: https://github.com/apache/accumulo/pull/5035
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added dry run option to accumulo-cluster script [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #5035:
URL: https://github.com/apache/accumulo/pull/5035#issuecomment-2465132401
I think this is ready for merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org