search

apache / accumulo-testing

Apache Accumulo Testing
https://accumulo.apache.org
Apache License 2.0
15 stars 40 forks source link

Agitator not randomly starting / stopping servers #248

Closed dlmarion closed 2 years ago

dlmarion commented 2 years ago

The agitator was refactored as part of https://github.com/apache/accumulo-testing/pull/184. I think there is a bug in the new script, specifically the use of the word RANDOM instead of the function $RANDOM here and here.

@ctubbsii - can you confirm?

dlmarion commented 2 years ago

So, I did some testing, and it's not the use of RANDOM vs $RANDOM. The log has the same IP being selected over and over again. So, maybe it's something else.

ctubbsii commented 2 years ago

So, I did some testing, and it's not the use of RANDOM vs $RANDOM. The log has the same IP being selected over and over again. So, maybe it's something else.

I'm not that familiar with RANDOM vs $RANDOM, but in testing the existing expressions, they seemed to both work for me.

ctubbsii commented 2 years ago

I don't see anything immediately obviously wrong with the use of RANDOM to select nodes from the bash array to kill. But, I wouldn't append to the kill array using the index, like the script is currently doing (array[index]=foo). Instead, I'd do array=("${array[@]}" foo), which I am more familiar with. I don't know if the former works on older bash, but when testing, it did seem to work on mine, so it could be fine. (I'm running 5.2.2)

dlmarion commented 2 years ago

I found it. Can't believe I didn't see it earlier.

dlmarion commented 2 years ago

I'm wondering if this was meant to be if num_hosts == 1 ...

ctubbsii commented 2 years ago

@dlmarion Those two lines of code look okay to me. It looks like it's a simplified case where it avoids all the array logic if you're only killing up to 1 node. (max_kill == 1). Though, in that case, I suppose it could pick a random one, instead of always selecting the first.