-
Notifications
You must be signed in to change notification settings - Fork 79
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
Better WFT buffer #636
Better WFT buffer #636
Conversation
// We accept that there might be tasks remaining in the buffer if we evicted and | ||
// re-instantiated here. It's likely those tasks are now invalidated anyway. |
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.
I could theoretically carry over any remaining buffered tasks to the next instantiation, but it adds quite a lot of complexity for little benefit IMO.
/// * Lang takes too long to complete a task and the task times out | ||
/// * Many queries are submitted concurrently and reach this worker (in this case, multiple | ||
/// tasks can be outstanding) | ||
/// * Multiple speculative tasks (ex: for updates) may also exist at once |
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.
Might be worth clarifying this statement that if multiple are sent only the latest is valid
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.
Will add
self.wft | ||
.take() | ||
.or_else(|| self.query_only_tasks.pop_front()) | ||
} |
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.
A comment about the ordering preference for WFT ahead of Query-only tasks would be good.
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.
Will add, and this reminds me I meant to test that we won't reply to queries that are "old" if we processed a newer WFT than they targeted.
20dd01d
to
ebb8454
Compare
2d3c323
to
5bcea37
Compare
There's some issue here causing intermittent problems with a test doing slow completions on a small cache. Need to figure that out. |
1864bcf
to
1a23583
Compare
What was changed
Better task buffering.
Why?
To avoid timing out concurrent queries. There is no reason to buffer more than one WFT, speculative or not, for the reasons described in the second issue. We already definitely will never try to apply more than one WFT at a time if the split brain edge case mentioned there happens.
Checklist
Closes [Feature Request] Quickly fail query-only tasks which would overflow task buffer #578 and closes [Feature Request] Quickly fail query-only tasks which would overflow task buffer #578
How was this tested:
New tests
Any docs updates needed?