Skip to content

Add dynamodb multikey kv #5026

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

Merged
merged 6 commits into from
Jan 24, 2023
Merged

Add dynamodb multikey kv #5026

merged 6 commits into from
Jan 24, 2023

Conversation

danielblando
Copy link
Contributor

@danielblando danielblando commented Dec 7, 2022

Signed-off-by: Daniel Deluiggi [email protected]

What this PR does:
This CR introduce the idea of multikey for kv on the ring. The proposal was made at #4832
It also add a new kv store using dynamodb which uses the multikey implementation. The new kv split the ring by InstanceDesc making each instance be a key in the ring. The new kv also introduce the concept of stale data and ttl.
TTL is an optional time to live of any data not update for x amount of time. This helps cleaning the ring when a pod leaves without gracefully shutdown.
Stale data is used when we have a problem communicating to the kv store, in this case dynamodb. The stale is returned and we use the last time we could sync the data to check if a instance is unhealthy or not.

Checklist

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

@danielblando danielblando force-pushed the ddb-kv branch 2 times, most recently from 7261cbe to 5be318f Compare December 7, 2022 17:21
@danielblando danielblando marked this pull request as ready for review December 7, 2022 18:34
Copy link
Member

@friedrichg friedrichg left a comment

Choose a reason for hiding this comment

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

Do you know if there is an increase in kv network usage after this?

@danielblando
Copy link
Contributor Author

@friedrichg
The network usage really depends on the configuration, mostly for heartbeat period.
I did some comparison with memberlist using the same values and there is a decrease in network usage. This is expected as we don't need to gossip the information around to update all nodes.
I did not compare it to etcd or consul but there should not be any difference on network usage as they use the same principle.

level.Error(c.logger).Log("msg", "error CASing", "key", key, "err", err)
continue
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a little bit worried about the multiple put and delete calls to DDB here. Can we use https://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_BatchWriteItem.html API here to reduce this to only 1 API call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that makes sense. I am not sure why i didnt see this api from the beginning. I will work on changing this. Thanks

@danielblando danielblando force-pushed the ddb-kv branch 2 times, most recently from 690176d to f7d4550 Compare January 12, 2023 23:26
@danielblando danielblando force-pushed the ddb-kv branch 2 times, most recently from 01f29f4 to 256a6cc Compare January 13, 2023 00:34
@danielblando danielblando removed the request for review from alvinlin123 January 16, 2023 20:28
Signed-off-by: Daniel Deluiggi <[email protected]>
Signed-off-by: Daniel Deluiggi <[email protected]>
Signed-off-by: Daniel Deluiggi <[email protected]>
@danielblando danielblando force-pushed the ddb-kv branch 3 times, most recently from 77bae08 to 79be362 Compare January 24, 2023 00:14
Signed-off-by: Daniel Deluiggi <[email protected]>
@danielblando danielblando force-pushed the ddb-kv branch 2 times, most recently from 2e5e480 to 96d2448 Compare January 24, 2023 08:16
Signed-off-by: Daniel Deluiggi <[email protected]>
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks @danielblando. Great work!

Signed-off-by: Daniel Deluiggi <[email protected]>
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks!

@yeya24 yeya24 enabled auto-merge (squash) January 24, 2023 18:40
@yeya24 yeya24 merged commit 4510e11 into cortexproject:master Jan 24, 2023
alexqyle pushed a commit to alexqyle/cortex that referenced this pull request May 2, 2023
* Add dynamodb multikey kv

Signed-off-by: Daniel Deluiggi <[email protected]>

* Reorganize import

Signed-off-by: Daniel Deluiggi <[email protected]>

* Rework test

Signed-off-by: Daniel Deluiggi <[email protected]>

* Change ddb CAS to use batch

Signed-off-by: Daniel Deluiggi <[email protected]>

* Use slice to batch writeRequests

Signed-off-by: Daniel Deluiggi <[email protected]>

* Change slice usage

Signed-off-by: Daniel Deluiggi <[email protected]>

Signed-off-by: Daniel Deluiggi <[email protected]>
Signed-off-by: Alex Le <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants