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

Add dynamodb multikey kv #5026

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
Collaborator

@danielblando danielblando commented Dec 7, 2022

Signed-off-by: Daniel Deluiggi ddeluigg@amazon.com

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 marked this pull request as ready for review December 7, 2022 18:34
pkg/ring/model.go Outdated Show resolved Hide resolved
pkg/ring/kv/dynamodb/client.go Outdated Show resolved Hide resolved
pkg/ring/kv/dynamodb/client.go Outdated Show resolved Hide resolved
pkg/ring/kv/dynamodb/client.go Outdated Show resolved Hide resolved
pkg/ring/kv/dynamodb/client.go Outdated Show resolved Hide resolved
pkg/ring/kv/dynamodb/dynamodb.go Outdated Show resolved Hide resolved
pkg/ring/kv/codec/codec.go Outdated Show resolved Hide resolved
pkg/ring/kv/codec/codec.go Outdated Show resolved Hide resolved
pkg/ring/kv/memberlist/memberlist_client_test.go Outdated Show resolved Hide resolved
pkg/ring/model.go Outdated Show resolved Hide resolved
pkg/ring/kv/dynamodb/client.go Outdated Show resolved Hide resolved
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
Collaborator 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
Collaborator

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
Collaborator 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

pkg/ring/kv/client.go Outdated Show resolved Hide resolved
@danielblando danielblando removed the request for review from alvinlin123 January 16, 2023 20:28
Signed-off-by: Daniel Deluiggi <ddeluigg@amazon.com>
Signed-off-by: Daniel Deluiggi <ddeluigg@amazon.com>
Signed-off-by: Daniel Deluiggi <ddeluigg@amazon.com>
pkg/ring/kv/dynamodb/dynamodb.go Outdated Show resolved Hide resolved
pkg/ring/kv/dynamodb/dynamodb.go Show resolved Hide resolved
Signed-off-by: Daniel Deluiggi <ddeluigg@amazon.com>
Signed-off-by: Daniel Deluiggi <ddeluigg@amazon.com>
Copy link
Collaborator

@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 <ddeluigg@amazon.com>
Copy link
Collaborator

@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 <ddeluigg@amazon.com>

* Reorganize import

Signed-off-by: Daniel Deluiggi <ddeluigg@amazon.com>

* Rework test

Signed-off-by: Daniel Deluiggi <ddeluigg@amazon.com>

* Change ddb CAS to use batch

Signed-off-by: Daniel Deluiggi <ddeluigg@amazon.com>

* Use slice to batch writeRequests

Signed-off-by: Daniel Deluiggi <ddeluigg@amazon.com>

* Change slice usage

Signed-off-by: Daniel Deluiggi <ddeluigg@amazon.com>

Signed-off-by: Daniel Deluiggi <ddeluigg@amazon.com>
Signed-off-by: Alex Le <leqiyue@amazon.com>
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.

None yet

5 participants