Skip to content
This repository has been archived by the owner on Aug 28, 2020. It is now read-only.

Add Completeness Checking Fetcher #15

Closed
wants to merge 6 commits into from

Conversation

Qinusty
Copy link
Owner

@Qinusty Qinusty commented Aug 13, 2020

Initial completeness checking implementation for the fetch service.

To check:

  • Should the response be NotFound on a failed completeness check?
    • Spec mentions ABORTED for failed consistency checks
  • We need to store referenced blobs and check these during a fetch (maybe also at push time although I am unsure if this is required)

Addresses #4

@Qinusty Qinusty requested a review from tomcoldrick-ct August 13, 2020 18:19
@tomcoldrick-ct
Copy link
Collaborator

Should the response be NotFound on a failed completeness check?

The spec seems to tell us to use ABORTED, but I'm not sure if clients will handle this gracefully right away... I'd say it's best to cautiously use ABORTED and patch clients if we have problems?

We need to store referenced blobs and check these during a fetch

Yup, that's just a matter of updating the Asset proto to have fields for this and checking them too in the CompletenessCheckingFetcher.

(maybe also at push time although I am unsure if this is required)

If this is the case we would also want to check the original blob on push too. From a purely "KISS" perspective, I think it's best to trust clients to not push references to things which aren't in CAS, given this should be checked at Fetch time with the completeness checker.

@Qinusty Qinusty force-pushed the qinusty/completenessChecking branch from c90b86a to e5674e3 Compare August 17, 2020 10:46
@Qinusty Qinusty force-pushed the qinusty/completenessChecking branch from e5674e3 to 8269ef9 Compare August 17, 2020 11:05
@Qinusty
Copy link
Owner Author

Qinusty commented Aug 25, 2020

Given recent discussions around the lack of GetTree in Buildbarn, this PR needs further thought to ensure that FetchDirectory can work efficiently with REv2/RAv1.

@Qinusty Qinusty closed this Aug 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants