ctubbsii commented on code in PR #4763:
URL: https://github.com/apache/accumulo/pull/4763#discussion_r1692274004
##########
assemble/bin/accumulo-service:
##########
@@ -91,27 +91,78 @@ function start_service() {
fi
}
-function stop_service() {
+function control_process() {
+ kill_code=${1}
if [[ -f $pid_file ]]; then
echo "Stopping $service on $host"
- kill -s TERM "$(cat "$pid_file")" 2>/dev/null
+ kill -s "$kill_code" "$(cat "$pid_file")" 2>/dev/null
rm -f "${pid_file}" 2>/dev/null
fi
}
+function stop_service() {
+ if [[ -n $1 ]]; then
+ case $1 in
+ "--all")
+ find_processes
+ for process in "${RUNNING_PROCESSES[@]}"; do
+ pid_file="${ACCUMULO_PID_DIR}/accumulo-${process}.pid"
+ control_process "TERM"
+ done
+ ;;
+ *"$service"*)
+ echo "Stopping service process: $1"
+ pid_file="${ACCUMULO_PID_DIR}/accumulo-${1}.pid"
+ ;;
+ esac
+ fi
+ control_process "TERM"
+}
+
function kill_service() {
- if [[ -f $pid_file ]]; then
- echo "Killing $service on $host"
- kill -s KILL "$(cat "$pid_file")" 2>/dev/null
- rm -f "${pid_file}" 2>/dev/null
+ if [[ -n $1 ]]; then
+ case $1 in
+ "--all")
+ find_processes
+ for process in "${RUNNING_PROCESSES[@]}"; do
+ pid_file="${ACCUMULO_PID_DIR}/accumulo-${process}.pid"
+ control_process "KILL"
+ done
+ ;;
+ *"$service"*)
+ pid_file="${ACCUMULO_PID_DIR}/accumulo-${1}.pid"
+ ;;
+ esac
fi
+ control_process "KILL"
+}
+
+function find_processes() {
+ files=$(echo "${ACCUMULO_PID_DIR}"/*)
+ for file in $files; do
Review Comment:
This will not handle edge cases well. You should do this instead:
```suggestion
local file
for file in $ACCUMULO_PID_DIR/*; do
```
Making the variable local is optional, but strongly recommended to always
use local variables when possible.
##########
assemble/bin/accumulo-service:
##########
@@ -159,10 +210,13 @@ function main() {
start_service "${@:3}"
;;
stop)
- stop_service
+ stop_service "${@:3:1}"
;;
kill)
- kill_service
+ kill_service "${@:3:1}"
Review Comment:
The array slicing is a bit confusing here. A lot of people don't know bash
well.
Since this stuff exists in a function called `main`, and the first argument
is checked right away, it'd be a good idea to just assign that first argument
to a named local variable `local service=$1` and then shift the arg array
`shift` so the remaining args are everything that's left. That would probably
simplify the rest of the references to the remaining args.
##########
assemble/bin/accumulo-cluster:
##########
@@ -168,6 +176,90 @@ function start_service() {
control_service start "$@"
}
+function start_compactors() {
+ echo -n "Starting compactor servers ..."
+ queues=$COMPACTION_QUEUES
+ if [[ -n $1 ]]; then
+ queues="$1"
+ echo "Only starting servers for group: ${queues}"
+ fi
+ for queue in $queues; do
+ var_name="NUM_COMPACTORS_${queue}"
+ [[ -n ${!var_name} ]] && NUM_COMPACTORS=${!var_name}
+ Q="COMPACTOR_HOSTS_${queue}"
+ if [[ -n ${!Q} ]]; then
+ for compactor in ${!Q}; do
+ start_service "$compactor" compactor "-q" "$queue"
+ done
+ else
+ echo "\"${queue}\" is not a valid queue ...exiting"
Review Comment:
This one has both quotes, but one of the others has none, and another has
only one. Again, single quotes are probably fine.
##########
assemble/bin/accumulo-cluster:
##########
@@ -156,9 +164,9 @@ function control_service() {
else
if [[ $# -gt 3 ]]; then
EXTRA_ARGS="${*:4}"
- $SSH "$host" "bash -c
'ACCUMULO_SERVICE_INSTANCE=${ACCUMULO_SERVICE_INSTANCE} ${bin}/accumulo-service
\"$service\" \"$control_cmd\" \"-a\" \"$host\" $EXTRA_ARGS '"
+ $SSH "$host" "bash -c
'ACCUMULO_SERVICE_INSTANCE=${ACCUMULO_SERVICE_INSTANCE} ${bin}/accumulo-service
\"$service\" \"$control_cmd\" \"-a\" \"$host\" $EXTRA_ARGS '&"
else
- $SSH "$host" "bash -c
'ACCUMULO_SERVICE_INSTANCE=${ACCUMULO_SERVICE_INSTANCE} ${bin}/accumulo-service
\"$service\" \"$control_cmd\" \"-a\" \"$host\"'"
+ $SSH "$host" "bash -c
'ACCUMULO_SERVICE_INSTANCE=${ACCUMULO_SERVICE_INSTANCE} ${bin}/accumulo-service
\"$service\" \"$control_cmd\" \"-a\" \"$host\"'&"
Review Comment:
I'm not so sure about having `&` on the end of this. If the command is
backgrounded, it might close the SSH connection, which will terminate the
command.
##########
assemble/bin/accumulo-service:
##########
@@ -91,27 +91,78 @@ function start_service() {
fi
}
-function stop_service() {
+function control_process() {
+ kill_code=${1}
if [[ -f $pid_file ]]; then
echo "Stopping $service on $host"
- kill -s TERM "$(cat "$pid_file")" 2>/dev/null
+ kill -s "$kill_code" "$(cat "$pid_file")" 2>/dev/null
rm -f "${pid_file}" 2>/dev/null
fi
}
+function stop_service() {
+ if [[ -n $1 ]]; then
+ case $1 in
+ "--all")
+ find_processes
+ for process in "${RUNNING_PROCESSES[@]}"; do
+ pid_file="${ACCUMULO_PID_DIR}/accumulo-${process}.pid"
+ control_process "TERM"
+ done
+ ;;
+ *"$service"*)
+ echo "Stopping service process: $1"
+ pid_file="${ACCUMULO_PID_DIR}/accumulo-${1}.pid"
+ ;;
+ esac
+ fi
+ control_process "TERM"
+}
+
function kill_service() {
- if [[ -f $pid_file ]]; then
- echo "Killing $service on $host"
- kill -s KILL "$(cat "$pid_file")" 2>/dev/null
- rm -f "${pid_file}" 2>/dev/null
+ if [[ -n $1 ]]; then
+ case $1 in
+ "--all")
+ find_processes
+ for process in "${RUNNING_PROCESSES[@]}"; do
+ pid_file="${ACCUMULO_PID_DIR}/accumulo-${process}.pid"
+ control_process "KILL"
+ done
+ ;;
+ *"$service"*)
+ pid_file="${ACCUMULO_PID_DIR}/accumulo-${1}.pid"
+ ;;
+ esac
fi
+ control_process "KILL"
+}
+
+function find_processes() {
+ files=$(echo "${ACCUMULO_PID_DIR}"/*)
+ for file in $files; do
+ if [[ $file == *$service* ]]; then
+ stripped_file=${file%.pid}
+ RUNNING_PROCESSES+=("${stripped_file##*accumulo-}")
+ fi
Review Comment:
This logic can be made much more secure against edge cases and much simpler
to parse while matching:
```suggestion
if file=$(expr "$file" : '^.*/accumulo-\('"$service"'.*\)[.]pid$'); then
RUNNING_PROCESSES+=("$file")
fi
```
However, my concern here is that `$service` is not a local variable and I
have no idea what it might contain at this point. It's still better than the
current multi-step stripping and parsing, but a lot of this code should use
more local variables in the functions, and explicitly passing parameters to the
functions, rather than rely on global variables being set when a particular
function is called.
Also, regarding the regex pattern... it could be made better by adding word
boundaries at the end of `$service`, so, a service named `prefix` doesn't match
`accumulo-prefixsuffix.pid` but I don't know if bash supports that easily.
Alternatively, you could replace the `.*` inside the parens after `$service`
with whatever syntax is expected after the service name... so it's a bit more
strict and does an exact match instead of wildcarding there.
##########
assemble/bin/accumulo-cluster:
##########
@@ -115,15 +119,19 @@ function parse_config {
echo "INFO: ${NUM_TSERVERS} tservers will be started per host"
fi
- # shellcheck disable=SC2153
- if [[ -z $NUM_SSERVERS ]]; then
- echo "INFO: ${NUM_SSERVERS} sservers will be started per host"
- fi
-
- if [[ -z $NUM_COMPACTORS ]]; then
- echo "INFO: ${NUM_COMPACTORS} compactors will be started per host"
- fi
+ for group in $SSERVER_GROUPS; do
Review Comment:
You might still need the shellcheck comment. It suppresses
https://www.shellcheck.net/wiki/SC2153
##########
assemble/bin/accumulo-service:
##########
@@ -91,27 +91,78 @@ function start_service() {
fi
}
-function stop_service() {
+function control_process() {
+ kill_code=${1}
if [[ -f $pid_file ]]; then
echo "Stopping $service on $host"
- kill -s TERM "$(cat "$pid_file")" 2>/dev/null
+ kill -s "$kill_code" "$(cat "$pid_file")" 2>/dev/null
rm -f "${pid_file}" 2>/dev/null
fi
}
+function stop_service() {
+ if [[ -n $1 ]]; then
+ case $1 in
+ "--all")
+ find_processes
+ for process in "${RUNNING_PROCESSES[@]}"; do
+ pid_file="${ACCUMULO_PID_DIR}/accumulo-${process}.pid"
+ control_process "TERM"
+ done
+ ;;
+ *"$service"*)
Review Comment:
This is a very broad wildcard match. At the very least, I think it always
starts with the service name, so you should drop the wildcard at the beginning.
After the service name, perhaps you can put a stricter pattern?
##########
assemble/bin/accumulo-cluster:
##########
@@ -168,6 +176,90 @@ function start_service() {
control_service start "$@"
}
+function start_compactors() {
+ echo -n "Starting compactor servers ..."
+ queues=$COMPACTION_QUEUES
+ if [[ -n $1 ]]; then
+ queues="$1"
+ echo "Only starting servers for group: ${queues}"
+ fi
+ for queue in $queues; do
+ var_name="NUM_COMPACTORS_${queue}"
+ [[ -n ${!var_name} ]] && NUM_COMPACTORS=${!var_name}
+ Q="COMPACTOR_HOSTS_${queue}"
+ if [[ -n ${!Q} ]]; then
+ for compactor in ${!Q}; do
+ start_service "$compactor" compactor "-q" "$queue"
+ done
+ else
+ echo "\"${queue}\" is not a valid queue ...exiting"
+ fi
+ done
+}
+
+function stop_compactors() {
+ echo "Stopping compactor servers ..."
+ queues=$COMPACTION_QUEUES
+ if [[ -n $1 ]]; then
+ queues="$1"
+ echo "Only stopping servers for group: ${queues}"
+ fi
+ for queue in $queues; do
+ var_name="NUM_COMPACTORS_${queue}"
+ [[ -n ${!var_name} ]] && NUM_COMPACTORS=${!var_name}
+ Q="COMPACTOR_HOSTS_${queue}"
+ if [[ -n ${!Q} ]]; then
+ for compactor in ${!Q}; do
+ stop_service "$compactor" compactor "-q" "$queue"
+ done
+ else
+ echo "$queue is not a valid compaction queue ...exiting"
+ fi
+ done
+}
+
+function start_sservers() {
+ echo "Starting scan servers ..."
+ groups=$SSERVER_GROUPS
+ if [[ -n $1 ]]; then
+ groups="$1"
+ echo "Only starting servers for group: ${groups}"
+ fi
+ for group in $groups; do
+ var_name="NUM_SSERVERS_${group}"
+ [[ -n ${!var_name} ]] && NUM_SSERVERS=${!var_name}
+ G="SSERVER_HOSTS_${group}"
+ if [[ -n ${!G} ]]; then
+ for sserver in ${!G}; do
+ start_service "$sserver" sserver "-g" "$group"
+ done
+ else
+ echo "\"${group} is not a valid resource group ...exiting"
Review Comment:
It looks like there's a missing closing quote after the group name? Also,
single quotes are fine to avoid unnecessary escaping. Also, the same comment
about queues above didn't have any quotes. It'd be good to make them consistent.
##########
assemble/bin/accumulo-cluster:
##########
@@ -23,16 +23,20 @@ function print_usage {
Usage: accumulo-cluster <command> (<argument> ...)
Commands:
- create-config Creates cluster config
- restart Restarts the Accumulo cluster
- start Starts Accumulo cluster
- stop Stops Accumulo cluster
- kill Kills Accumulo cluster
- start-non-tservers Starts all services except tservers
- start-tservers Starts all tservers on cluster
- stop-tservers Stops all tservers on cluster
- start-here Starts all services on this node
- stop-here Stops all services on this node
+ create-config Creates cluster config
+ restart Restarts the Accumulo cluster
+ start Starts Accumulo cluster
+ stop Stops Accumulo cluster
+ kill Kills Accumulo cluster
+ start-non-tservers Starts all services except tservers (Deprecated)
+ start-servers [--all|--tservers|--no-tservers|--sservers
[group]|--compactors [queue]]
+ Starts various server types, can optionally
specify a queue or group
+ stop-servers [--all|--tservers| --no-tservers|--sservers
[group]|--compactors [queue]]
+ Starts various server types, can optionally
specify a queue or group
+ start-tservers Starts all tservers on cluster (Deprecated)
Review Comment:
It might be easier to see if "Deprecated" moves to the front of the command,
as in:
```suggestion
start-tservers Deprecated. Starts all tservers on cluster
```
This helps it be seen more easily because it has a fixed length and always
appears at the same position on the line. At the end, it's harder to notice
because descriptions vary in length, so the position changes. Plus, it might
scroll outside of view, depending on the user's terminal window settings.
##########
assemble/bin/accumulo-cluster:
##########
@@ -202,6 +294,8 @@ function start_all() {
done
for group in $SSERVER_GROUPS; do
+ var_name="NUM_SSERVERS_${group}"
+ [[ -n ${!var_name} ]] && NUM_SSERVERS=${!var_name}
Review Comment:
The variable indirection also lends itself to possibly consolidating and
reusing a lot of these loops in a function. Doesn't have to be done here...
just an idea for subsequent improvement.
##########
assemble/bin/accumulo-service:
##########
@@ -91,27 +91,78 @@ function start_service() {
fi
}
-function stop_service() {
+function control_process() {
+ kill_code=${1}
Review Comment:
Code looks cleaner without the unnecessary use of curlies. Also, local
variable FTW
```suggestion
local kill_code=$1
```
##########
assemble/bin/accumulo-cluster:
##########
@@ -23,16 +23,20 @@ function print_usage {
Usage: accumulo-cluster <command> (<argument> ...)
Commands:
- create-config Creates cluster config
- restart Restarts the Accumulo cluster
- start Starts Accumulo cluster
- stop Stops Accumulo cluster
- kill Kills Accumulo cluster
- start-non-tservers Starts all services except tservers
- start-tservers Starts all tservers on cluster
- stop-tservers Stops all tservers on cluster
- start-here Starts all services on this node
- stop-here Stops all services on this node
+ create-config Creates cluster config
+ restart Restarts the Accumulo cluster
+ start Starts Accumulo cluster
+ stop Stops Accumulo cluster
+ kill Kills Accumulo cluster
+ start-non-tservers Starts all services except tservers (Deprecated)
Review Comment:
Does this include scan servers? If not, perhaps the description should be
updated to exclude those also?
##########
assemble/bin/accumulo-cluster:
##########
@@ -138,10 +146,10 @@ function control_service() {
[[ $service == "compactor" ]] && last_instance_id=${NUM_COMPACTORS:-1}
for ((inst_id = 1; inst_id <= last_instance_id; inst_id++)); do
+ # Only increment the service name when more than one service is desired.
ACCUMULO_SERVICE_INSTANCE=""
- [[ $service == "tserver" && ${NUM_TSERVERS:-1} -gt 1 ]] &&
ACCUMULO_SERVICE_INSTANCE=${inst_id}
- [[ $service == "compactor" ]] &&
ACCUMULO_SERVICE_INSTANCE="${inst_id}_${5}"
- [[ $service == "sserver" ]] && ACCUMULO_SERVICE_INSTANCE="${inst_id}_${5}"
+ [[ last_instance_id -gt 1 ]] && ACCUMULO_SERVICE_INSTANCE="${inst_id}"
+ [[ $service == "compactor" || $service == "sserver" ]] &&
ACCUMULO_SERVICE_INSTANCE="${ACCUMULO_SERVICE_INSTANCE}_${5}"
Review Comment:
You could also do:
```suggestion
[[ $service =~ ^compactor|sserver$ ]] &&
ACCUMULO_SERVICE_INSTANCE="${ACCUMULO_SERVICE_INSTANCE}_${5}"
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
