Test: wait for network changes in TestNetworkDBNodeJoinLeaveIteration#42829
Conversation
thaJeztah
added
area/networking
8accd45 to
5f5937d
Compare
|
Thanks for contributing @zq-david-wang - and apologies for the long delay; I was trying to get more eyes on this fix, but it slipped off my radar. Let me try to get some eyes on this one. I did a quick rebase of your PR to make CI run again, and I included your description from GitHub in the commit message (which can be useful when looking through git history). |
| nntime, ncount := db.networkClock.Time(), 0 | ||
| for i := 0; i < maxRetries; i++ { | ||
| nntime, ncount = db.networkClock.Time(), 0 | ||
| if nodes, ok := db.networkNodes[network]; ok { |
There was a problem hiding this comment.
Does this map access need to be done in a critical section? I see there's an RWMutex embedded in the NetworkDB struct, and application code only accesses the networkNodes map while holding the lock.
| t.Errorf("%v(%v): Node existence verification for node %s failed", db.config.Hostname, db.config.NodeID, node) | ||
| } | ||
|
|
||
| func (db *NetworkDB) witnessNetworkNodeChanges(t *testing.T, ntime serf.LamportTime, network string, count int) bool { |
There was a problem hiding this comment.
| func (db *NetworkDB) witnessNetworkNodeChanges(t *testing.T, ntime serf.LamportTime, network string, count int) bool { | |
| func (db *NetworkDB) witnessNetworkNodeChanges(t *testing.T, ntime serf.LamportTime, network string, count int) bool { | |
| t.Helper() |
| if nntime != ntime && ncount == count { | ||
| return true | ||
| } | ||
| time.Sleep(sleepInterval) |
There was a problem hiding this comment.
Have you considered using poll.WaitOn instead of hand-rolling a polling loop?
|
Ah, wrong tab; sorry |
191f5bc to
63c88ef
Compare
|
@thaJeztah I am not sure whether this PR is still necessary, after such a long period, any recent random failure for this test case? It is ok for me to close this if the PR is not needed anymore, otherwise @corhere , could you help review the new code, thx~ |
In network node change test, the expected behavior is focused on how many nodes left in networkDB, besides timing issues, things would also go tricky for a leave-then-join sequence, if the check (counting the nodes) happened before the first "leave" event, then the testcase actually miss its target and report PASS without verifying its final result; if the check happened after the 'leave' event, but before the 'join' event, the test would report FAIL unnecessary; This code change would check both the db changes and the node count, it would report PASS only when networkdb has indeed changed and the node count is expected. Signed-off-by: David Wang <[email protected]>
63c88ef to
f499c6b
Compare
|
I think the test was still flaky (at least to some extend), and (IIRC) mostly waiting for the suggestions to be addressed; so thanks for updating it! @corhere ptal |
| var ( | ||
| dbIndex int32 | ||
| staleNetworkTime [2]serf.LamportTime | ||
| expectNodeCount int | ||
| network = "network1" | ||
| ) | ||
| dbChangeWitness := func(t poll.LogT) poll.Result { |
There was a problem hiding this comment.
Those variables are functionally arguments to the dbChangeWitness poll check function. It would be a lot clearer if they were actually passed in as function arguments rather than by mutating variables.
| var ( | |
| dbIndex int32 | |
| staleNetworkTime [2]serf.LamportTime | |
| expectNodeCount int | |
| network = "network1" | |
| ) | |
| dbChangeWitness := func(t poll.LogT) poll.Result { | |
| dbChangeWitness := func( | |
| dbIndex int32, | |
| staleNetworkTime [2]serf.LamportTime, | |
| expectNodeCount int, | |
| network string, | |
| ) poll.Check { | |
| return func(t poll.LogT) poll.Result { |
There was a problem hiding this comment.
This can be addressed in a follow-up PR since it's all private to the one test
|
Thanks! Let's get this one in 👍 |
3 participants
Signed-off-by: David Wang [email protected]
fix #42698
In network node change test, the expected behavior is focused on how many nodes left in networkDB, besides timing issues, things would also go tricky for a leave-then-join sequence, if the check (counting the nodes) happened before the first "leave" event, then the testcase actually miss its target and report PASS without verifying its final result; if the check happened after the 'leave' event, but before the 'join' event, the test would report FAIL unnecessary;
This code change would check both the db changes and the node count, it would report PASS only when networkdb has indeed changed and the node count is expected