Memberlist: support for debouncing notifications

[seizethedave]

What this PR does:

• Adds a layer of buffering to Memberlist's notification handling so that notifications are fired at most once per NotifyInterval, at which point it will deliver notifications using the most recently-observed data.
• Adds a new config flag to control this interval: -memberlist.notify-interval which defaults to 0 (off).

Motivation for this change:

In clusters where the memberlist KVStore watched by Ring has many replicas, redeploying those replicas can cause WatchKey and updateRingState to be called hundreds of times per second. When there are many concurrent goroutines calling ring.ShuffleShard, the high rate of updateRingState calls (which take locks and clear caches) can create heavy lock contention and latency as ShuffleShard attempts to take locks in order to repopulate those caches.

Checklist

• Tests updated
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[pstibrany]

Thank you, PR looks fine to me. Tests that do not initialize UpdateInterval are now failing.

I think the change is safe to enable it by default, without the need to go back to previous behaviour.

Alternative to this would be implementing rate-limiting in memberlist KV client's WatchKey/WatchPrefix implementations, which would automatically cover all rings, but only benefit memberlist users. (However we would like to get rid of supporting other KV stores eventually, so that's not a big deal)

[seizethedave]

I vendored this change locally and it actually does break tests in Mimir, as ruler tests attempt to wait 100ms for ring changes to propagate. I may just bite the bullet and make this change a no-op by default (synchronous propagation) and make the added delay configurable. @pstibrany let me know if you have thoughts.

[seizethedave]

/find-flaky-tests

[pstibrany]

I vendored this change locally and it actually does break tests in Mimir, as ruler tests attempt to wait 100ms for ring changes to propagate. I may just bite the bullet and make this change a no-op by default (synchronous propagation) and make the added delay configurable. @pstibrany let me know if you have thoughts.

I'm fine with having it disabled by default.

One previously mentioned alternative would be to move this to memberlist KV store -- not only would it apply to all rings, but clients of memberlist KV store already expect updates to be delayed. WDYT?

[pstibrany]

Thank you, I'm fine with keeping this disabled by default. Once we

[seizethedave]

I vendored this change locally and it actually does break tests in Mimir, as ruler tests attempt to wait 100ms for ring changes to propagate. I may just bite the bullet and make this change a no-op by default (synchronous propagation) and make the added delay configurable. @pstibrany let me know if you have thoughts.

I'm fine with having it disabled by default.

One previously mentioned alternative would be to move this to memberlist KV store -- not only would it apply to all rings, but clients of memberlist KV store already expect updates to be delayed. WDYT?

I had taken a cursory look at this earlier and it looked like I've have to pepper a bunch of ad-hoc timer code into the bodies of WatchKey/Prefix. But today I gave it another look and realized it could cleanly go in the memberlist notification handling before the notifs make it to the Watch methods. I like this better! PTAL in your limited time. :)

[pstibrany]

Very nice! Thank you!