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 2020/03/21 02:21:04 UTC
[GitHub] [accumulo] arvindshmicrosoft opened a new pull request #1568:
Support multiple tservers / node in accumulo-service
arvindshmicrosoft opened a new pull request #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568
If NUM_TSERVERS is defined (typically by adding it in accumulo-env.sh
this modification to accumulo-service enables trivial support for
running more than 1 tserver per node. Of course, the corresponding
changes in accumulo.properties (tserver.port.search and
replication.receipt.service.port) are still 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] arvindshmicrosoft commented on issue #1568: Support
multiple tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
arvindshmicrosoft commented on issue #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#issuecomment-601999204
Acknowledged, thank you Christopher for your candid feedback. I'll think through the alternatives. Also appreciate the tip about the commit message, that mistake will not be repeated.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] keith-turner commented on issue #1568: Support multiple
tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
keith-turner commented on issue #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#issuecomment-602084624
> The purpose of the bin/accumulo-service script is to provide a mechanism to manage a single process, in order to provide a simple mechanism to build other tools around.
> Furthermore, this script isn't really intended to be used by anything other than the accumulo-cluster script.
@ctubbsii the above statements seem to contradict each other. I think these changes are very useful. Your objection feels a bit arbitrary and subjective. However I trust your intuition and historical knowledge on this matter. I was trying to think of ways to make this less subjective. One thought I had was maybe you could document the purpose/design principals/intentions as comments in the script itself. These would serve as a guide for these changes and future changes.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on issue #1568: Support multiple
tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#issuecomment-603600158
> @ctubbsii I have redone this PR (hence the force push). Hopefully this version is more aligned with the guiding principles for the 2 scripts.
Thanks. I'll take a look as soon as I can.
> One thing I'd like advice on (in addition to any other comments) is: given that accumulo-cluster currently does not source accumulo-env.sh, the user must export NUM_TSERVERS prior to invoking accumulo-cluster. This is probably fine for dev/test but I was hoping to define the variable somewhere on the node itself. Any thoughts on sourcing accumulo-env.sh in accumulo-cluster itself?
I think it's okay to require exporting it for now. As follow-on work, I think it might be best if the `accumulo-cluster` script had its own config file (for similar reasons as why Apache Fluo Muchos has its own, but much more lightweight than that, of course). A config file could replace all the separate node list files, and offer additional features, like number of tservers, etc. I don't think sourcing `accumulo-env.sh` is best, only because I think it's important to keep the dividing between the cluster management/service scripts and the basic `bin/accumulo` script.
(FWIW, I realize `accumulo-service` also sources `accumulo-env.sh`, which sort of breaks the dividing line a bit... I'm not a fan of this, but I understand why it needs to do it right now. I don't have an alternative to suggest right now, though. I think there are better alternatives for `accumulo-cluster`, though.)
>
> I also observed in single-node testing (with Uno) with 2 TServers is that the user has to set gc.port.client to a number such as 10002 to avoid having GC pick port 9998 - which GC cannot - since 9998 has already been used by the second TServer running. So the second question I have is why do these scripts start TServers first, ahead of master, monitor and GC?
Mainly, to give the tservers a chance to be ready, before master begins assigning tablets, so the system can start up with tablets balanced, insteady of thrashing on re-balance as new tservers start up and report in. monitor and gc also start creating work as soon as they start, work that depends on the master and tservers to be up (to report status, or to scan the metadata table).
I'd have to investigate the issue of setting the conflicting ports... that might be an Uno-specific issue, since Uno does set up its own configuration. I'm not sure.
>
> I will try joining the Slack chat on Wednesday to discuss further if needed. Thank you and @keith-turner for working through this with me.
Cool. If you can't make it, let me know what time window might be better for you, and I can try to accommodate on a second video chat.
----------------------------------------------------------------
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on issue #1568: Support multiple
tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#issuecomment-605311214
I tested this by setting `NUM_TSERVERS=3` in my Uno config file. I ran into issue #1573, but worked around it with:
```diff
diff --git a/conf/accumulo/2/accumulo.properties b/conf/accumulo/2/accumulo.properties
index d0251bf..a2ddcb7 100755
--- a/conf/accumulo/2/accumulo.properties
+++ b/conf/accumulo/2/accumulo.properties
@@ -6,3 +6,6 @@ instance.zookeeper.host=UNO_HOST:2181
table.durability=flush
tserver.memory.maps.native.enabled=ACCUMULO_USE_NATIVE_MAP
tserver.walog.max.size=512M
+tserver.port.search=true
+master.port.client=9996
+gc.port.client=9994
```
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] arvindshmicrosoft commented on a change in pull request
#1568: Support multiple tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
arvindshmicrosoft commented on a change in pull request #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#discussion_r398113140
##########
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=
Review comment:
Will consider and implement. Thanks!
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on issue #1568: Support multiple
tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#issuecomment-602158205
> @ctubbsii the above statements seem to contradict each other.
You're right. Sorry. My multiple revisions, in an attempt to hone the message, got mangled.
Rather than repeat that mistake (I'm already several hours of revisions into this response... I wish I were kidding), I offer this: I have some time this coming week. Let's set up a video chat to discuss this or any other topics of interest to Accumulo devs. Maybe the points will be easier to write down after talking them through? If it works well, maybe we can set up a regular thing. We can coordinate that on the dev list.
> One thought I had was maybe you could document the purpose/design principals/intentions as comments in the `accumulo-cluster` and `accumulo-service` scripts. These would serve as a guide for these changes and future changes.
Good idea. I can try.
In the meantime, I think the main relevant points for this PR are:
* `accumulo-service` is analogous to SysVinit scripts
* it should only manage a single service/process at a time
* however, it can be called multiple times with different environment
* `accumulo-cluster` manages multiple nodes
* it can be modified to call `accumulo-service` multiple times with different environment
----------------------------------------------------------------
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] arvindshmicrosoft commented on a change in pull request
#1568: Support multiple tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
arvindshmicrosoft commented on a change in pull request #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#discussion_r398113948
##########
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:
Thanks. I had run shellcheck initially but forgot to run it before pushing this round of updates. Indeed that caught this issue as well. I'll make sure I fix it.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on issue #1568: Support multiple
tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#issuecomment-602006769
> Acknowledged, thank you Christopher for your candid feedback. I'll think through the alternatives.
No problem. Sorry if I seem overly critical. I don't intend to be harsh.
Honestly, I think you could probably adapt what you've already done into the `accumulo-cluster` script pretty easily instead, and just export the environment variable before each call to the `accumulo-service` script in there. The `accumulo-service` script itself would need only minor updates to leverage the environment variable for its own output file names.
> Also appreciate the tip about the commit message, that mistake will not be repeated.
No worries. (FWIW, one could legitimately argue that I only routinely provide that advice to keep the record for longest/worst one-line git log message in 8851c3578e5b12281a47fdb0b0f7c43516a544d6 :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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] keith-turner edited a comment on issue #1568: Support
multiple tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
keith-turner edited a comment on issue #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#issuecomment-602084624
> The purpose of the bin/accumulo-service script is to provide a mechanism to manage a single process, in order to provide a simple mechanism to build other tools around.
> Furthermore, this script isn't really intended to be used by anything other than the accumulo-cluster script.
@ctubbsii the above statements seem to contradict each other. I think these changes are very useful. Your objection feels a bit arbitrary and subjective. However I trust your intuition and historical knowledge on this matter. I was trying to think of ways to make this less subjective. One thought I had was maybe you could document the purpose/design principals/intentions as comments in the `accumulo-cluster` and `accumulo-service` scripts. These would serve as a guide for these changes and future changes.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] keith-turner commented on a change in pull request
#1568: Support multiple tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#discussion_r397923832
##########
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=
Review comment:
Not sure the following will work, the thought is to make running on localhost and remote consistent. Both ensuring the env var is not set when running the script.
```suggestion
service_instance_cmd="export ACCUMULO_SERVICE_INSTANCE=;"
```
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii edited a comment on issue #1568: Support
multiple tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
ctubbsii edited a comment on issue #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#issuecomment-604908642
Thanks @arvindshmicrosoft ! I really like how this change worked out. This is a nice feature to have in our out-of-box cluster management script.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] arvindshmicrosoft commented on issue #1568: Support
multiple tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
arvindshmicrosoft commented on issue #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#issuecomment-603590280
@ctubbsii I have redone this PR (hence the force push). Hopefully this version is more aligned with the guiding principles for the 2 scripts. One thing I'd like advice on (in addition to any other comments) is: given that accumulo-cluster currently does not source accumulo-env.sh, the user must export NUM_TSERVERS prior to invoking accumulo-cluster. This is probably fine for dev/test but I was hoping to define the variable somewhere on the node itself. Any thoughts on sourcing accumulo-env.sh in accumulo-cluster itself?
I also observed in single-node testing (with Uno) with 2 TServers is that the user has to set gc.port.client to a number such as 10002 to avoid having GC pick port 9998 - which GC cannot - since 9998 has already been used by the second TServer running. So the second question I have is why do these scripts start TServers first, ahead of master, monitor and GC?
I will try joining the Slack chat on Wednesday to discuss further if needed. Thank you and @keith-turner for working through this with me.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] keith-turner commented on issue #1568: Support multiple
tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
keith-turner commented on issue #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#issuecomment-602669425
> Let's set up a video chat to discuss
I am in favor of that.
> I'm already several hours of revisions into this response... I wish I were kidding
It is very hard to word things sometimes. Not sure if what I said earlier came across. What I was trying to say is that I would like to get to the point where I can come to the same conclusion as you. I feel like I am missing some knowledge to achieve 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] arvindshmicrosoft commented on a change in pull request
#1568: Support multiple tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
arvindshmicrosoft commented on a change in pull request #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#discussion_r398235057
##########
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:
Thank you and I did plan to submit a docs PR for `NUM_TSERVERS` usage when this change is done.
----------------------------------------------------------------
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] keith-turner edited a comment on issue #1568: Support
multiple tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
keith-turner edited a comment on issue #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#issuecomment-602084624
> The purpose of the bin/accumulo-service script is to provide a mechanism to manage a single process, in order to provide a simple mechanism to build other tools around.
> Furthermore, this script isn't really intended to be used by anything other than the accumulo-cluster script.
@ctubbsii the above statements seem to contradict each other. I think these changes are very useful. Your objection feels a bit arbitrary and subjective. However I trust your intuition and historical knowledge on this matter and would follow your recommendations based on this. I was trying to think of ways to make this less subjective, so that I could come to the same conclusions on my own. One thought I had was maybe you could document the purpose/design principals/intentions as comments in the `accumulo-cluster` and `accumulo-service` scripts. These would serve as a guide for these changes and future changes.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] keith-turner edited a comment on issue #1568: Support
multiple tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
keith-turner edited a comment on issue #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#issuecomment-602084624
> The purpose of the bin/accumulo-service script is to provide a mechanism to manage a single process, in order to provide a simple mechanism to build other tools around.
> Furthermore, this script isn't really intended to be used by anything other than the accumulo-cluster script.
@ctubbsii the above statements seem to contradict each other. I think these changes are very useful. Your objection feels a bit arbitrary and subjective. However I trust your intuition and historical knowledge on this matter and would follow your recommendations base on thi. I was trying to think of ways to make this less subjective, so that I could come to the same conclusions on my own. One thought I had was maybe you could document the purpose/design principals/intentions as comments in the `accumulo-cluster` and `accumulo-service` scripts. These would serve as a guide for these changes and future changes.
----------------------------------------------------------------
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] arvindshmicrosoft commented on a change in pull request
#1568: Support multiple tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
arvindshmicrosoft commented on a change in pull request #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#discussion_r398113539
##########
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:
Cool! Since this was existing code, I did not pay attention. I will address it.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii merged pull request #1568: Support multiple
tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568
----------------------------------------------------------------
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] arvindshmicrosoft commented on a change in pull request
#1568: Support multiple tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
arvindshmicrosoft commented on a change in pull request #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#discussion_r399076381
##########
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=
Review comment:
Thanks Keith. With the suggestions that Christopher made, this has been handled in a different way now. Please take a look.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] arvindshmicrosoft commented on a change in pull request
#1568: Support multiple tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
arvindshmicrosoft commented on a change in pull request #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#discussion_r399075699
##########
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 changes have been done - please review when you have a chance.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] arvindshmicrosoft commented on issue #1568: Support
multiple tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
arvindshmicrosoft commented on issue #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#issuecomment-605033055
Thank you @ctubbsii for the guidance and insights. I learnt a lot in this process. I'll follow up with a PR to update the docs.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] keith-turner commented on a change in pull request
#1568: Support multiple tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#discussion_r399562448
##########
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=
Review comment:
Those changes look good and 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] arvindshmicrosoft commented on a change in pull request
#1568: Support multiple tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
arvindshmicrosoft commented on a change in pull request #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#discussion_r398120277
##########
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 - I think the usage of the semicolon in my script was the reason the variable was not visible within the accumulo-service script. I guess I was going off the syntax in the [docs](https://accumulo.apache.org/docs/2.x/administration/in-depth-install#running-multiple-tabletservers-on-a-single-node) which has a semicolon between the ACCUMULO_SEVICE_INSTANCE variable and the accumulo script. I wonder now if that is a typo which needs to be addressed?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] keith-turner edited a comment on issue #1568: Support
multiple tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
keith-turner edited a comment on issue #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#issuecomment-602084624
> The purpose of the bin/accumulo-service script is to provide a mechanism to manage a single process, in order to provide a simple mechanism to build other tools around.
> Furthermore, this script isn't really intended to be used by anything other than the accumulo-cluster script.
@ctubbsii the above statements seem to contradict each other. I think these changes are very useful. Your objection feels a bit arbitrary and subjective. However I trust your intuition and historical knowledge on this matter and would follow your recommendations base on this. I was trying to think of ways to make this less subjective, so that I could come to the same conclusions on my own. One thought I had was maybe you could document the purpose/design principals/intentions as comments in the `accumulo-cluster` and `accumulo-service` scripts. These would serve as a guide for these changes and future changes.
----------------------------------------------------------------
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on issue #1568: Support multiple
tservers / node in accumulo-service
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1568: Support multiple tservers / node in accumulo-service
URL: https://github.com/apache/accumulo/pull/1568#issuecomment-604908642
Thanks @arvindshmicrosoft ! I really like how this change worked out. This is a nice feature to have in our cluster management script.
----------------------------------------------------------------
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services