-
Notifications
You must be signed in to change notification settings - Fork 91
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
Add replication for missing blobs in readFallbackBlobAccess #174
Conversation
The changes are very similar to mirroredBlobAccess
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.
Hey @annapst,
This change looks pretty good. Thanks for working on this. Be sure to process the comments I've left behind, and I'll merge this in a timely manner.
@@ -90,5 +91,23 @@ func (ba *readFallbackBlobAccess) FindMissing(ctx context.Context, digests diges | |||
if err != nil { | |||
return digest.EmptySet, util.StatusWrap(err, "Secondary") | |||
} | |||
|
|||
// Replicate the blobs that are present only in the secondary backend to primary | |||
if ba.replicator != nil { |
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.
This if-statement can be removed. The Buildbarn codebase tries to follow the null object pattern where possible. So if you specify a 'noop' blob replicator, it's actually an instance of this type:
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've added this check because of the description of a previous pull request touching this file, mentioning backwards compatibility (47ea975) and because there is a nil check in other places in this file.
Is this still safe to remove with this info?
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.
It's still safe to remove it. What's in the commit message likely applied to the initial version of the PR. See this comment in the PR, where I requested the same thing:
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.
Thank you for clarifying! Let me update!
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.
LGTM! Thanks! Will merge once the CI run completes and passes.
The changes are very similar to mirroredBlobAccess