You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/01/06 21:46:56 UTC
[GitHub] [accumulo-testing] ctubbsii commented on a change in pull request #184: Fix broken agitator scripts
ctubbsii commented on a change in pull request #184:
URL: https://github.com/apache/accumulo-testing/pull/184#discussion_r779881163
##########
File path: bin/agitator
##########
@@ -30,6 +30,144 @@ Possible commands:
EOF
}
+function run_agitators() {
+ unset APP_HOSTS
+ export APP_HOSTS=$MANAGER_HOSTS
+ echo "Starting manager agitation as ${1} for ${APP_HOSTS}"
+ start_app_agitator "manager" "${AGTR_MASTER_KILL_SLEEP_TIME}" "${AGTR_MASTER_RESTART_SLEEP_TIME}" 1 1 > "${manager_log}" 2>&1 &
+ unset APP_HOSTS
+ export APP_HOSTS=$TSERVER_HOSTS
+ echo "Starting tserver agitation as ${1} for ${APP_HOSTS}"
+ start_app_agitator "tserver" "${AGTR_TSERVER_KILL_SLEEP_TIME}" "${AGTR_TSERVER_RESTART_SLEEP_TIME}" "${AGTR_TSERVER_MIN_KILL}" "${AGTR_TSERVER_MAX_KILL}" > "${tserver_log}" 2>&1 &
+}
+
+# Starts a app specific agitator
+# usage: start_app_agitator app_name kill_sleep_time restart_sleep_time min_kill max_kill
+# Requires that a list of hosts to be set in $APP_HOSTS
+function start_app_agitator() {
+ app_name=$1
+ kill_sleep_time=$2
+ restart_sleep_time=$3
+ min_kill=$4
+ max_kill=$5
+ ENV_VARS="ACCUMULO_HOME=${ACCUMULO_HOME} ZOOKEEPER_HOME=${ZOOKEEPER_HOME} HADOOP_HOME=${HADOOP_HOME} JAVA_HOME=${JAVA_HOME}"
+ if [[ -z $APP_HOSTS ]]; then
+ echo "ERROR: No hosts were found in env for ${app_name}"
+ exit 1
+ fi
Review comment:
Based on my other suggestions, this could just check the size of the array
##########
File path: bin/agitator
##########
@@ -30,6 +30,144 @@ Possible commands:
EOF
}
+function run_agitators() {
+ unset APP_HOSTS
+ export APP_HOSTS=$MANAGER_HOSTS
+ echo "Starting manager agitation as ${1} for ${APP_HOSTS}"
+ start_app_agitator "manager" "${AGTR_MASTER_KILL_SLEEP_TIME}" "${AGTR_MASTER_RESTART_SLEEP_TIME}" 1 1 > "${manager_log}" 2>&1 &
+ unset APP_HOSTS
+ export APP_HOSTS=$TSERVER_HOSTS
+ echo "Starting tserver agitation as ${1} for ${APP_HOSTS}"
+ start_app_agitator "tserver" "${AGTR_TSERVER_KILL_SLEEP_TIME}" "${AGTR_TSERVER_RESTART_SLEEP_TIME}" "${AGTR_TSERVER_MIN_KILL}" "${AGTR_TSERVER_MAX_KILL}" > "${tserver_log}" 2>&1 &
+}
+
+# Starts a app specific agitator
+# usage: start_app_agitator app_name kill_sleep_time restart_sleep_time min_kill max_kill
+# Requires that a list of hosts to be set in $APP_HOSTS
+function start_app_agitator() {
+ app_name=$1
+ kill_sleep_time=$2
+ restart_sleep_time=$3
+ min_kill=$4
+ max_kill=$5
+ ENV_VARS="ACCUMULO_HOME=${ACCUMULO_HOME} ZOOKEEPER_HOME=${ZOOKEEPER_HOME} HADOOP_HOME=${HADOOP_HOME} JAVA_HOME=${JAVA_HOME}"
+ if [[ -z $APP_HOSTS ]]; then
+ echo "ERROR: No hosts were found in env for ${app_name}"
+ exit 1
+ fi
+ T="$(date +'%Y%m%d %H:%M:%S')"
+ echo "${T} Running ${app_name} agitator ${kill_sleep_time} ${restart_sleep_time} ${min_kill} ${max_kill} for HOSTS=${APP_HOSTS}"
+ while true; do
+ echo "${T} Sleeping for ${kill_sleep_time} minutes"
+ sleep $((kill_sleep_time * 60))
+
+ T="$(date +'%Y%m%d %H:%M:%S')"
+ numToWack=$((min_kill + RANDOM % max_kill))
+ nodeToWack=$(echo "${APP_HOSTS}" | cut -d" " -f1)
+ hostsArray=("${APP_HOSTS}")
+ numOfHosts=${#hostsArray[@]}
+ declare -a nodesToWack
+
+ if ((max_kill == 1)) ; then
+ echo "${T} Killing ${app_name}"
+ ssh "${nodeToWack}" "pkill -f '[ ]org.apache.accumulo.start.*${app_name}'"
+ else
+ # get the random nodes to kill
+ count=0
+ while [ $count -lt $numToWack ]; do
Review comment:
Should use `[[ ]]` instead of `[ ]` to avoid any possible performance hit by calling `/usr/bin/[`, or any possible confusion because it looks like that's what it's doing. `[[ ]]` is built-in, but `[ ]` is a standalone executable that bash may or may not execute or replace with its own built-in interpretation (just like /usr/bin/echo).
##########
File path: bin/agitator
##########
@@ -30,6 +30,144 @@ Possible commands:
EOF
}
+function run_agitators() {
+ unset APP_HOSTS
+ export APP_HOSTS=$MANAGER_HOSTS
+ echo "Starting manager agitation as ${1} for ${APP_HOSTS}"
+ start_app_agitator "manager" "${AGTR_MASTER_KILL_SLEEP_TIME}" "${AGTR_MASTER_RESTART_SLEEP_TIME}" 1 1 > "${manager_log}" 2>&1 &
+ unset APP_HOSTS
+ export APP_HOSTS=$TSERVER_HOSTS
+ echo "Starting tserver agitation as ${1} for ${APP_HOSTS}"
+ start_app_agitator "tserver" "${AGTR_TSERVER_KILL_SLEEP_TIME}" "${AGTR_TSERVER_RESTART_SLEEP_TIME}" "${AGTR_TSERVER_MIN_KILL}" "${AGTR_TSERVER_MAX_KILL}" > "${tserver_log}" 2>&1 &
Review comment:
These first three lines are duplicated with different parameters for the managers and the tservers. These can be passed as parameters to the `start_app_agitator` function instead.
Also, if at all possible, you should avoid passing around variables in and out of functions using global variables. This is especially important since you're backgrounding the calls and then immediately editing the value of the global variable, which could create concurrency issues. You can declare variables local to a function with the `local` keyword, as in `local x; x=$1` to assign the first argument to the function to the local variable named `x`.
Also, you don't need all the curly braces for scalar variables that aren't ambiguous in a string. They can add noise that affect readability.
```suggestion
local agtr_user=$1
start_app_agitator manager "$agtr_user" "$AGTR_MASTER_KILL_SLEEP_TIME" "$AGTR_MASTER_RESTART_SLEEP_TIME" 1 1 $MANAGER_HOSTS > "$manager_log" 2>&1 &
start_app_agitator tserver "$agtr_user" "$AGTR_TSERVER_KILL_SLEEP_TIME" "$AGTR_TSERVER_RESTART_SLEEP_TIME" "$AGTR_TSERVER_MIN_KILL" "$AGTR_TSERVER_MAX_KILL" $TSERVER_HOSTS > "$tserver_log" 2>&1 &
```
##########
File path: bin/agitator
##########
@@ -30,6 +30,144 @@ Possible commands:
EOF
}
+function run_agitators() {
+ unset APP_HOSTS
+ export APP_HOSTS=$MANAGER_HOSTS
+ echo "Starting manager agitation as ${1} for ${APP_HOSTS}"
+ start_app_agitator "manager" "${AGTR_MASTER_KILL_SLEEP_TIME}" "${AGTR_MASTER_RESTART_SLEEP_TIME}" 1 1 > "${manager_log}" 2>&1 &
+ unset APP_HOSTS
+ export APP_HOSTS=$TSERVER_HOSTS
+ echo "Starting tserver agitation as ${1} for ${APP_HOSTS}"
+ start_app_agitator "tserver" "${AGTR_TSERVER_KILL_SLEEP_TIME}" "${AGTR_TSERVER_RESTART_SLEEP_TIME}" "${AGTR_TSERVER_MIN_KILL}" "${AGTR_TSERVER_MAX_KILL}" > "${tserver_log}" 2>&1 &
+}
+
+# Starts a app specific agitator
+# usage: start_app_agitator app_name kill_sleep_time restart_sleep_time min_kill max_kill
+# Requires that a list of hosts to be set in $APP_HOSTS
+function start_app_agitator() {
+ app_name=$1
+ kill_sleep_time=$2
+ restart_sleep_time=$3
+ min_kill=$4
+ max_kill=$5
+ ENV_VARS="ACCUMULO_HOME=${ACCUMULO_HOME} ZOOKEEPER_HOME=${ZOOKEEPER_HOME} HADOOP_HOME=${HADOOP_HOME} JAVA_HOME=${JAVA_HOME}"
+ if [[ -z $APP_HOSTS ]]; then
+ echo "ERROR: No hosts were found in env for ${app_name}"
+ exit 1
+ fi
+ T="$(date +'%Y%m%d %H:%M:%S')"
+ echo "${T} Running ${app_name} agitator ${kill_sleep_time} ${restart_sleep_time} ${min_kill} ${max_kill} for HOSTS=${APP_HOSTS}"
+ while true; do
+ echo "${T} Sleeping for ${kill_sleep_time} minutes"
+ sleep $((kill_sleep_time * 60))
+
+ T="$(date +'%Y%m%d %H:%M:%S')"
+ numToWack=$((min_kill + RANDOM % max_kill))
+ nodeToWack=$(echo "${APP_HOSTS}" | cut -d" " -f1)
+ hostsArray=("${APP_HOSTS}")
Review comment:
don't forget to use local variables for anything you don't think should be set outside this function; if assigning a simple value to another local variable, you can do `local x=$y`. For arrays, you can do `local x=("${y[@]}")`. For assigning the result of an operation, you have to declare as local and assign it in two steps: `local x; x=$(echo y)`
My previous suggestion already set APP_HOSTS to an array from the function's input arguments, so that could simplify some of this array logic in here.
##########
File path: bin/agitator
##########
@@ -30,6 +30,144 @@ Possible commands:
EOF
}
+function run_agitators() {
+ unset APP_HOSTS
+ export APP_HOSTS=$MANAGER_HOSTS
+ echo "Starting manager agitation as ${1} for ${APP_HOSTS}"
+ start_app_agitator "manager" "${AGTR_MASTER_KILL_SLEEP_TIME}" "${AGTR_MASTER_RESTART_SLEEP_TIME}" 1 1 > "${manager_log}" 2>&1 &
+ unset APP_HOSTS
+ export APP_HOSTS=$TSERVER_HOSTS
+ echo "Starting tserver agitation as ${1} for ${APP_HOSTS}"
+ start_app_agitator "tserver" "${AGTR_TSERVER_KILL_SLEEP_TIME}" "${AGTR_TSERVER_RESTART_SLEEP_TIME}" "${AGTR_TSERVER_MIN_KILL}" "${AGTR_TSERVER_MAX_KILL}" > "${tserver_log}" 2>&1 &
+}
+
+# Starts a app specific agitator
+# usage: start_app_agitator app_name kill_sleep_time restart_sleep_time min_kill max_kill
+# Requires that a list of hosts to be set in $APP_HOSTS
+function start_app_agitator() {
+ app_name=$1
+ kill_sleep_time=$2
+ restart_sleep_time=$3
+ min_kill=$4
+ max_kill=$5
Review comment:
```suggestion
# Requires that a list of hosts to be set in $APP_HOSTS
function start_app_agitator() {
local app_name=$1; shift
local agtr_user=$1; shift
local kill_sleep_time=$1; shift
local restart_sleep_time=$1; shift
local min_kill=$1; shift
local max_kill=$1; shift
local APP_HOSTS=("$@") # can keep this as an array
echo "Starting $app_name agitation as $agtr_user for ${APP_HOSTS[*]}"
```
##########
File path: bin/agitator
##########
@@ -30,6 +30,144 @@ Possible commands:
EOF
}
+function run_agitators() {
+ unset APP_HOSTS
+ export APP_HOSTS=$MANAGER_HOSTS
+ echo "Starting manager agitation as ${1} for ${APP_HOSTS}"
+ start_app_agitator "manager" "${AGTR_MASTER_KILL_SLEEP_TIME}" "${AGTR_MASTER_RESTART_SLEEP_TIME}" 1 1 > "${manager_log}" 2>&1 &
+ unset APP_HOSTS
+ export APP_HOSTS=$TSERVER_HOSTS
+ echo "Starting tserver agitation as ${1} for ${APP_HOSTS}"
+ start_app_agitator "tserver" "${AGTR_TSERVER_KILL_SLEEP_TIME}" "${AGTR_TSERVER_RESTART_SLEEP_TIME}" "${AGTR_TSERVER_MIN_KILL}" "${AGTR_TSERVER_MAX_KILL}" > "${tserver_log}" 2>&1 &
+}
+
+# Starts a app specific agitator
+# usage: start_app_agitator app_name kill_sleep_time restart_sleep_time min_kill max_kill
+# Requires that a list of hosts to be set in $APP_HOSTS
+function start_app_agitator() {
+ app_name=$1
+ kill_sleep_time=$2
+ restart_sleep_time=$3
+ min_kill=$4
+ max_kill=$5
+ ENV_VARS="ACCUMULO_HOME=${ACCUMULO_HOME} ZOOKEEPER_HOME=${ZOOKEEPER_HOME} HADOOP_HOME=${HADOOP_HOME} JAVA_HOME=${JAVA_HOME}"
+ if [[ -z $APP_HOSTS ]]; then
+ echo "ERROR: No hosts were found in env for ${app_name}"
+ exit 1
+ fi
+ T="$(date +'%Y%m%d %H:%M:%S')"
+ echo "${T} Running ${app_name} agitator ${kill_sleep_time} ${restart_sleep_time} ${min_kill} ${max_kill} for HOSTS=${APP_HOSTS}"
+ while true; do
+ echo "${T} Sleeping for ${kill_sleep_time} minutes"
+ sleep $((kill_sleep_time * 60))
+
+ T="$(date +'%Y%m%d %H:%M:%S')"
+ numToWack=$((min_kill + RANDOM % max_kill))
+ nodeToWack=$(echo "${APP_HOSTS}" | cut -d" " -f1)
+ hostsArray=("${APP_HOSTS}")
+ numOfHosts=${#hostsArray[@]}
+ declare -a nodesToWack
+
+ if ((max_kill == 1)) ; then
+ echo "${T} Killing ${app_name}"
+ ssh "${nodeToWack}" "pkill -f '[ ]org.apache.accumulo.start.*${app_name}'"
+ else
+ # get the random nodes to kill
+ count=0
+ while [ $count -lt $numToWack ]; do
+ randomHostIndex=$((1 + RANDOM % numOfHosts))
+ nodeToWack=${hostsArray[randomHostIndex]}
+ # only add host to the array if its not already there
+ if [[ ! " ${nodesToWack[*]} " =~ ${nodeToWack} ]]; then
+ nodesToWack[count]=${nodeToWack}
+ fi
+ count=${#nodesToWack[@]}
+ done
+ echo "${T} Killing ${count} ${app_name} nodes"
+ for i in "${nodesToWack[@]}"; do
+ ssh "${i}" "pkill -f '[ ]org.apache.accumulo.start.*${app_name}'"
Review comment:
Could make this pattern be a little more specific (might not be correct):
```suggestion
ssh "${i}" "pkill -f '[ ]org[.]apache[.]accumulo[.]start[.]Main ${app_name}'"
```
--
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