I'm not sure this is relevant to us. In classic, we have this check to not use object_contexts on non primaries. Nevertheless, we would still fallback to "loading" this object.

it looks like a memory-saving measure to me, still relevant here - replicas don't usually cache object contexts, so we're bypassing the obc cache on them.

In classic at least, the primary normally processes reads and the obc-loading part of a write, and the secondaries just get the resulting transaction and log to apply, so the replicas don't waste cache on an obc they won't use. Is crimson structured the same way?

replicas don't usually cache object contexts, so we're bypassing the obc cache on them.

That seems to be the original intent of this if-case, thank you for looking into this.
While the logic remains true for Crimson, the issue here is that using the cache directly (without the obc manager) would expose this obc in undefined state - e.g not loaded yet.

I think that we should still omit this bit in Crimson and prevent replicas from caching obcs from within the obc_loader. This will allow us to eventually prevent direct cache usage (one user left after this PR) and we could prevent the redundant caching later on.
What do you think?

btw, won't secondaries obc caching benefit to replicated reads cases?

That sounds like a good approach, better to encapsulate the loading logic as you suggest. Localized reads (reading from secondary) could potentially benefit from the cache, but this is not the common case, and I'd guess this code in classic predated localized reads.

I'm not sure it affects performance much, but it's possible the obc cache should not be used by backfill/scrub at all, since they aren't indicative of future use of those objects by clients.