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

streaming ontop redis effect #985

Merged
merged 11 commits into from
Mar 27, 2025
Merged

Conversation

mmienko
Copy link
Collaborator

@mmienko mmienko commented Mar 7, 2025

Use stream commands from the main redis-effects module in the FS2 Stream wrapper module. Update read method to be DRY. Attempt to clarify some internal methods. Follow up to #969.

Could review be commit.

  • Use core streams api and data structures in FS2Streaming module
  • Refactor streaming read
  • Rename file to match class
  • Update test
  • Update docs and demo
  • Dry up the stream read api

This PR introduces breaking changes. Users will need to update imports, update read calls, and update XAddMessage to pass XAddArgs.
However, in the long term these should be beneficial.

We could consider an intermediate PR to introduce some of these changes in backwards compatible way and to mark old methods as depreciated.

If we decide to go forward, would be merge this into the 1.x.x series?

@mmienko mmienko requested review from arturaz, gvolpe and yisraelU March 7, 2025 19:34
@arturaz
Copy link
Collaborator

arturaz commented Mar 14, 2025

One note: chunkSize and count seems redundant? It was the same previously, but now I noticed it. It seems that we read all or "count" elements from redis and then just chunk it from memory. That feels... pointless? Should we just remove chunkSize parameter?

@mmienko
Copy link
Collaborator Author

mmienko commented Mar 17, 2025

Should we just remove chunkSize parameter?

Good callout, it seems redundant as it is all in-memory at this point so they could be equal. Will remove.

@arturaz arturaz merged commit c9bea1b into series/1.x Mar 27, 2025
2 checks passed
@arturaz arturaz deleted the streaming-ontop-redis-effect branch March 27, 2025 08:00
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