-
Notifications
You must be signed in to change notification settings - Fork 3
feat: update event processor to handle context key deduplication #150
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
feat: update event processor to handle context key deduplication #150
Conversation
|
This pull request has been linked to Shortcut Story #206551: Update EventProcessor for server SDK. |
| event.creation_date, | ||
| filter_.filter(event.context)}); | ||
| } | ||
| } |
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.
This is the actual change (1/2), not sure why the diff chunk is so large.
| if constexpr (std::is_same<SDK, | ||
| config::shared::ServerSDK>::value) { | ||
| context_key_cache_.Notice(event.context.CanonicalKey()); | ||
| } |
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.
Other change (2/2).
libs/internal/include/launchdarkly/events/context_key_cache.hpp
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,32 @@ | |||
| #include <launchdarkly/events/lru_cache.hpp> | |||
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.
Filename of this one is still context_key_cache.
But maybe if this cache is only for strings, then it doesn't need to be generic.
(Big segments, if we do it at some point has three states. Basically a tribool. Included in the segment, not included in the segment, or it doesn't exist. So when it isn't part of the segment we still cache that.)
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.
Uhg, sorry for the churn. Renamed to lru_cache and renamed the test file as well.
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.
No biggie.
Augments event processor with context-key deduplication abilities using an LRU cache.
Since the implementation is shared between server/client, I use
if constexpron the SDK template to enable the behavior only on server-side SDKs.Cache eviction behavior subject to ongoing discussion.