Skip to content

feat(readfallback): allow disabling FindMissing replication - #386

Merged
EdSchouten merged 3 commits into
buildbarn:mainfrom
joeljeske:joeljeske/readfallback-disable-findmissing-replication
Oct 9, 2026
Merged

EdSchouten merged 3 commits into
buildbarn:mainfrom
joeljeske:joeljeske/readfallback-disable-findmissing-replication

Conversation

@joeljeske

@joeljeske joeljeske commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Add disableFindMissingReplication to the shared readFallback configuration. When true, FindMissingBlobs() still checks the primary backend and then queries the secondary for digests missing from the primary, but does not replicate blobs found only in the secondary. Replication during Get() and GetFromComposite() is unchanged.

This lets existence checks avoid pulling blobs into the primary before they are actually read. The option is available wherever a readFallback backend is configured, including nested backends.

NewReadFallbackBlobAccess() now takes a pair of replicators: one for Get()/GetFromComposite() and one for FindMissing(). The configuration layer passes the same replicator instance for both by default, so concurrency limits and deduplication remain shared. When disableFindMissingReplication is set, it passes a no-op replicator for FindMissing().

Compatibility

  • Use an ordinary proto3 bool, matching existing configuration fields such as disable_http2; presence tracking is unnecessary.
  • Omitted and explicit false values retain the existing replication behavior; existing configuration files need no changes.
  • NewReadFallbackBlobAccess() gains a second replicator argument. All in-repository callers are updated.
  • Regenerate the Go protobuf bindings using the repository's Bazel toolchain.

Testing

  • bazel test //pkg/blobstore/... — all 12 test targets passed.
  • Mock tests cover the existing behavior with a shared replicator, and that Get(), GetFromComposite() and FindMissing() each use their own replicator when they differ.
  • bazel run //:gazelle — no BUILD changes.

Generated with Toshi

@joeljeske
joeljeske force-pushed the joeljeske/readfallback-disable-findmissing-replication branch from 083611a to bca7048 Compare October 7, 2026 17:37
@@ -650,6 +650,15 @@ message ReadFallbackBlobAccessConfiguration {
// the secondary backend to the primary backend. If unset, objects
// will not be copied.
BlobReplicatorConfiguration replicator = 3;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not change this field into two separate ones:

  • get_replicator
  • find_missing_replicator

And then you can set the latter to { noop: {} }.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hey @EdSchouten I can make that change but I didn't for 2 reasons:

  1. Users that want to replicate both (like today), will now need to configure 2 replicators, which means it will duplicate the concurrency limits between the replicators , and it will prevent deduplication across the replicators from functioning. For some setups using a remote replicator, this may be fine, but others using a local replicator (e.g. with bb_clientd) it can cause duplicate replication.
  2. Its not backwards compatible, and users must update their configuration

What do you think? Can we keep it this way, or would you prefer to have separate replicators despite the potential performance impact?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fair point. Then let's aim for a solution in between:

  • Let the BlobAccess take a pair of replicators.
  • Let the configuration files use the structure as is.

Let ReadFallbackBlobAccess take a pair of replicators instead of a
bool. The configuration is unchanged: by default the same replicator
is used for both, so concurrency limits and deduplication remain
shared. When disable_find_missing_replication is set, a no-op
replicator is used for FindMissing().
@EdSchouten

Copy link
Copy Markdown
Member

LGTM! Will merge once I've confirmed CI is happy.

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.

2 participants