Added registered timestamp to instances in the ring #3248
Conversation
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
| return ringDesc, true, nil | ||
| } | ||
|
|
||
| // The instance already exists in the ring, so we can't change the registered timestamp (even if it's zero) |
There was a problem hiding this comment.
It may not be clear why we don't set "registered at" if it's zero. I would suggest extending description of this field in ring.proto to describe its intended usage, and explaining why preserving zero is important.
There was a problem hiding this comment.
I updated the command. I haven't specifically referenced the shuffle sharding because it's not a thing yet and this is a generic property, but I tried to make it clear to never change it once its set (even if it's 0).
WDYT?
There was a problem hiding this comment.
What I meant is to explain purpose of the field. Something like: This field is used to find out subset of instances that could have possibly owned the specific token in the past.
There was a problem hiding this comment.
What I meant is to explain purpose of the field. Something like: This field is used to find out subset of instances that could have possibly owned the specific token in the past.
And from there, explain why updating to non-zero value is a bad idea.
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
What this PR does:
I'm working on adding shuffle sharding support for ingesters on the read path. A preliminary step is to have the timestamp at which an instance registered to the ring, which is what I'm proposing in this PR.
The changeset is large because I had to add the registered timestamp as parameter of the
AddIngester()function, which is widely used in tests. A part from this, logic changes are pretty minimal.A part from unit tests, I've also run some manual tests both on ingesters ring (to test
Lifecycler) and store-gateways ring (to testBasicLifecycler):SIGKILL) an instance and then, after a while, restart it (the registered timestamp is preserved if the instance wasn't forgotten in the ring)Which issue(s) this PR fixes:
N/A
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]