Skip to main content
added 4 characters in body
Source Link
muru
  • 973
  • 4
  • 14
  • Note how I eliminated the IFS=: read ... and replaced "${config[1]}""${config[1]}" with "${CONFIG_PORT[index]}""${CONFIG_PORT[index]}", which immediately makes the meaning of that variable clearer.
  • The indentation was mixed up, both in this and the other loop. Be consistent.
  • The echo "" > log.txt from earlier was unnecessary, since you use > log.txt here, the file will be truncated anyway.
  • You are losing the output from all but the last mongod process, since you use > instead of >>.
  • Always quote your variables.
  • Note how I eliminated the IFS=: read ... and replaced "${config[1]}" with "${CONFIG_PORT[index]}", which immediately makes the meaning of that variable clearer.
  • The indentation was mixed up, both in this and the other loop. Be consistent.
  • The echo "" > log.txt from earlier was unnecessary, since you use > log.txt here, the file will be truncated anyway.
  • You are losing the output from all but the last mongod process, since you use > instead of >>.
  • Always quote your variables.
  • Note how I eliminated the IFS=: read ... and replaced "${config[1]}" with "${CONFIG_PORT[index]}", which immediately makes the meaning of that variable clearer.
  • The indentation was mixed up, both in this and the other loop. Be consistent.
  • The echo "" > log.txt from earlier was unnecessary, since you use > log.txt here, the file will be truncated anyway.
  • You are losing the output from all but the last mongod process, since you use > instead of >>.
  • Always quote your variables.
Source Link
muru
  • 973
  • 4
  • 14

To begin with, I agree with Jesse_b that command substitution should use $(). There's no reason to use backticks in a bash script that uses arrays (that's to say, you're not particularly aiming for portability with some obscure shell).

Indentation

I didn't notice until the end of the script that the majority of your code is in a {...} block. Indenting that portion of the code more than the braces would have helped.


Reading the config file

Continuing from the previous point, the majority of your code seems to be in the block because of the pipe used for reading the file.

Avoid the pipe

You can use process substitution to avoid the pipe:

while read -r line
do
...
done < <(sed '...' file)

This way, you eliminate the pipe and the need to keep most of your code in a subshell. You can also make the comments more friendly by allowing leading spaces:

sed '/^[[:space:]]*#/d;/^[[:space:]]*$/d'

Or:

sed '/^[[:space:]]*\(#.*\)\?$/d'

Eliminate unnecessary reads and variables

That said, the code for reading the lines seems to be unnecessarily convoluted: a read first and then again to split? Why not do it in one op? You also use an array, then immediately proceed to assign the array elements to two named variables and thereafter ignore the array completely. Just set those variables via read directly;

while IFS='=' read -r key value
do
...
done < <(sed '/^[[:space:]]*\(#.*\)\?$/d' file)

Bug, again, later on you split the values read here, because they are all host-port pairs. If so, then do the splitting now, and save them in separate arrays in the first place:

declare -a CONFIG_HOSTS CONFIG_PORTS SHARD_HOSTS SHARD_PORTS
while IFS='=:' read -r key host port
do
...
done < <(...)

Repetitive if statements

Next, the chain of ifs are so repetitive that I suggest taking out the echos, by making an array out of the labels you print:

declare -A key_labels
key_labels["config"]="mongod config server"
key_labels["mongos"]="mongos instance" 
key_labels["shard"]="shard instance"

Then, a single printf should do. And the block of ifs also look like they fit the role of a case statement. So:

printf '%s: %s:%s\n' "${key_labels[$key]}" "$host" "$port"
case key in
config)
    CONFIG_HOST+=("$host")
    CONFIG_PORT+=("$port")
    ;;
mongos)
    MONGOS_HOST="$host"
    MONGOS_PORT="$port"
    ;;
shard)
    SHARD_HOST+=("$host")
    SHARD_PORT+=("$port")
    ;;
esac

So, the entire config reading section becomes:

declare -a CONFIG_HOSTS CONFIG_PORTS SHARD_HOSTS SHARD_PORTS
declare -A key_labels
key_labels["config"]="mongod config server"
key_labels["mongos"]="mongos instance" 
key_labels["shard"]="shard instance"

while IFS='=:' read -r key host port
do
    printf '%s: %s:%s\n' "${key_labels[$key]}" "$host" "$port"
    case key in
    config)
        CONFIG_HOSTS+=("$host")
        CONFIG_PORTS+=("$port")
        ;;
    mongos)
        MONGOS_HOST="$host"
        MONGOS_PORT="$port"
        ;;
    shard)
        SHARD_HOSTS+=("$host")
        SHARD_PORTS+=("$port")
        ;;
    esac
done < <(sed '/^[[:space:]]*\(#.*\)\?$/d' file)

Looping with an index

Here, since you need the indices for creating the directories, you can just ask bash for the indices directly. Instead of:

index=0
for conf in "${CONFIG[@]}"
do
...
    index=$(($index + 1))
done

Do:

for index in "${!CONFIG_HOSTS[@]}"
do    
    mkdir /data/config"$index"
    echo "starting config server $index" 
    mongod --configsvr --port "${CONFIG_PORT[index]}" --dbpath /data/config"$index" --replSet conf > log.txt &
    sleep 1
done

Other notes on this block:

  • Note how I eliminated the IFS=: read ... and replaced "${config[1]}" with "${CONFIG_PORT[index]}", which immediately makes the meaning of that variable clearer.
  • The indentation was mixed up, both in this and the other loop. Be consistent.
  • The echo "" > log.txt from earlier was unnecessary, since you use > log.txt here, the file will be truncated anyway.
  • You are losing the output from all but the last mongod process, since you use > instead of >>.
  • Always quote your variables.

This index method can be applied to the other loop as well. Also, every instance of IFS=: read ... <<<"$foo" can now be eliminated, and the resultant variables replaced with the arrays we created earlier, as shown in point 1 above. So, things like config0[1] become ${CONFIG_PORT[0]}.