Skip to content
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: SnapshotProvider edge case scenarios on insertion and query #6707

Merged
merged 7 commits into from
Feb 21, 2024

Conversation

joshieDo
Copy link
Collaborator

This PR fixes two edge case scenarios.

get_or_create_jar_provider: change from .insert to entry api. Although not encountered, it is possible that two parallel threads reach the self.map.insert almost at the same time. The first one holding the lock would insert a LoadedJar. The second one once unblocked, would do the same thing invalidating the previous LoadedJar and potentially affecting the first threads task.

fetch_range_with_predicate: on a normal node behaviour it shouldn't happen, but when testing stuff out we have encountered this.

@joshieDo joshieDo added C-bug An unexpected or incorrect behavior A-static-files Related to static files labels Feb 21, 2024
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just one q re var location

result.push(res);
break 'inner
}
None => {
if retrying {
let err = if segment.is_headers() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a warn! here?

Copy link
Collaborator Author

@joshieDo joshieDo Feb 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can, but this is not an actual malfunction, which I kinda associate with a warning.

if the range spans across multiple files, there will be a time when the number no longer belongs to the current file (and thus returning None on the get_fn), and we need to find the associated file provider (get_provider).

With that in mind, do you still want me to add a warn!? Maybe a trace! would be a better fit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh wait you mean inside the if retrying, yeah okay adding

crates/storage/provider/src/providers/snapshot/manager.rs Outdated Show resolved Hide resolved
@joshieDo joshieDo merged commit 56ea6d5 into feat/static-files Feb 21, 2024
21 of 25 checks passed
@joshieDo joshieDo deleted the joshie/manager-issues branch February 21, 2024 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-static-files Related to static files C-bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants