Skip to content

Conversation

@elena-kolevska
Copy link
Contributor

@elena-kolevska elena-kolevska commented Mar 19, 2025

Description

Adding the option to trim Redis streams based on timestamp. When minIDApprox is specified in the component file entries with IDs smaller than the specified threshold are evicted, keeping the stream trimmed to only contain entries with IDs greater than or equal to this value.
This can be used for time-based eviction since Redis stream IDs are in the format {timestamp-sequence}, where timestamp is in milliseconds since epoch.

Issue reference

#3664

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Signed-off-by: Elena Kolevska <elena@kolevska.com>
yaron2
yaron2 previously approved these changes Apr 24, 2025
Copy link
Member

@yaron2 yaron2 left a comment

Choose a reason for hiding this comment

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

LGTM

@JoshVanL
Copy link
Contributor

We should use a TTL time duration value as part of the component field, not a timestamp- the timestamp should be computed at runtime.

Component field values should be evergreen.

@elena-kolevska
Copy link
Contributor Author

We should use a TTL time duration value as part of the component field, not a timestamp- the timestamp should be computed at runtime.

Absolutely! That was my initial idea when i started doing this, I don't remember how it changed by the time I implemented it. I was just looking into it last week. I'll update the pr.

Signed-off-by: Elena Kolevska <elena@kolevska.com>
@elena-kolevska
Copy link
Contributor Author

Updated

Comment on lines +135 to +137
if s.StreamTTL == 0 {
return ""
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if s.StreamTTL == 0 {
return ""
}
if s.StreamTTL <= 0 {
return ""
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we shouldn't fail silently, and it would be too late to return the error here (we use this on message send). We should probably catch it on init.

@yaron2 yaron2 merged commit 5f17025 into dapr:main May 6, 2025
90 checks passed
jjcollinge pushed a commit to jjcollinge/components-contrib that referenced this pull request May 12, 2025
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Co-authored-by: Cassie Coyle <cassie@diagrid.io>
Co-authored-by: Nelson Parente <nelson_parente@live.com.pt>
Co-authored-by: Yaron Schneider <schneider.yaron@live.com>
Co-authored-by: Bernd Verst <github@bernd.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants