-
Notifications
You must be signed in to change notification settings - Fork 383
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
Fix issues with cache invalidation related to parallel queries. #6292
Conversation
Deployed docs
|
b5807be
to
44c686b
Compare
(promises_back.last().map(|(index, _)| index), indices.back()) | ||
{ | ||
if let (Some(p_index), Some(i_index)) = ( | ||
promises_back.first().map(|(index, _)| index), |
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.
Changing this this didn't catch any new sanity issues, but I think this was just a typo unless I'm misunderstanding the intent.
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.
Yep, nice catch
44c686b
to
ee90c86
Compare
0cb35f5
to
a8d374f
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.
very nice
What
Issue 1: data invalidation before data use
This failure mode happens where:
I worked around this by cloning the data and orphaning the Inner when we execute an invalidation. This feels a bit heavy-handed for the cases where we don't need it. Maybe there's an alternative way to keep the structure locked for read until the results have been consumed?
Issue 2: not considering pending data
The
time_range
logic wasn't factoring in the possibility of pending_data. This meant we could have pending gaps in queries that had not yet been resolved, but wouldn't count them as gaps. Subsequently as the promises are resolved we would still end up with gaps in the data.time_range()
has now been replaced withpending_time_range()
to do the proper book-keeping.Checklist
main
build: rerun.io/viewernightly
build: rerun.io/viewerTo run all checks from
main
, comment on the PR with@rerun-bot full-check
.