[oximeter] really fix race in test_self_stat_collection_count#10585
Conversation
Created using spr 1.3.6-beta.1 [skip ci]
Created using spr 1.3.6-beta.1
bnaecker
left a comment
bnaecker
left a comment
There was a problem hiding this comment.
Thanks! This looks good overall, but I have one comment about disabling collections entirely.
| // Whether collection tasks schedule collections on their producers' | ||
| // intervals. Always enabled in production; tests disable it to control | ||
| // exactly how many collections occur. | ||
| scheduled_collections: ScheduledCollections, |
There was a problem hiding this comment.
I'm a little nervous about setting up a mechanism where no collections are ever performed on the usual timer. Leaking that into production would be pretty bad. Can we instead set up the tests we're fixing here with very long collection intervals (i.e., days)? That would provide complete control over the collections without any additional risk.
There was a problem hiding this comment.
Heh that's actually what I had earlier and just didn't feel great about it! I'll switch back to that since you prefer it.
There was a problem hiding this comment.
Thanks! Both suck a little, but I think at least that approach can't torpedo all collections rack-wide.
sunshowers
changed the base branch from
sunshowers/spr/main.oximeter-really-fix-race-in-test_self_stat_collection_count
to
main
Created using spr 1.3.6-beta.1
sunshowers
deleted the
sunshowers/spr/oximeter-really-fix-race-in-test_self_stat_collection_count
branch
2 participants
Following on from #10040.
Switch the test to being level-triggered, and don't rely on paused time because we do real I/O which would introduce a second time domain.
Identified and fixed by Claude based on the flake-patterns doc I wrote in #10577.
Depends on:
Fixes #7255.
Fixes #8636.