Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor PeerProvider & hashring interaction #6296

Merged
merged 2 commits into from
Sep 27, 2024

Commits on Sep 27, 2024

  1. Refactor PeerProvider & hashring interaction

    As the result:
     - hashring never misses updated (no "channel is full" situation possible)
     - subscribers of MultiringResolver (history, matching) will always get
       sane ChangedEvent which reflects the REAL changes (see details below)
    
    Previously there were problems:
     - hashring subscribed to PeerProvider (ringpop/uns) with non-buffered channel
       which led to failures to write every time `ring` was doing something
       else than reading the channel (happened 60% of times based on error-logs).
       Switched to calling handlers instead which are implementing
       schedule-update with channel with cap=1 approach (see `signalSelf`).
       This approach never skips updates.
     - PeerProvider supplies ChangedEvent to `ring`, but in reality, we do
       not use it - we refresh everything from scratch. This makes very
       misleading to even rely on the ChangedEvent. Basically, we might be
       triggered by some event (host "c" appeared), but during refresh() we
       realise there are more changes (host "a" removed, host "c" added as
       well, etc.), and we notify our Subscribers with an absolutely
       irrelevant data.
     - Same misleading took place in other methods like
       `emitHashIdentifier`. It retrieved list of members from PeerProvider
       **independantly**, which could lead to emitting hash of a different
       state than members we just retrieved in refresh().
     - Some tests were working "by mistake": like
       `TestFailedLookupWillAskProvider` and
       `TestRefreshUpdatesRingOnlyWhenRingHasChanged`.
    
    All in all, not methods are more synchronised, called more expectedly
    (`compareMembers` should not make a new map), and notifiying subscribers
    is **inseparable** from ring::refresh() like it should be.
    dkrotx committed Sep 27, 2024
    Configuration menu
    Copy the full SHA
    8b49cc2 View commit details
    Browse the repository at this point in the history
  2. Adding more tests to ringpop provider

    Also fixed race-condition: we were waiting for ring to stop, but in
    order to read stop-channel it sometimes had to finish notifying
    subscribers which took the same lock. We need to be careful with
    lock-scope.
    dkrotx committed Sep 27, 2024
    Configuration menu
    Copy the full SHA
    8d31c74 View commit details
    Browse the repository at this point in the history