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

[discussion] issue 975 example fix #976

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

luxe
Copy link
Collaborator

@luxe luxe commented Dec 22, 2021

No description provided.

@werkt
Copy link
Collaborator

werkt commented Dec 22, 2021

Definitely need some discussion on this. Can you frame the situation that you discovered to determine that the entry is actually missing on the filesystem?

The longer term presentation here is that we need to a) invalidate the entry if it no longer exists, b) invalidate the downstream directories that contain it, and c) switch to a future to ensure that we can block all concurrent requests for this truth on it, assuming we move ahead with this.

@luxe
Copy link
Collaborator Author

luxe commented Dec 22, 2021

This is for the discussion on #975. I don't think its a good solution, but worth @shirchen trying

@shirchen
Copy link
Contributor

shirchen commented Dec 22, 2021

thanks! yeah, this worked and fixed #975. since our deployment system is locked down till early january, i need to wait to deploy it there to test this out.

@@ -393,7 +397,7 @@ private boolean contains(
result.mergeFrom(digest).setSizeBytes(entry.size);
}
onContains.accept(key);
return true;
return blobExistsOnLocalStorage(key);
Copy link
Collaborator

Choose a reason for hiding this comment

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

To prevent the extra overhead, can we only perform the check if ensure outputs present is set?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's a good idea. I'll adjust for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants