feat: distinguish between read / write events (Cosmo Streams)#2304
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe PR introduces mutable event handling and collection-based APIs across the pub/sub system. Events are now wrapped via Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
There was a problem hiding this comment.
Nice work @alepane21 ! We could do some small tweaks here and there but I do like this idea. I added some suggestions on how we could streamline the interface even further
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
Summary by CodeRabbit
Checklist
Motivation
This pull is a draft to discuss a way of avoiding lots of allocations being done whenever we call the
OnReceiveEventshook. We do this to isolate event changes between subscribers on that hook, so a change in a hook from one subscriber does not affect the other. The downside is we need to allocate memory for every event for every subscriber. The question is: How many hooks will actually modify the event slice items? Because only then do we need a deep copy of them. If the hooks don't do that we do lots of allocation for nothing.This PR for now provides the code changes, without touching tests yet. This is only to show the idea and once thats settling, I will do the corrections and adjust tests.
The idea
Don't copy events before calling OnReceiveEvents hook. Instead make the user aware via type/method names, godoc and cosmo docs he needs to clone events when he wants to modify them to prevent data races. He can use the original events object to read events (and avoid allocations along the way) as long as he doesn't touch the actual event items themself.
The design
Thx to @alepane21 for this idea!
Instead of providing events as a slice to the hook, we provide a new type
datasource.StreamEventsto hooks:You cannot access the slice directly, because we hide the slice in a private struct field.
To access it you have two options:
events.All()allocation-free iterator without the option to override slice elements.
Good for getting read access to events.
To modify the events in a thread safe way, a hook user has to do this
Note
Working on getting this easier and more straight forward
events.UnsafeStreamEvents():There is a way to access the underlying slice directly for those who really want to.
For example they need indexable read access to the slice
The name implies that doing this is dangerous. It should lead to the user checking godocs and see that this can cause race conditions, if data is modified.