[GitHub] [accumulo] ctubbsii commented on a change in pull request #1568: Support multiple tservers / node in accumulo-service
ctubbsii commented on a change in pull request #1568: Support multiple tservers
/ node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#discussion_r398162385
##
File path: assemble/bin/accumulo-cluster
##
@@ -100,15 +100,45 @@ function get_ip() {
echo "$ip_addr"
}
+function set_last_instance_id() {
+ if [[ "$service" == "tserver" ]]; then
+last_instance_id=${NUM_TSERVERS:-1}
+ else
+last_instance_id=1
+ fi
+}
+
+function set_service_instance_if_needed() {
+ if [[ "$service" == "tserver" && ${NUM_TSERVERS:-1} > 1 ]]; then
+export ACCUMULO_SERVICE_INSTANCE=$inst_id
+service_instance_cmd="export ACCUMULO_SERVICE_INSTANCE=${inst_id};"
+ else
+export ACCUMULO_SERVICE_INSTANCE=
+service_instance_cmd=
+ fi
+}
+
+function control_service() {
+ set_last_instance_id
+
+ for (( inst_id=1; inst_id<=last_instance_id; inst_id++ ))
+ do
+set_service_instance_if_needed
+
+if [[ $host == localhost || $host = "$(hostname -s)" || $host =
"$(hostname -f)" || $host = $(get_ip) ]] ; then
+ "${bin}/accumulo-service" "$service" "$control_cmd"
+else
+ $SSH "$host" "bash -c '${service_instance_cmd} ${bin}/accumulo-service
\"$service\" \"$control_cmd\"'"
Review comment:
The docs will further need to be updated to mention the use of
`NUM_TSERVERS` for 2.1 and later, once this PR gets merged.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1568: Support multiple tservers / node in accumulo-service
ctubbsii commented on a change in pull request #1568: Support multiple tservers
/ node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#discussion_r398160867
##
File path: assemble/bin/accumulo-cluster
##
@@ -100,15 +100,45 @@ function get_ip() {
echo "$ip_addr"
}
+function set_last_instance_id() {
+ if [[ "$service" == "tserver" ]]; then
+last_instance_id=${NUM_TSERVERS:-1}
+ else
+last_instance_id=1
+ fi
+}
+
+function set_service_instance_if_needed() {
+ if [[ "$service" == "tserver" && ${NUM_TSERVERS:-1} > 1 ]]; then
+export ACCUMULO_SERVICE_INSTANCE=$inst_id
+service_instance_cmd="export ACCUMULO_SERVICE_INSTANCE=${inst_id};"
+ else
+export ACCUMULO_SERVICE_INSTANCE=
+service_instance_cmd=
+ fi
+}
+
+function control_service() {
+ set_last_instance_id
+
+ for (( inst_id=1; inst_id<=last_instance_id; inst_id++ ))
+ do
+set_service_instance_if_needed
+
+if [[ $host == localhost || $host = "$(hostname -s)" || $host =
"$(hostname -f)" || $host = $(get_ip) ]] ; then
+ "${bin}/accumulo-service" "$service" "$control_cmd"
+else
+ $SSH "$host" "bash -c '${service_instance_cmd} ${bin}/accumulo-service
\"$service\" \"$control_cmd\"'"
Review comment:
Ah, yeah, that's definitely a typo in the docs. I'll fix that.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1568: Support multiple tservers / node in accumulo-service
ctubbsii commented on a change in pull request #1568: Support multiple tservers
/ node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#discussion_r398121451
##
File path: assemble/bin/accumulo-cluster
##
@@ -100,15 +100,45 @@ function get_ip() {
echo "$ip_addr"
}
+function set_last_instance_id() {
+ if [[ "$service" == "tserver" ]]; then
+last_instance_id=${NUM_TSERVERS:-1}
+ else
+last_instance_id=1
+ fi
+}
+
+function set_service_instance_if_needed() {
+ if [[ "$service" == "tserver" && ${NUM_TSERVERS:-1} > 1 ]]; then
+export ACCUMULO_SERVICE_INSTANCE=$inst_id
+service_instance_cmd="export ACCUMULO_SERVICE_INSTANCE=${inst_id};"
+ else
+export ACCUMULO_SERVICE_INSTANCE=
+service_instance_cmd=
+ fi
+}
+
+function control_service() {
+ set_last_instance_id
+
+ for (( inst_id=1; inst_id<=last_instance_id; inst_id++ ))
+ do
+set_service_instance_if_needed
+
+if [[ $host == localhost || $host = "$(hostname -s)" || $host =
"$(hostname -f)" || $host = $(get_ip) ]] ; then
Review comment:
I didn't notice it was existing code. It's fine if you want to leave it.
It's entirely up to you.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1568: Support multiple tservers / node in accumulo-service
ctubbsii commented on a change in pull request #1568: Support multiple tservers
/ node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#discussion_r398120830
##
File path: assemble/bin/accumulo-cluster
##
@@ -100,15 +100,45 @@ function get_ip() {
echo "$ip_addr"
}
+function set_last_instance_id() {
+ if [[ "$service" == "tserver" ]]; then
+last_instance_id=${NUM_TSERVERS:-1}
+ else
+last_instance_id=1
+ fi
+}
+
+function set_service_instance_if_needed() {
+ if [[ "$service" == "tserver" && ${NUM_TSERVERS:-1} > 1 ]]; then
Review comment:
I saw you already fixed the "unnecesarry $ in math expression" issue.
shellcheck is awesome, isn't it? :smiley_cat:
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1568: Support multiple tservers / node in accumulo-service
ctubbsii commented on a change in pull request #1568: Support multiple tservers
/ node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#discussion_r398071195
##
File path: assemble/bin/accumulo-cluster
##
@@ -100,15 +100,45 @@ function get_ip() {
echo "$ip_addr"
}
+function set_last_instance_id() {
+ if [[ "$service" == "tserver" ]]; then
+last_instance_id=${NUM_TSERVERS:-1}
+ else
+last_instance_id=1
+ fi
+}
+
+function set_service_instance_if_needed() {
+ if [[ "$service" == "tserver" && ${NUM_TSERVERS:-1} > 1 ]]; then
Review comment:
Should use numeric greater-than comparison here, instead of ASCII lexical
comparison:
```suggestion
if [[ "$service" == "tserver" && ${NUM_TSERVERS:-1} -gt 1 ]]; then
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1568: Support multiple tservers / node in accumulo-service
ctubbsii commented on a change in pull request #1568: Support multiple tservers
/ node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#discussion_r398069835
##
File path: assemble/bin/accumulo-cluster
##
@@ -100,15 +100,45 @@ function get_ip() {
echo "$ip_addr"
}
+function set_last_instance_id() {
+ if [[ "$service" == "tserver" ]]; then
+last_instance_id=${NUM_TSERVERS:-1}
+ else
+last_instance_id=1
+ fi
+}
+
+function set_service_instance_if_needed() {
+ if [[ "$service" == "tserver" && ${NUM_TSERVERS:-1} > 1 ]]; then
+export ACCUMULO_SERVICE_INSTANCE=$inst_id
+service_instance_cmd="export ACCUMULO_SERVICE_INSTANCE=${inst_id};"
+ else
+export ACCUMULO_SERVICE_INSTANCE=
+service_instance_cmd=
+ fi
+}
+
+function control_service() {
+ set_last_instance_id
Review comment:
Could use a simple local variable here.
```suggestion
local last_instance_id; last_instance_id=1
[[ "$service" = "tserver" ]] && last_instance_id=${NUM_TSERVERS:-1}
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1568: Support multiple tservers / node in accumulo-service
ctubbsii commented on a change in pull request #1568: Support multiple tservers
/ node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#discussion_r398070573
##
File path: assemble/bin/accumulo-cluster
##
@@ -100,15 +100,45 @@ function get_ip() {
echo "$ip_addr"
}
+function set_last_instance_id() {
+ if [[ "$service" == "tserver" ]]; then
+last_instance_id=${NUM_TSERVERS:-1}
+ else
+last_instance_id=1
+ fi
+}
+
Review comment:
This can be inline'd. It's only used once, and doesn't do much.
```suggestion
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1568: Support multiple tservers / node in accumulo-service
ctubbsii commented on a change in pull request #1568: Support multiple tservers
/ node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#discussion_r398074861
##
File path: assemble/bin/accumulo-cluster
##
@@ -100,15 +100,45 @@ function get_ip() {
echo "$ip_addr"
}
+function set_last_instance_id() {
+ if [[ "$service" == "tserver" ]]; then
+last_instance_id=${NUM_TSERVERS:-1}
+ else
+last_instance_id=1
+ fi
+}
+
+function set_service_instance_if_needed() {
+ if [[ "$service" == "tserver" && ${NUM_TSERVERS:-1} > 1 ]]; then
+export ACCUMULO_SERVICE_INSTANCE=$inst_id
+service_instance_cmd="export ACCUMULO_SERVICE_INSTANCE=${inst_id};"
+ else
+export ACCUMULO_SERVICE_INSTANCE=
+service_instance_cmd=
+ fi
+}
+
+function control_service() {
+ set_last_instance_id
+
+ for (( inst_id=1; inst_id<=last_instance_id; inst_id++ ))
+ do
+set_service_instance_if_needed
+
+if [[ $host == localhost || $host = "$(hostname -s)" || $host =
"$(hostname -f)" || $host = $(get_ip) ]] ; then
+ "${bin}/accumulo-service" "$service" "$control_cmd"
+else
+ $SSH "$host" "bash -c '${service_instance_cmd} ${bin}/accumulo-service
\"$service\" \"$control_cmd\"'"
+fi
+ done
+}
+
function start_service() {
host="$1"
service="$2"
+ control_cmd="start"
- if [[ $host == "localhost" || $host == $(hostname -f) || $host == $(hostname
-s) || $host == $(get_ip) ]]; then
-"${bin}/accumulo-service" "$service" start
- else
-$SSH "$host" "bash -c '${bin}/accumulo-service \"$service\" start'"
- fi
+ control_service
Review comment:
Could pass the arguments to the function instead of setting up global
variables and picking them up in the function.
```suggestion
control_service start "$@"
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1568: Support multiple tservers / node in accumulo-service
ctubbsii commented on a change in pull request #1568: Support multiple tservers
/ node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#discussion_r398058190
##
File path: assemble/bin/accumulo-cluster
##
@@ -100,15 +100,45 @@ function get_ip() {
echo "$ip_addr"
}
+function set_last_instance_id() {
+ if [[ "$service" == "tserver" ]]; then
+last_instance_id=${NUM_TSERVERS:-1}
+ else
+last_instance_id=1
+ fi
+}
+
+function set_service_instance_if_needed() {
+ if [[ "$service" == "tserver" && ${NUM_TSERVERS:-1} > 1 ]]; then
+export ACCUMULO_SERVICE_INSTANCE=$inst_id
+service_instance_cmd="export ACCUMULO_SERVICE_INSTANCE=${inst_id};"
+ else
+export ACCUMULO_SERVICE_INSTANCE=
+service_instance_cmd=
+ fi
+}
+
+function control_service() {
+ set_last_instance_id
+
+ for (( inst_id=1; inst_id<=last_instance_id; inst_id++ ))
+ do
+set_service_instance_if_needed
+
+if [[ $host == localhost || $host = "$(hostname -s)" || $host =
"$(hostname -f)" || $host = $(get_ip) ]] ; then
Review comment:
This mixes double equals and single equals. Both are equivalent in bash
double-square braces, so it's fine either way, but it'd be nice to be
consistent.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1568: Support multiple tservers / node in accumulo-service
ctubbsii commented on a change in pull request #1568: Support multiple tservers
/ node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#discussion_r398073126
##
File path: assemble/bin/accumulo-cluster
##
@@ -100,15 +100,45 @@ function get_ip() {
echo "$ip_addr"
}
+function set_last_instance_id() {
+ if [[ "$service" == "tserver" ]]; then
+last_instance_id=${NUM_TSERVERS:-1}
+ else
+last_instance_id=1
+ fi
+}
+
+function set_service_instance_if_needed() {
+ if [[ "$service" == "tserver" && ${NUM_TSERVERS:-1} > 1 ]]; then
+export ACCUMULO_SERVICE_INSTANCE=$inst_id
+service_instance_cmd="export ACCUMULO_SERVICE_INSTANCE=${inst_id};"
+ else
+export ACCUMULO_SERVICE_INSTANCE=
+service_instance_cmd=
+ fi
+}
+
+function control_service() {
+ set_last_instance_id
+
+ for (( inst_id=1; inst_id<=last_instance_id; inst_id++ ))
+ do
+set_service_instance_if_needed
+
+if [[ $host == localhost || $host = "$(hostname -s)" || $host =
"$(hostname -f)" || $host = $(get_ip) ]] ; then
+ "${bin}/accumulo-service" "$service" "$control_cmd"
+else
+ $SSH "$host" "bash -c '${service_instance_cmd} ${bin}/accumulo-service
\"$service\" \"$control_cmd\"'"
Review comment:
Instead of creating a second variable and exporting here, you can probably
just execute it with the environment variable set for the execution:
```suggestion
$SSH "$host" "bash -c
'ACCUMULO_SERVICE_INSTANCE='"$ACCUMULO_SERVICE_INSTANCE"'
${bin}/accumulo-service \"$service\" \"$control_cmd\"'"
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
