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 2021/06/02 15:54:43 UTC
[GitHub] [accumulo] Manno15 opened a new pull request #2133: Fix warnings in accumulo-cluster scrip
Manno15 opened a new pull request #2133:
URL: https://github.com/apache/accumulo/pull/2133
Found from looking into https://issues.apache.org/jira/browse/ACCUMULO-2987.
--
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
[GitHub] [accumulo] ctubbsii merged pull request #2133: Fix warnings in accumulo-cluster scrip
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #2133:
URL: https://github.com/apache/accumulo/pull/2133
--
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
[GitHub] [accumulo] Manno15 commented on a change in pull request #2133: Fix warnings in accumulo-cluster scrip
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2133:
URL: https://github.com/apache/accumulo/pull/2133#discussion_r645026477
##########
File path: assemble/bin/accumulo-cluster
##########
@@ -235,21 +235,21 @@ function stop_tservers() {
function kill_all() {
echo "Killing Accumulo cluster..."
- for manager in $(grep -v '^#' "${conf}/$manager_file"); do
+ grep -v '^#' "${conf}/$manager_file" | while read -r manager; do
kill_service "$manager" manager
done
- for gc in $(grep -v '^#' "${conf}/gc"); do
+ grep -v '^#' "${conf}/gc" | while read -r gc; do
kill_service "$gc" gc
done
kill_service "$monitor" monitor
- for tracer in $(egrep -v '(^#|^\s*$)' "${conf}/tracers"); do
+ grep -Ev '(^#|^\s*$)' "${conf}/tracers" | while read -r tracer; do
kill_service "$tracer" tracer
done
- for host in $(egrep -v '(^#|^\s*$)' "${conf}/tservers"); do
+ grep -Ev '(^#|^\s*$)' "${conf}/tservers" | while read -r server; do
Review comment:
Good catch. 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
[GitHub] [accumulo] ctubbsii merged pull request #2133: Fix warnings in accumulo-cluster scrip
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #2133:
URL: https://github.com/apache/accumulo/pull/2133
--
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
[GitHub] [accumulo] Manno15 commented on pull request #2133: Fix warnings in accumulo-cluster scrip
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #2133:
URL: https://github.com/apache/accumulo/pull/2133#issuecomment-853949515
I made the requested changes in the latest commit @ctubbsii.
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2133: Fix warnings in accumulo-cluster scrip
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2133:
URL: https://github.com/apache/accumulo/pull/2133#discussion_r644797502
##########
File path: assemble/bin/accumulo-cluster
##########
@@ -128,7 +128,7 @@ function start_service() {
function start_tservers() {
echo -n "Starting tablet servers ..."
count=1
- for server in $(egrep -v '(^#|^\s*$)' "${conf}/tservers"); do
+ grep -E -v '(^#|^\s*$)' < "${conf}/tservers" | while read -r server; do
Review comment:
The indentation here doesn't look right. Also, you don't need to read in the file with `<` to read it in as STDIN, because grep itself takes file names as arguments. So, you can just do:
```suggestion
grep -Ev '(^#|^\s*$)' "${conf}/tservers" | while read -r server; do
```
##########
File path: assemble/bin/accumulo-cluster
##########
@@ -147,17 +147,17 @@ function start_all() {
start_tservers
fi
- for host in $(egrep -v '(^#|^\s*$)' "${conf}/$manager_file"); do
- start_service "$host" manager
- done
+ grep -E -v '(^#|^\s*$)' < "${conf}/$manager_file" | while read -r host; do
+ start_service "$host" manager
+ done
- for host in $(egrep -v '(^#|^\s*$)' "${conf}/gc"); do
- start_service "$host" gc
- done
+ grep -E -v '(^#|^\s*$)' < "${conf}/gc" | while read -r host; do
+ start_service "$host" gc
+ done
- for host in $(egrep -v '(^#|^\s*$)' "${conf}/tracers"); do
- start_service "$host" tracer
- done
+ grep -E -v '(^#|^\s*$)' < "${conf}/tracers" | while read -r host; do
+ start_service "$host" tracer
+ done
Review comment:
The indentation changes here don't look right.
##########
File path: assemble/bin/accumulo-cluster
##########
@@ -299,9 +299,9 @@ function stop_all() {
function stop_here() {
# Determine hostname without errors to user
- hosts_to_check=($(hostname -a 2> /dev/null | head -1) $(hostname -f))
+ hosts_to_check="$(hostname -a 2> /dev/null | head -1) $(hostname -f)"
- if egrep -q localhost\|127.0.0.1 "${conf}/tservers"; then
+ if grep -E -q localhost\|127.0.0.1 "${conf}/tservers"; then
Review comment:
The pattern here would make more sense quoted, rather than escaped, and it looks like the dots aren't being matched properly. Here's a fix:
```suggestion
if grep -Eq 'localhost|127[.]0[.]0[.]1' "${conf}/tservers"; then
```
##########
File path: assemble/bin/accumulo-cluster
##########
@@ -299,9 +299,9 @@ function stop_all() {
function stop_here() {
# Determine hostname without errors to user
- hosts_to_check=($(hostname -a 2> /dev/null | head -1) $(hostname -f))
+ hosts_to_check="$(hostname -a 2> /dev/null | head -1) $(hostname -f)"
Review comment:
This is wrong. It should be assigning as an array, not a flat string. To properly quote the elements of the array, do:
```suggestion
hosts_to_check=("$(hostname -a 2> /dev/null | head -1)" "$(hostname -f)")
```
##########
File path: assemble/bin/accumulo-cluster
##########
@@ -59,16 +59,16 @@ function verify_config {
unset manager1
if [[ -f "${conf}/$manager_file" ]]; then
- manager1=$(egrep -v '(^#|^\s*$)' "${conf}/$manager_file" | head -1)
+ manager1=$(grep -E -v '(^#|^\s*$)' "${conf}/$manager_file" | head -1)
Review comment:
Single-character options can be combined. This doesn't really matter, but the end result of converting from `egrep -v` to `grep -Ev` is that it is the same number of characters, rather than `grep -E -v`, which is 2 more.
```suggestion
manager1=$(grep -Ev '(^#|^\s*$)' "${conf}/$manager_file" | head -1)
```
##########
File path: assemble/bin/accumulo-cluster
##########
@@ -235,21 +235,21 @@ function stop_tservers() {
function kill_all() {
echo "Killing Accumulo cluster..."
- for manager in $(grep -v '^#' "${conf}/$manager_file"); do
+ grep -v '^#' "${conf}/$manager_file" | while read -r manager; do
kill_service "$manager" manager
done
- for gc in $(grep -v '^#' "${conf}/gc"); do
+ grep -v '^#' "${conf}/gc" | while read -r gc; do
kill_service "$gc" gc
done
kill_service "$monitor" monitor
- for tracer in $(egrep -v '(^#|^\s*$)' "${conf}/tracers"); do
+ grep -Ev '(^#|^\s*$)' "${conf}/tracers" | while read -r tracer; do
kill_service "$tracer" tracer
done
- for host in $(egrep -v '(^#|^\s*$)' "${conf}/tservers"); do
+ grep -Ev '(^#|^\s*$)' "${conf}/tservers" | while read -r server; do
Review comment:
Wrong variable name:
```suggestion
grep -Ev '(^#|^\s*$)' "${conf}/tservers" | while read -r host; do
```
--
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
[GitHub] [accumulo] Manno15 commented on pull request #2133: Fix warnings in accumulo-cluster scrip
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #2133:
URL: https://github.com/apache/accumulo/pull/2133#issuecomment-853949515
I made the requested changes in the latest commit @ctubbsii.
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2133: Fix warnings in accumulo-cluster scrip
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2133:
URL: https://github.com/apache/accumulo/pull/2133#discussion_r644797502
##########
File path: assemble/bin/accumulo-cluster
##########
@@ -128,7 +128,7 @@ function start_service() {
function start_tservers() {
echo -n "Starting tablet servers ..."
count=1
- for server in $(egrep -v '(^#|^\s*$)' "${conf}/tservers"); do
+ grep -E -v '(^#|^\s*$)' < "${conf}/tservers" | while read -r server; do
Review comment:
The indentation here doesn't look right. Also, you don't need to read in the file with `<` to read it in as STDIN, because grep itself takes file names as arguments. So, you can just do:
```suggestion
grep -Ev '(^#|^\s*$)' "${conf}/tservers" | while read -r server; do
```
##########
File path: assemble/bin/accumulo-cluster
##########
@@ -147,17 +147,17 @@ function start_all() {
start_tservers
fi
- for host in $(egrep -v '(^#|^\s*$)' "${conf}/$manager_file"); do
- start_service "$host" manager
- done
+ grep -E -v '(^#|^\s*$)' < "${conf}/$manager_file" | while read -r host; do
+ start_service "$host" manager
+ done
- for host in $(egrep -v '(^#|^\s*$)' "${conf}/gc"); do
- start_service "$host" gc
- done
+ grep -E -v '(^#|^\s*$)' < "${conf}/gc" | while read -r host; do
+ start_service "$host" gc
+ done
- for host in $(egrep -v '(^#|^\s*$)' "${conf}/tracers"); do
- start_service "$host" tracer
- done
+ grep -E -v '(^#|^\s*$)' < "${conf}/tracers" | while read -r host; do
+ start_service "$host" tracer
+ done
Review comment:
The indentation changes here don't look right.
##########
File path: assemble/bin/accumulo-cluster
##########
@@ -299,9 +299,9 @@ function stop_all() {
function stop_here() {
# Determine hostname without errors to user
- hosts_to_check=($(hostname -a 2> /dev/null | head -1) $(hostname -f))
+ hosts_to_check="$(hostname -a 2> /dev/null | head -1) $(hostname -f)"
- if egrep -q localhost\|127.0.0.1 "${conf}/tservers"; then
+ if grep -E -q localhost\|127.0.0.1 "${conf}/tservers"; then
Review comment:
The pattern here would make more sense quoted, rather than escaped, and it looks like the dots aren't being matched properly. Here's a fix:
```suggestion
if grep -Eq 'localhost|127[.]0[.]0[.]1' "${conf}/tservers"; then
```
##########
File path: assemble/bin/accumulo-cluster
##########
@@ -299,9 +299,9 @@ function stop_all() {
function stop_here() {
# Determine hostname without errors to user
- hosts_to_check=($(hostname -a 2> /dev/null | head -1) $(hostname -f))
+ hosts_to_check="$(hostname -a 2> /dev/null | head -1) $(hostname -f)"
Review comment:
This is wrong. It should be assigning as an array, not a flat string. To properly quote the elements of the array, do:
```suggestion
hosts_to_check=("$(hostname -a 2> /dev/null | head -1)" "$(hostname -f)")
```
##########
File path: assemble/bin/accumulo-cluster
##########
@@ -59,16 +59,16 @@ function verify_config {
unset manager1
if [[ -f "${conf}/$manager_file" ]]; then
- manager1=$(egrep -v '(^#|^\s*$)' "${conf}/$manager_file" | head -1)
+ manager1=$(grep -E -v '(^#|^\s*$)' "${conf}/$manager_file" | head -1)
Review comment:
Single-character options can be combined. This doesn't really matter, but the end result of converting from `egrep -v` to `grep -Ev` is that it is the same number of characters, rather than `grep -E -v`, which is 2 more.
```suggestion
manager1=$(grep -Ev '(^#|^\s*$)' "${conf}/$manager_file" | head -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
[GitHub] [accumulo] Manno15 commented on a change in pull request #2133: Fix warnings in accumulo-cluster scrip
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2133:
URL: https://github.com/apache/accumulo/pull/2133#discussion_r645026477
##########
File path: assemble/bin/accumulo-cluster
##########
@@ -235,21 +235,21 @@ function stop_tservers() {
function kill_all() {
echo "Killing Accumulo cluster..."
- for manager in $(grep -v '^#' "${conf}/$manager_file"); do
+ grep -v '^#' "${conf}/$manager_file" | while read -r manager; do
kill_service "$manager" manager
done
- for gc in $(grep -v '^#' "${conf}/gc"); do
+ grep -v '^#' "${conf}/gc" | while read -r gc; do
kill_service "$gc" gc
done
kill_service "$monitor" monitor
- for tracer in $(egrep -v '(^#|^\s*$)' "${conf}/tracers"); do
+ grep -Ev '(^#|^\s*$)' "${conf}/tracers" | while read -r tracer; do
kill_service "$tracer" tracer
done
- for host in $(egrep -v '(^#|^\s*$)' "${conf}/tservers"); do
+ grep -Ev '(^#|^\s*$)' "${conf}/tservers" | while read -r server; do
Review comment:
Good catch. 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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2133: Fix warnings in accumulo-cluster scrip
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2133:
URL: https://github.com/apache/accumulo/pull/2133#discussion_r645024091
##########
File path: assemble/bin/accumulo-cluster
##########
@@ -235,21 +235,21 @@ function stop_tservers() {
function kill_all() {
echo "Killing Accumulo cluster..."
- for manager in $(grep -v '^#' "${conf}/$manager_file"); do
+ grep -v '^#' "${conf}/$manager_file" | while read -r manager; do
kill_service "$manager" manager
done
- for gc in $(grep -v '^#' "${conf}/gc"); do
+ grep -v '^#' "${conf}/gc" | while read -r gc; do
kill_service "$gc" gc
done
kill_service "$monitor" monitor
- for tracer in $(egrep -v '(^#|^\s*$)' "${conf}/tracers"); do
+ grep -Ev '(^#|^\s*$)' "${conf}/tracers" | while read -r tracer; do
kill_service "$tracer" tracer
done
- for host in $(egrep -v '(^#|^\s*$)' "${conf}/tservers"); do
+ grep -Ev '(^#|^\s*$)' "${conf}/tservers" | while read -r server; do
Review comment:
Wrong variable name:
```suggestion
grep -Ev '(^#|^\s*$)' "${conf}/tservers" | while read -r host; do
```
--
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