GH-35 randomize validation time #73
Conversation
maksymvavilov
force-pushed
the
GH-35
branch
3 times, most recently
from
051c334 to
91c61da
Compare
| @@ -69,6 +70,13 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( | |||
| logger := log.FromContext(ctx) | |||
|
|
|||
| reconcileStart = metav1.Now() | |||
|
|
|||
| // randomize validation reconcile delay | |||
| seconds := float64(validationRequeueTime.Milliseconds()) | |||
There was a problem hiding this comment.
nit: is this var name a bit misleading? should it be milliseconds?
| seconds := float64(validationRequeueTime.Milliseconds()) | ||
| validationRequeueTime = time.Millisecond * time.Duration(rand.Int63nRange( | ||
| int64(seconds*0.9), | ||
| int64(seconds*1.1))) |
There was a problem hiding this comment.
ok so the interval will be between 90 and 110 % of the validationRequeueTime? wondering if we should increase the possible range a bit. Like 50-150% . Anyway just a thought
There was a problem hiding this comment.
possibly worth isolating into a function and adding a simple unit test?
There was a problem hiding this comment.
increased variance to 50% and isolated into a separate function.
Not sure though how to reliably test the randomization function: we could have a test just ensuring that we get a valid value from this (but given how the code works we always will have a valid time.Duration value out of this) and we already testing for "usable" duration in the integration suite (if it to randomize seconds into minutes we will timeout out of the integration suite). The suggested way to test randomness is through a lot of iterations to build up a statistically significant amount of data - that doesn't sound like something worth doing.
In any case - I put together a tiny test that should be what you had in mind
Went through the package and there are a few functions that could use unit tests as well. If we do this - maybe better to move our integration suite into a separate package and only keep filename_test.go for the unit suite? I can create a issue for "better days" to reorganize tests in the DNS Operator if that sounds cool 🤔
There was a problem hiding this comment.
Yeah not huge value here, I guess we could do a number of iterations and ensure the value is within the expected range?
|
/lgtm |
maksymvavilov
force-pushed
the
GH-35
branch
2 times, most recently
from
9b59ca6 to
0e5fd39
Compare
| duration time.Duration | ||
| }{ | ||
| { | ||
| name: "returns valid duration in range", |
There was a problem hiding this comment.
yep this is fine perhaps forcing it to do a loop and a set number of iterations testing each is within the range?
There was a problem hiding this comment.
🤦 yep, that is smart. Done
Randomize validation reconcilation delays