Avoid pruning oximeter producers registered during a refresh #6905
Conversation
bnaecker
commented
Oct 18, 2024
bnaecker
commented
- Add generation numbers to the collection of oximeter producers, and assign the currrent generation to each producer as it is registered.
- Modify the refresh method to first take the generation number before starting to list current producers. Then use that to avoid pruning producers that are new since we started refreshing our list.
- Fixes test failed in CI: integration_tests::metrics::test_metrics #6895 and possibly test failed in CI: test_oximeter_reregistration #6901
- Add generation numbers to the collection of oximeter producers, and assign the currrent generation to each producer as it is registered. - Modify the refresh method to first take the generation number before starting to list current producers. Then use that to avoid pruning producers that are _new_ since we started refreshing our list. - Fixes #6895 and possibly #6901
|
I'd really like a close review on this one. It is extremely difficult to trigger this flaky behavior, since we need to start a server pretending to be Nexus; interrupt a request to refresh the producer list; and concurrently register a new producer as the same time. The test would then assert that we've not pruned that concurrently-registered producer. I spent most of the day trying and failing to test this, but could not come up with an approach that reliably triggers the bug. Given that we're trying to reduce test flakes, I'm very hesitant to add a new test that would itself be flaky. I think I've got the logic on bumping the generation numbers correctly, anytime we modify the map of producers, but I could definitely use other, more experienced eyes. |
