You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "tonghuaroot (via GitHub)" <gi...@apache.org> on 2026/06/10 05:42:34 UTC
[PR] fix(layers/foyer): preserve read args while filling full-object cache [opendal]
tonghuaroot opened a new pull request, #7718:
URL: https://github.com/apache/opendal/pull/7718
# Which issue does this PR close?
Closes #7686.
# Rationale for this change
`FullReader::read_full_object` filled the full-object cache with `stat(OpStat::default())` and `read(OpRead::default())`, dropping the reader's `OpRead`. A versioned read then fetched and cached latest content under a versioned key, and conditional options (`if_match`, `if_none_match`, ...) were bypassed on the cache-fill path. The fallback paths already use `self.args.clone()`; the cache-fill path now matches them.
# What changes are included in this PR?
- Cache-fill `read` now uses the reader's `OpRead` instead of the default.
- Cache-fill `stat` derives its conditions (version, if-match, if-none-match, if-modified-since, if-unmodified-since) from the reader's `OpRead`.
- Added `test_cache_fill_preserves_read_args`, which fails on the old default-args path and verifies the inner accessor sees version and `if_match`.
# Are there any user-facing changes?
No public API change. The full-object cache now honors versioned and conditional read semantics on cache miss.
# AI Usage Statement
Implemented with assistance from Claude (Anthropic). All changes were reviewed and tested locally (`cargo test`, `cargo clippy -- -D warnings`, `cargo fmt --check` on `opendal-layer-foyer`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(layers/foyer): preserve read args while filling full-object cache [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on PR #7718:
URL: https://github.com/apache/opendal/pull/7718#issuecomment-4825464021
I read Xuanwo's expected behavior: Conditional reads such as if_match / if_none_match should either be respected during cache fill or explicitly bypass the cache after validating the condition.
This PR passes through if_match and if_none_match to a service without checking the headers. e.g.,
when fetching a s3 object with foyer:
1. reads the object without a if_none_match, triggers a cache.
2. reads the object again with if_none_match, will use the cache without the flag.
So this PR doesn't address this behavior. If we were to proceed with this PR, I will mark the PR as partially resolving the issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(layers/foyer): preserve read args while filling full-object cache [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan merged PR #7718:
URL: https://github.com/apache/opendal/pull/7718
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(layers/foyer): preserve read args while filling full-object cache [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #7718:
URL: https://github.com/apache/opendal/pull/7718#discussion_r3487595618
##########
core/layers/foyer/src/full.rs:
##########
@@ -81,6 +81,28 @@ impl<A: Access> FullReader<A> {
.contains(key)
}
+ /// Derive the `OpStat` used for cache fill from the reader's `OpRead`, so the
Review Comment:
Nice!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(layers/foyer): preserve read args while filling full-object cache [opendal]
Posted by "tonghuaroot (via GitHub)" <gi...@apache.org>.
tonghuaroot commented on PR #7718:
URL: https://github.com/apache/opendal/pull/7718#issuecomment-4826538689
Thanks @erickguan! Merge conflicts are resolved (merged main in and re-applied the change on top of the read-range / ReadStream refactors).
On the conditional-read point: you are right, this PR only aligns the cache-fill (miss) path so that a miss reaches the service with the request's version and conditions. On a cache hit it still returns the cached bytes without re-evaluating `if_match` / `if_none_match`, which is the gap you described.
Worth separating the two cases: `version` is effectively a cache-key selector (each version is immutable, so caching per version is correct, and the key already includes it). The conditional headers (`if_match`/`if_none_match`/`if_*_since`) have to be evaluated against the object's live state, which the full-object cache cannot do on a hit. So the clean way to fully honor them is to bypass the full-object cache when the request carries a conditional, letting the service evaluate the condition.
Fine to mark this one as partially resolving. I am happy to send that cache-bypass-on-conditional as a follow-up PR (or fold it in here if you would prefer it in one change) — whichever you like.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(layers/foyer): preserve read args while filling full-object cache [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on PR #7718:
URL: https://github.com/apache/opendal/pull/7718#issuecomment-4826619272
Your evaluation is 100%. Agreed.
I will merge this one so you can build against master! Thank you for bringing PRs and also your patience.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(layers/foyer): preserve read args while filling full-object cache [opendal]
Posted by "tonghuaroot (via GitHub)" <gi...@apache.org>.
tonghuaroot commented on PR #7718:
URL: https://github.com/apache/opendal/pull/7718#issuecomment-4668171618
The failing HDFS behavior test looks unrelated to this change (it only touches FoyerLayer) — likely the same flakiness as #7577. Could a maintainer retrigger the job?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org