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 missing blobs request/response #31

Merged
merged 7 commits into from
Nov 11, 2023
Merged

Fix missing blobs request/response #31

merged 7 commits into from
Nov 11, 2023

Conversation

saquibmian
Copy link
Contributor

Blobs are not globally addressable across the BSR, largely for SOC2 compliance so that we can safely and cheaply delete all assets for a repository. Blobs are scoped to a repository.

Therefore, when we query for missing blobs, we have to query for missing blobs in the context of a ModuleRef. I chose to take the route of querying for multiple sets of module blobs in a single request rather than one request per set of module blobs.

Blobs are not globally addressable across the BSR, largely for SOC2 compliance so that we can safely and cheaply delete all assets for a repository. Blobs are scoped to a repository.

Therefore, when we query for missing blobs, we have to query for missing blobs in the context of a `ModuleRef`. I chose to take the route of querying for multiple sets of module blobs in a single request rather than one request per set of module blobs.
(buf.validate.field).repeated.min_items = 1,
(buf.validate.field).repeated.max_items = 250
];
}

message GetMissingBlobDigestsResponse {
message MissingBlobDigest {
// The reference to the Module that the missing digests are for.
ModuleRef module = 1;
Copy link
Contributor Author

@saquibmian saquibmian Nov 9, 2023

Choose a reason for hiding this comment

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

@bufdev what type would I use here? ModuleRef is marked as request_only.

Is this canonical?

  string module_id = 6 [
    (buf.validate.field).required = true,
    (buf.validate.field).string.uuid = true
  ];

If so, that's incongruent with ModuleRef, which may not be an ID. Ideally the caller doesn't have to make other calls to fetch IDs for the modules (if that's the case, why take in ModuleRef at all).

Copy link
Member

Choose a reason for hiding this comment

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

It should be Module module = 1 on the response. Good catch.

Copy link
Member

Choose a reason for hiding this comment

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

and required

(buf.validate.field).repeated.min_items = 1,
(buf.validate.field).repeated.max_items = 250
];
}

message GetMissingBlobDigestsResponse {
message MissingBlobDigest {
// The Module that the missing digests are for.
Module module = 1 [(buf.validate.field).required = true];
Copy link
Member

Choose a reason for hiding this comment

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

Quite a lot of info just to say "is this the module you requested?", but ok.

(buf.validate.field).repeated.min_items = 1,
(buf.validate.field).repeated.max_items = 250
];
}

message GetMissingBlobDigestsResponse {
message MissingBlobDigest {
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a single digest, this is a set of Digests, so this name doesn't make as much sense. I would want to call it ModuleDigests, but then the field repeated ModuleDigests module_digests doesn't really make sense, it's more like repeated ModuleDigests module_digests_es. Plurals for message names rarely make sense.

Perhaps something like ModuleBlobDigestSet, and then repeated ModuleBlobDigestSet module_blob_digest_sets, or just Value and repeated Value values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@saquibmian saquibmian requested a review from bufdev November 10, 2023 22:26
@bufdev
Copy link
Member

bufdev commented Nov 11, 2023

GetBlobs also needed fixing, this is now done.

@bufdev bufdev merged commit e74415b into main Nov 11, 2023
5 checks passed
@bufdev bufdev deleted the smian/digests-per-module branch November 11, 2023 21:19
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.

3 participants