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

feat(ha_tracker): Replicadesc implement mergeable interface #10020

Conversation

NickAnge
Copy link
Contributor

@NickAnge NickAnge commented Nov 25, 2024

What this PR does

In this PR, we implement the Mergeable interface for ReplicaDesc and add tests to verify the implementation. Below, I will describe the high-level algorithm chosen for the Merge function.

It’s important to note that the Mergeable interface was created with the assumption that the objects implementing it would maintain a "view" (list) of items over which they would iterate. At least based on my understanding after implementing,. In this case, there is only one instance of ReplicaDesc for each key.

Merge

We can have 2 possible cases:

In the first case, the incoming Mergeable has the same Replica name as the one already in the KVStore. In this scenario, we keep the one with the most recent timestamp. If the timestamps are the same, we only update if the incoming one has been marked as deleted, in which case we need to make the KVStore aware of the deletion.

In the second case, the incoming Mergeable has a different Replica name. Here, we use the electedAt timestamp to decide which of the two ReplicaDesc objects should be retained.

Which issue(s) this PR fixes or relates to

Fixes #https://github.com/grafana/mimir-squad/issues/2653

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

@NickAnge NickAnge force-pushed the nickange/ha_tracker/support-replicadesc-mergeable-interface branch from 1bd6fab to 2789864 Compare November 25, 2024 14:19
@NickAnge NickAnge changed the title Nickange/ha tracker/support replicadesc mergeable interface feat(ha_tracker): Replicadesc implement mergeable interface Nov 25, 2024
@NickAnge NickAnge force-pushed the nickange/ha_tracker/support-replicadesc-mergeable-interface branch from e4ac231 to 116b7aa Compare November 27, 2024 14:46
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pretty solid 🚀 i left a few comments, but can't see anything major

@NickAnge NickAnge marked this pull request as ready for review December 2, 2024 09:11
@NickAnge NickAnge requested a review from a team as a code owner December 2, 2024 09:11
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the core change LGTM. I left a couple of small comments for the tests, can you take a look?

Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dimitarvdimitrov dimitarvdimitrov merged commit c87b63f into main Dec 2, 2024
29 checks passed
@dimitarvdimitrov dimitarvdimitrov deleted the nickange/ha_tracker/support-replicadesc-mergeable-interface branch December 2, 2024 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants