shuffle targets during parse phase #343
Conversation
| @@ -82,7 +82,6 @@ type JSONGateResolverBuilder struct { | |||
| targets map[string][]targetHost | |||
| resolvers []*JSONGateResolver | |||
|
|
|||
| rand *rand.Rand | |||
There was a problem hiding this comment.
I think it's ok to just use the global rand in the rand package here. We actually introduced a bug when we added GetTargets because rand.Rand is not safe to use concurrently (the top level package funcs are safe)
There was a problem hiding this comment.
That's fine, but then we need to call rand.Seed during Init somewhere
There was a problem hiding this comment.
rand.Seed was deprecated in go1.20, so if we want to rebase off a current branch, we might want to avoid it. If possible, I'd like to avoid using package level global instances like rand.globalRandGenerator because they can be poisoned by other packages. If concurrency is a problem, could we use a rand.Rand and a sync.Mutex?
There was a problem hiding this comment.
noted, rewritten to use a rand.Rand with a sync.Mutex
|
Confirmed, this does fix the issue when I re-run tests. |
| j := head + b.rand.Intn(tail-head+1) | ||
| s.mu.Lock() | ||
| j := head + s.rand.Intn(tail-head+1) | ||
| s.mu.Unlock() |
There was a problem hiding this comment.
This is likely irrelevant since we're not ever calling this under load, but my .02 would be that we don't need to take and drop the lock so often, and instead just taking it once at the beginning of sort and dropping at end would be fine.
* shuffle targets during parse phase * don't use global rand
* shuffle targets during parse phase * don't use global rand
* shuffle targets during parse phase * don't use global rand
Description
@rjlaine found an issue where if the list of hosts changes e.g. from a scale up the new hosts may not end up in the list of target hosts. We can fix this by doing a shuffle sort during the parse phase.