-
Notifications
You must be signed in to change notification settings - Fork 567
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
feat(ha_tracker): Replicadesc implement mergeable interface #10020
Conversation
1bd6fab
to
2789864
Compare
e4ac231
to
116b7aa
Compare
There was a problem hiding this 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
There was a problem hiding this 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What this PR does
In this PR, we implement the
Mergeable
interface forReplicaDesc
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 ofReplicaDesc
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
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]
.about-versioning.md
updated with experimental features.