Skip to content

feat: warn when multiple redis stream trim strategies are specified#4252

Merged
JoshVanL merged 3 commits intodapr:mainfrom
erj826:warn-multiple-redis-trim-strategies
Mar 6, 2026
Merged

feat: warn when multiple redis stream trim strategies are specified#4252
JoshVanL merged 3 commits intodapr:mainfrom
erj826:warn-multiple-redis-trim-strategies

Conversation

@erj826
Copy link
Copy Markdown
Contributor

@erj826 erj826 commented Mar 3, 2026

Description

This is a bug/feature that I encountered while working with Dapr's Redis pubsub component. Redis supports either MAXLEN or MINID for XADD and XTRIM

The go-redis client quietly chooses MAXLEN when both are specified:

Dapr let's me declare both in the pubsub.redis component

apiVersion: dapr.io/v1alpha1
kind: Component
metadata:
  name: pubsub
spec:
  type: pubsub.redis
  version: v1
  metadata:
    ...
    - name: maxLenApprox
      value: "100000"
    - name: streamTTL
      value: "10s"

and streamTTL is ignored. I think the warning log and/or a docs update would be helpful.

Issue reference

None -- can open if appropriate.

Checklist

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

Signed-off-by: Eric Jacobson <eric.jacobson@grafana.com>
@erj826 erj826 force-pushed the warn-multiple-redis-trim-strategies branch from 19ed97f to 58f8b40 Compare March 3, 2026 15:29
@erj826 erj826 changed the title feat: warn when multiple redis stream trim strategies are specified Feat: warn when multiple redis stream trim strategies are specified Mar 3, 2026
@erj826 erj826 changed the title Feat: warn when multiple redis stream trim strategies are specified feat: warn when multiple redis stream trim strategies are specified Mar 3, 2026
@erj826 erj826 marked this pull request as ready for review March 3, 2026 15:37
@erj826 erj826 requested review from a team as code owners March 3, 2026 15:37
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a warning and documentation updates to help operators avoid configuring mutually-exclusive Redis Streams trimming options (maxLenApprox vs streamTTL) in the Redis pubsub component, which can otherwise lead to one strategy being silently ignored depending on client behavior.

Changes:

  • Log a warning during component initialization when both maxLenApprox and streamTTL are set.
  • Update the Redis pubsub component metadata docs to state the two options are mutually exclusive (and reflow the maxLenApprox description formatting).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
pubsub/redis/redis.go Adds an init-time warning when both stream trimming strategies are configured.
pubsub/redis/metadata.yaml Documents the mutual exclusivity between maxLenApprox and streamTTL (plus minor formatting).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

@erj826
Copy link
Copy Markdown
Contributor Author

erj826 commented Mar 4, 2026

@JoshVanL Happy to make copilot's suggested changes if you think they're appropriate, but two thoughts

Consider updating the warning to clearly state which setting will be honored

Right now go-redis will fall back to MaxLenApprox, but it's just a switch statement on their side that could easily get refactored and change the default behavior. (See go-redis links in the PR description)

To make behavior deterministic and avoid depending on go-redis internals, consider enforcing a single strategy here (e.g., return an init error, or explicitly clear one setting).

I wanted to avoid introducing a potentially breaking change if users are unknowingly passing both arguments, and didn't want to refactor the logic entirely to create a single setting.

Let me know if this makes sense.

@JoshVanL
Copy link
Copy Markdown
Contributor

JoshVanL commented Mar 4, 2026

Hey @erj826! Totally agree with you :) we can resolve those. I've started auto requesting from CoPilot as a sanity check & second pair of "eyes".

@JoshVanL JoshVanL added this pull request to the merge queue Mar 6, 2026
Merged via the queue into dapr:main with commit 777f6d5 Mar 6, 2026
108 checks passed
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.

3 participants