-
Notifications
You must be signed in to change notification settings - Fork 54
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
chore(rest): improved message cache logic #2221
Conversation
You can find the image built from this PR at
Built from d3d4bf1 |
0c20ae1
to
e778198
Compare
e778198
to
b75945b
Compare
f138828
to
2acc66b
Compare
d58821a
to
99da23b
Compare
test fixes Fix legacy handler Fixes and tests
dbf6038
to
1642958
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM. Since the cache here is generally very small, I would have imagined that a simple, less efficient structure (e.g. just iterating through the cache to get and remove messages based on a filter) would be acceptable, but there may be some complexity that I'm missing.
I discussed the impl. at length with @gabrielmer and yes, not my best work. It's done, it works, I don't want to spend even more time on this. |
Fair enough 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks so much! 🤩
Description
I refactored the message cache with the intent to allow fetching messages by pubsub OR content topic. Adding a message now add the missing topic either pubsub or content (still need to be subscribed to either). I also added a check to prevent duplicate messages.
The capacity is now TOTAL instead of PER topic.
Changes
Tracking: #2201
Fix: #2192