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:
[email protected]
