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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
