Proposal for ring multikey #4832
Conversation
danielblando
force-pushed
the
proposal-ring-multikey
branch
2 times, most recently
from
1f79ffa to
4f13780
Compare
Signed-off-by: Daniel Deluiggi <[email protected]>
danielblando
force-pushed
the
proposal-ring-multikey
branch
from
4f13780 to
ddfec3d
Compare
|
Thanks @friedrichg for taking the time. I got some ideas from how memberlist works mixing the code with ring using the Mergeable interface. Agree with you about the experiment. I actually did a second POC changing the ring to support multikey instead of putting the logic mostly on the KV. The idea was adding a new Codec to the ring which would run using multikey. The problem encountered was that we dont have a proper migration path from singlekey to multikey on that approach. We would need to create a multi interface similar to KV to use both rings for a while. I preferred proposing a simple approach which does not block Cortex to change the ring in the future. |
|
I think this approach make sense in order to prevent a more disruptive change that will require a painful migration. |
| The proposal is to create an interface called MultiKey. The interface allows KV store to request the codec to split and | ||
| join the values is separated keys. | ||
|
|
||
| ``` | ||
| type MultiKey interface { | ||
| SplitById() map[string]interface{} | ||
|
|
||
| JoinIds(map[string]interface{}) Multikey | ||
|
|
||
| GetChildFactory() proto.Message | ||
|
|
||
| FindDifference(MultiKey) (Multikey, []string, error) | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Have you considered reusing existing code to deal with this? I am thinking about Client.List to get all keys under a prefix, Client.WatchPrefix to watch for updates under given prefix, and Desc.Merge (in ring) for merging multiple keys into final ring.
That way the ring itself would decide to use single key or multiple keys, and would handle merging them together.
There was a problem hiding this comment.
I did a similar approach to what you mentioned. I changed the ring to support multiples keys. My concern was how we migrate online application. We would need to create a similar solution as multi client from kv store on the ring level. I was thinking that the ring would use single or multi keys operations depending on configuration. I think it goes back to my answer to Friedrich suggesting that this would be a first step as it does not change any existent code and we can change the ring as a long term approach.
| decode the data stored in the kv store. | ||
|
|
||
| Example of CAS being used with multikey design: | ||
|  |
There was a problem hiding this comment.
The ingesters generate hash tokens locally, how do we resolve the token conflicts?
There was a problem hiding this comment.
@damnever that is a good point that i should have touched more in the proposal. The idea is to have a similar approach to memberlist design.
The memberlist code resolves conflict is in the merge implementation of mergeable interface. The idea is to use a very similar code (maybe the same) of resolveConflict in the WatchKey for a new KV.
We still have the 'observe_period' on ring to verify any conflicts.
Line 68 in 48bc900
Signed-off-by: Daniel Deluiggi [email protected]
What this PR does:
Add new proposal for ring multikey
Which issue(s) this PR fixes:
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]