-
Notifications
You must be signed in to change notification settings - Fork 269
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(event cache): redact previously-seen events in the linked chunk too #4536
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4536 +/- ##
==========================================
- Coverage 85.43% 85.39% -0.05%
==========================================
Files 285 285
Lines 32112 32205 +93
==========================================
+ Hits 27435 27501 +66
- Misses 4677 4704 +27 ☔ View full report in Codecov by Sentry. |
VectorDiff::Clear => accumulator.clear(), | ||
diff => unimplemented!("{diff:?}"), | ||
} | ||
diff.apply(accumulator); |
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.
👍
if let Some(updates) = self.updates.as_mut() { | ||
updates.push(Update::ReplaceItem { | ||
at: Position(chunk_identifier, item_index), | ||
item: item.clone(), | ||
}); | ||
} |
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.
Push the update after current_items[item_index] = item;
: do the action before triggering the update.
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 means we have to clone before, and the original thing is unused if self.updates.as_mut().is_none()
. Plus, the updates aren't reflected in real-time, they're buffered until they're consumed, as far as I can tell? So this should be fine to keep as is. I can add a comment explaining that this avoids a clone if you'd like.
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.
Looks good. I'm not hugely confident I understand all the implications, but the code looks good. Some small suggestions and questions, nothing blocking.
4c08194
to
c54935d
Compare
…item from a given position
c54935d
to
17db2fb
Compare
This makes it possible to:
We could add a migration to apply redactions for existing users of the nightly cache. I'd rather do this as a followup, since this PR is growing a bit large already (and another possibility is also to clear all events stored in the event cache, as a dirty yet safe migration — we might need this because I spotted an opportunity to not persist unneeded data from storage).