fix(fileservice): bound full-object cache wait fallback - #24871
Merged
mergify[bot] merged 4 commits intoJun 8, 2026
Merged
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
LeftHandCold
force-pushed
the
fix/fileservice-bounded-full-object-wait
branch
from
June 7, 2026 14:49
e1115bb to
7749ae8
Compare
There was a problem hiding this comment.
Pull request overview
This PR adjusts MatrixOne’s fileservice S3 read-path concurrency behavior to improve cache convergence under concurrent range reads, by adding a short bounded wait for in-flight full-object work (disk-cache fill and IOMerger merges) before falling back to minimal-range S3 reads.
Changes:
- Add a short bounded wait for full-object
IOMergerwaits, and fall back to minimal-range S3 reads only if the full-object merge remains active. - Add a short bounded wait when reading the full-file disk-cache entry while it is being updated (instead of immediately treating it as a miss).
- In the streaming full-object disk-cache fill path, release the captured
Dataslice afterCachedDatais produced; add tests for both the bounded-wait behavior and theDatarelease behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/fileservice/s3_fs.go | Adjusts read/merge control flow to use a short wait for full-object merges and minimal-range fallback only when still merging; releases entry Data after producing CachedData in streaming fill. |
| pkg/fileservice/s3_fs_test.go | Adds a regression test ensuring full-object streaming fill returns CachedData without retaining the full-object Data slice. |
| pkg/fileservice/io_merger.go | Introduces shortIOWaitDuration and makes IOMerger waiting honor short max wait durations (not delayed by the slow wait interval). |
| pkg/fileservice/io_merger_test.go | Adds a test verifying short max-wait isn’t delayed by the slow wait duration. |
| pkg/fileservice/disk_cache.go | Adds a short bounded wait when attempting to read the full-file cache entry while it’s being updated. |
fengttt
approved these changes
Jun 8, 2026
XuPeng-SH
requested changes
Jun 8, 2026
XuPeng-SH
left a comment
XuPeng-SH
left a comment
Contributor
There was a problem hiding this comment.
I did not find a blocking logic bug in the implementation itself, but there are two important unhappy-path regressions that still need tests before I am comfortable approving this.
pkg/fileservice/disk_cache.gonow adds a bounded wait (waitUpdateCompleteFor(..., shortIOWaitDuration)) before treating an in-progress full-file cache update as a miss, but there is no regression test for the success path where the update finishes within that window and the read then hits disk cache. That is one of the main new branches introduced by this PR.pkg/fileservice/s3_fs.gonow adds a bounded wait for full-objectIOMergerwork and then falls back to minimal-range S3 read when the merge is still active, but I do not see a test that pins this timeout/fallback path either. This is the other key unhappy-path boundary added by the change.
Please add regression coverage for both new timeout/wait branches, then this should be in much better shape.
Contributor
Author
|
Addressed in cb2b316. Added regression coverage for both requested paths:
Verified:
|
XuPeng-SH
approved these changes
Jun 8, 2026
XuPeng-SH
left a comment
XuPeng-SH
left a comment
Contributor
There was a problem hiding this comment.
This looks good to me.
The bounded-wait change stays focused on the real regression: it gives full-object cache reuse a short chance to win, but still preserves the no-long-wait fallback to a minimal S3 range read. The updated tests also cover the important timing-sensitive branches well enough for this kind of internal fileservice fix.
Contributor
Merge Queue Status
This pull request spent 21 seconds in the queue, including 3 seconds running CI. Required conditions to merge
|
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Which issue(s) this PR fixes:
issue #24870
What this PR does / why we need it:
Summary
IOMergerwaits, then fall back to minimal S3 range read only if the full-object merge is still activeDataslice afterCachedDatais producedRoot Cause
#24759 correctly fixed RSS pressure by streaming full-object disk cache fill, but the same PR also made concurrent range reads bypass full-object disk-cache update and full-object
IOMergerimmediately. Under TPCC/sysbench warmup this can turn one shared full-object fill into many independent S3 range reads, reducing cache convergence and increasing S3 fan-out.A full revert to the old unbounded wait was tested in nightly run 26930027366 and made TPCC 1000w load too slow. The bounded-wait probe in nightly run 26935873712 kept the completed sysbench cases clean (
ignored errors: 0) and let the TPCC job succeed, so this PR keeps the no-long-wait property while restoring a short opportunity for cache reuse.