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 timeout for dynamodb ring kv #6544

Merged
merged 4 commits into from
Jan 28, 2025

Conversation

yeya24
Copy link
Contributor

@yeya24 yeya24 commented Jan 24, 2025

What this PR does:

This PR tries to fix #6211 using a different approach of #6212.

#6212 added context timeout to heartbeat but there are more places missing context timeout.

This PR instead tries to enforce context timeout on DDB KV so that request to DDB KV doesn't hang forever.

Which issue(s) this PR fixes:
Fixes #

Checklist

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

@danielblando
Copy link
Contributor

danielblando commented Jan 24, 2025

Shouldnt we add to RegisterFlags with a default? We probably want to add a default as we noticed issues when not having a timeout

@yeya24 yeya24 force-pushed the dynamodbkv-timeout branch from d1d0fac to ced8a5c Compare January 28, 2025 07:42
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jan 28, 2025
Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: Ben Ye <[email protected]>
@yeya24 yeya24 marked this pull request as ready for review January 28, 2025 17:59
Copy link
Contributor

@danielblando danielblando left a comment

Choose a reason for hiding this comment

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

LGTM

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jan 28, 2025
@yeya24 yeya24 merged commit b4953a3 into cortexproject:master Jan 28, 2025
17 checks passed
@yeya24 yeya24 deleted the dynamodbkv-timeout branch January 28, 2025 23:30
alexqyle pushed a commit to alexqyle/cortex that referenced this pull request Jan 31, 2025
* add dynamodb kv with timeout enforced

Signed-off-by: yeya24 <[email protected]>

* add tests

Signed-off-by: yeya24 <[email protected]>

* docs

Signed-off-by: Ben Ye <[email protected]>

* update changelog

Signed-off-by: Ben Ye <[email protected]>

---------

Signed-off-by: yeya24 <[email protected]>
Signed-off-by: Ben Ye <[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
backend/dynamodb lgtm This PR has been approved by a maintainer size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ingester stops Ring CAS when lifecycler hangs on query to KV store
3 participants