You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "ddanielr (via GitHub)" <gi...@apache.org> on 2024/07/25 21:47:53 UTC
[PR] Improves accumulo-cluster for start and stop commands [accumulo]
ddanielr opened a new pull request, #4763:
URL: https://github.com/apache/accumulo/pull/4763
Adds the ability to start and stop compactors and scan servers by compaction queue or scan server group.
This is important when attempting to run scan servers or compactors and not want to stop/start other accumulo services (gc, monitor, manager, compaction-coordinator).
--
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] Improves accumulo-cluster for start and stop commands [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4763:
URL: https://github.com/apache/accumulo/pull/4763#discussion_r1693702997
##########
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:
and agreed, using something like pssh or pdsh would be simpler
--
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] Improves accumulo-cluster for start and stop commands [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4763:
URL: https://github.com/apache/accumulo/pull/4763#discussion_r1693662554
##########
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:
That makes sense. I'll move that to the front of the usage statement.
--
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] Improves accumulo-cluster for start and stop commands [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #4763:
URL: https://github.com/apache/accumulo/pull/4763#discussion_r1694280169
##########
assemble/bin/accumulo-service:
##########
@@ -137,40 +192,61 @@ function main() {
mkdir -p "$ACCUMULO_LOG_DIR" 2>/dev/null
mkdir -p "$ACCUMULO_PID_DIR" 2>/dev/null
- host="$(hostname)"
- if [[ -z $host ]]; then
- host=$(ip addr | grep 'state UP' -A2 | tail -n1 | awk '{print $2}' | cut -f1 -d'/')
+ HOST="$(hostname)"
+ if [[ -z $HOST ]]; then
+ HOST=$(ip addr | grep 'state UP' -A2 | tail -n1 | awk '{print $2}' | cut -f1 -d'/')
fi
- service="$1"
- if [[ $service == "master" ]]; then
+ local service_type="$1"
+ local command_name="$2"
+ shift 2
+ local service_name="unknown"
+ local all_flag=false
+
+ if [[ $service_type == "master" ]]; then
echo "WARN : Use of 'master' service name is deprecated; use 'manager' instead."
- service="manager"
+ service_type="manager"
+ fi
+
+ # Check and see if accumulo-cluster is calling this script
+ if [ -z ${ACCUMULO_SERVICE_INSTANCE+x} ]; then
+ # The rest of the arguments are from a user
Review Comment:
This looks like a bug. This is going always going to be false even if `ACCUMULO_SERVICE_INSTANCE` is defined as an empty string / null value because the alternate value of `x` is always substituted in those cases.
See https://tldp.org/LDP/abs/html/parameter-substitution.html#PARAMALTV
Also, it'd be a good idea to just the double square bash braces just for consistency, and to make it clear that we're using bash's built-in language syntax and not the external standalone executable at `/usr/bin/[` that traditionally is executed with basic earlier POSIX shells.
I think this should be:
```suggestion
if [[ -z $ACCUMULO_SERVICE_INSTANCE ]]; then
# The rest of the arguments are from a user
```
##########
assemble/bin/accumulo-service:
##########
@@ -137,40 +192,61 @@ function main() {
mkdir -p "$ACCUMULO_LOG_DIR" 2>/dev/null
mkdir -p "$ACCUMULO_PID_DIR" 2>/dev/null
- host="$(hostname)"
- if [[ -z $host ]]; then
- host=$(ip addr | grep 'state UP' -A2 | tail -n1 | awk '{print $2}' | cut -f1 -d'/')
+ HOST="$(hostname)"
+ if [[ -z $HOST ]]; then
+ HOST=$(ip addr | grep 'state UP' -A2 | tail -n1 | awk '{print $2}' | cut -f1 -d'/')
fi
- service="$1"
- if [[ $service == "master" ]]; then
+ local service_type="$1"
+ local command_name="$2"
+ shift 2
+ local service_name="unknown"
+ local all_flag=false
+
+ if [[ $service_type == "master" ]]; then
echo "WARN : Use of 'master' service name is deprecated; use 'manager' instead."
- service="manager"
+ service_type="manager"
+ fi
+
+ # Check and see if accumulo-cluster is calling this script
+ if [ -z ${ACCUMULO_SERVICE_INSTANCE+x} ]; then
+ # The rest of the arguments are from a user
+ if [[ $1 == "--all" ]]; then
+ all_flag=true
+ else
+ # A named service has been specified
+ service_name="$1"
+ fi
+ # Use the special bash env var from accumulo-cluster
+ else
+ service_name=${service_type}${ACCUMULO_SERVICE_INSTANCE}
fi
- pid_file="${ACCUMULO_PID_DIR}/accumulo-${service}${ACCUMULO_SERVICE_INSTANCE}.pid"
- case "$service" in
+ case "$service_type" in
gc | manager | monitor | tserver | compaction-coordinator | compactor | sserver)
- if [[ -z $2 ]]; then
+ if [ -z ${command_name+z} ]; then
Review Comment:
Just like my other comment, this looks like a bug, because it looks like it will always evaluate to false. I suggest:
```suggestion
if [[ -z $command_name ]]; then
```
##########
assemble/bin/accumulo-service:
##########
@@ -137,40 +192,61 @@ function main() {
mkdir -p "$ACCUMULO_LOG_DIR" 2>/dev/null
mkdir -p "$ACCUMULO_PID_DIR" 2>/dev/null
- host="$(hostname)"
- if [[ -z $host ]]; then
- host=$(ip addr | grep 'state UP' -A2 | tail -n1 | awk '{print $2}' | cut -f1 -d'/')
+ HOST="$(hostname)"
+ if [[ -z $HOST ]]; then
+ HOST=$(ip addr | grep 'state UP' -A2 | tail -n1 | awk '{print $2}' | cut -f1 -d'/')
fi
- service="$1"
- if [[ $service == "master" ]]; then
+ local service_type="$1"
+ local command_name="$2"
+ shift 2
Review Comment:
No change needed here for this comment... I'm just making an observation.
I don't remember ever using the numeric option on shift before. I probably would have written this as:
```bash
local service_type="$1"
local command_name="$2"
shift; shift
```
Or even:
```bash
local service_type="$1"; shift
local command_name="$1"; shift
```
I wonder if this is a newer feature of bash or has always been there and I just never noticed.
--
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] Improves accumulo-cluster for start and stop commands [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on PR #4763:
URL: https://github.com/apache/accumulo/pull/4763#issuecomment-2254558665
@ctubbsii I believe I've addressed all the changes that were needed.
--
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] Improves accumulo-cluster for start and stop commands [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4763:
URL: https://github.com/apache/accumulo/pull/4763#discussion_r1693661875
##########
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:
Yes it includes scan servers.
Currently the `start-non-tservers` starts all server types except for tservers.
There's a flag that's passed to the `start_all` function which explicitly excludes the `start_tservers` function.
--
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] Improves accumulo-cluster for start and stop commands [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #4763:
URL: https://github.com/apache/accumulo/pull/4763#discussion_r1693696521
##########
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:
Well, I definitely understand it being faster... I just don't trust it to not kill the command before it's finished starting things up. If you're sure it's always working... it's probably fine. Can keep it and just try to fix it later if it's buggy. I don't care that much. I still see these scripts as more of a minimal reference implementation of a cluster management utility. I'd be somewhat interested in seeing us just use pssh instead, to try to get the parallelism without the risks.
--
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] Improves accumulo-cluster for start and stop commands [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4763:
URL: https://github.com/apache/accumulo/pull/4763#discussion_r1693704434
##########
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:
After removing it I didn't see shellcheck throwing errors about it.
I'm curious if it was previous flagging on `SSERVER_GROUPS` vs `TSERVER_GROUPS` but maybe now there's enough other references that it doesn't trigger.
--
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] Improves accumulo-cluster for start and stop commands [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4763:
URL: https://github.com/apache/accumulo/pull/4763#discussion_r1693661875
##########
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:
Currently the `start-non-tservers` starts all server types except for tservers.
There's a flag that's passed to the start_all command which explicitly excludes tservers so this command will start scan servers.
--
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] Improves accumulo-cluster for start and stop commands [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on PR #4763:
URL: https://github.com/apache/accumulo/pull/4763#issuecomment-2253731405
One nice side-effect of the accumulo-service refactor is that now the specific service process name is returned to the user, not just the service type:
```
./accumulo-cluster start-servers --all
...
Starting sserver_default on localhost
Starting compaction-coordinator on localhost
Starting compactor_q1 on localhost
Starting compactor_q2 on localhost
```
--
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] Improves accumulo-cluster for start and stop commands [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4763:
URL: https://github.com/apache/accumulo/pull/4763#discussion_r1694305012
##########
assemble/bin/accumulo-service:
##########
@@ -137,40 +192,61 @@ function main() {
mkdir -p "$ACCUMULO_LOG_DIR" 2>/dev/null
mkdir -p "$ACCUMULO_PID_DIR" 2>/dev/null
- host="$(hostname)"
- if [[ -z $host ]]; then
- host=$(ip addr | grep 'state UP' -A2 | tail -n1 | awk '{print $2}' | cut -f1 -d'/')
+ HOST="$(hostname)"
+ if [[ -z $HOST ]]; then
+ HOST=$(ip addr | grep 'state UP' -A2 | tail -n1 | awk '{print $2}' | cut -f1 -d'/')
fi
- service="$1"
- if [[ $service == "master" ]]; then
+ local service_type="$1"
+ local command_name="$2"
+ shift 2
+ local service_name="unknown"
+ local all_flag=false
+
+ if [[ $service_type == "master" ]]; then
echo "WARN : Use of 'master' service name is deprecated; use 'manager' instead."
- service="manager"
+ service_type="manager"
+ fi
+
+ # Check and see if accumulo-cluster is calling this script
+ if [ -z ${ACCUMULO_SERVICE_INSTANCE+x} ]; then
+ # The rest of the arguments are from a user
Review Comment:
I was trying to ensure that ACCUMULO_SERVICE_INSTANCE always contained a non-empty string.
However, after your comment I went back and tested setting a single instance of each service type.
When accumulo-cluster starts up a single tserver, ACCUMULO_SERVICE_INSTANCE is set to a empty value.
Then this is passed to accumulo-service.
https://github.com/apache/accumulo/blob/8b879c4e7ae8583e72448d66d073973a508fb716/assemble/bin/accumulo-cluster#L141
I've switched the logic to use your suggestion and tested that it works with single and multiple instances of accumulo services.
--
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] Improves accumulo-cluster for start and stop commands [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4763:
URL: https://github.com/apache/accumulo/pull/4763#discussion_r1694306132
##########
assemble/bin/accumulo-service:
##########
@@ -137,40 +192,61 @@ function main() {
mkdir -p "$ACCUMULO_LOG_DIR" 2>/dev/null
mkdir -p "$ACCUMULO_PID_DIR" 2>/dev/null
- host="$(hostname)"
- if [[ -z $host ]]; then
- host=$(ip addr | grep 'state UP' -A2 | tail -n1 | awk '{print $2}' | cut -f1 -d'/')
+ HOST="$(hostname)"
+ if [[ -z $HOST ]]; then
+ HOST=$(ip addr | grep 'state UP' -A2 | tail -n1 | awk '{print $2}' | cut -f1 -d'/')
fi
- service="$1"
- if [[ $service == "master" ]]; then
+ local service_type="$1"
+ local command_name="$2"
+ shift 2
+ local service_name="unknown"
+ local all_flag=false
+
+ if [[ $service_type == "master" ]]; then
echo "WARN : Use of 'master' service name is deprecated; use 'manager' instead."
- service="manager"
+ service_type="manager"
+ fi
+
+ # Check and see if accumulo-cluster is calling this script
+ if [ -z ${ACCUMULO_SERVICE_INSTANCE+x} ]; then
+ # The rest of the arguments are from a user
+ if [[ $1 == "--all" ]]; then
+ all_flag=true
+ else
+ # A named service has been specified
+ service_name="$1"
+ fi
+ # Use the special bash env var from accumulo-cluster
+ else
+ service_name=${service_type}${ACCUMULO_SERVICE_INSTANCE}
fi
- pid_file="${ACCUMULO_PID_DIR}/accumulo-${service}${ACCUMULO_SERVICE_INSTANCE}.pid"
- case "$service" in
+ case "$service_type" in
gc | manager | monitor | tserver | compaction-coordinator | compactor | sserver)
- if [[ -z $2 ]]; then
+ if [ -z ${command_name+z} ]; then
Review Comment:
suggestion has been applied
--
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] Improves accumulo-cluster for start and stop commands [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4763:
URL: https://github.com/apache/accumulo/pull/4763#discussion_r1693679111
##########
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:
I agree that shift is better so I can look at modifying this code to avoid this.
I need to go back and revisit the logic but iirc I was using the substring Expansion to pull the service name and ignore any additional details only on stop and kill commands.
This was because we need the additional compaction queue or scan server group information to be passed on startup so that `accumulo-env.sh` could support assigning different JAVA_OPTS based on the scan server group or compaction queue vs just a single server type.
However `stop_service` and `kill_service` don't use those extra args for anything and instead attempt to treat them as additional service names which generate a lot of extra log message noise.
--
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] Improves accumulo-cluster for start and stop commands [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4763:
URL: https://github.com/apache/accumulo/pull/4763#discussion_r1693680394
##########
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:
This was part of our speed up during scale testing when running multiple processes per host
I'm happy to pull it out since it was a quick & easy fix and we can revisit it in 2.1.4
--
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] Improves accumulo-cluster for start and stop commands [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4763:
URL: https://github.com/apache/accumulo/pull/4763#discussion_r1693746975
##########
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:
Removed all of this in favor of local variables and passing vars between the bash functions.
##########
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:
Removed all of this in favor of local variables and passing vars between the bash functions.
--
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] Improves accumulo-cluster for start and stop commands [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improves accumulo-cluster for start and stop commands [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4763:
URL: https://github.com/apache/accumulo/pull/4763#discussion_r1694307268
##########
assemble/bin/accumulo-service:
##########
@@ -137,40 +192,61 @@ function main() {
mkdir -p "$ACCUMULO_LOG_DIR" 2>/dev/null
mkdir -p "$ACCUMULO_PID_DIR" 2>/dev/null
- host="$(hostname)"
- if [[ -z $host ]]; then
- host=$(ip addr | grep 'state UP' -A2 | tail -n1 | awk '{print $2}' | cut -f1 -d'/')
+ HOST="$(hostname)"
+ if [[ -z $HOST ]]; then
+ HOST=$(ip addr | grep 'state UP' -A2 | tail -n1 | awk '{print $2}' | cut -f1 -d'/')
fi
- service="$1"
- if [[ $service == "master" ]]; then
+ local service_type="$1"
+ local command_name="$2"
+ shift 2
Review Comment:
Interesting, I think I've always used the numeric option.
--
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] Improves accumulo-cluster for start and stop commands [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr merged PR #4763:
URL: https://github.com/apache/accumulo/pull/4763
--
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] Improves accumulo-cluster for start and stop commands [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4763:
URL: https://github.com/apache/accumulo/pull/4763#discussion_r1693702822
##########
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 can pull it just so we don't have a buggy version of 2.1.3.
It's an easy enough modification to make at a later date.
--
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] Improves accumulo-cluster for start and stop commands [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4763:
URL: https://github.com/apache/accumulo/pull/4763#discussion_r1693747434
##########
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:
OBE
--
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] Improves accumulo-cluster for start and stop commands [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4763:
URL: https://github.com/apache/accumulo/pull/4763#discussion_r1693893996
##########
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:
created #4770 as a follow-on
--
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