[improvement](compaction) extract functions of compaction execution - #67466
Open
mymeiyi wants to merge 1 commit into
Open
[improvement](compaction) extract functions of compaction execution#67466mymeiyi wants to merge 1 commit into
mymeiyi wants to merge 1 commit into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
It introduces a potentially incorrect DORIS_CHECK_GT(execution_start_time_us, 0) that can fail for valid monotonic timestamps (e.g., 0 very early after boot).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refactors BE storage compaction code by extracting compaction execution steps into reusable helper functions (especially for cloud compaction), which should make it easier to introduce parallel compaction execution later.
Changes:
- Split
Compaction::merge_input_rowsets()intoprepare_*,execute_*, andfinish_*stages via a newMergeInputRowsetsContext. - Split cloud compaction execution (
CloudCompactionMixin::execute_compact_impl) intoprepare_execute_compact()+finish_execute_compact()with elapsed-time accounting based onMonotonicMicros(). - Minor header re-organization to expose the new helper methods and adjust declarations.
Critical Checkpoints (Part 1.3)
- Goal & completeness: Achieves the stated goal (function extraction for reuse) without changing the overall execution order of cloud compaction.
- Concurrency: No new concurrency introduced; execution flow remains sequential.
- Lifecycle / cleanup: No new ownership/lifetime hazards introduced; the new context is stack-scoped and passed through stages.
- Compatibility / storage format: No protocol or on-disk format changes observed in the diff.
- Tests: No tests were added/updated; this appears to be a refactor rather than new behavior.
- Observability: Logging remains present; elapsed-time metric updates are preserved (but see the correctness issue below).
- Main issue found: An invariant check in the new
finish_execute_compact()can spuriously fail (comment added).
File summaries
| File | Description |
|---|---|
| be/src/storage/compaction/compaction.h | Adds context/stage APIs for merge_input_rowsets and cloud compaction execution helpers. |
| be/src/storage/compaction/compaction.cpp | Implements the staged execution helpers and refactors cloud compaction timing to use MonotonicMicros(). |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Compaction merge execution and Cloud compaction execution each combined preparation, merge work, and finalization in one method, preventing callers from coordinating those phases independently. Extract shared execution contexts and prepare, execute, and finish methods while preserving operation order and timer scopes. Move get_compaction_permits to protected so derived compaction implementations can coordinate the same execution flow. ### Release note None ### Check List (For Author) - Test: Not run per request; clang-format and build hygiene checks passed - Behavior changed: No - Does this need documentation: No
mymeiyi
force-pushed
the
5-abstract-compaction
branch
from
September 3, 2026 06:13
699c33f to
b18221d
Compare
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review conclusion: no blocking correctness issue found on head b18221de47adf5d49f8a70e7c66e3cf5a9ecc817.
Critical checkpoints:
- Goal and proof: the PR extracts merge and Cloud execution phases for reuse. The only current wrappers preserve the original operation order, timer boundaries, virtual-hook dispatch, and
Statusshort-circuit points. No targeted test was added and the BE build/tests were skipped; existing compaction tests cover the unchanged grouped-result hooks, so this is a test-proof limitation rather than a demonstrated regression. - Scope and compatibility: the two-file change is focused. It adds protected non-virtual helpers/context and widens access to an existing helper without changing object layout, vtables, config, FE/BE protocol, serialization, storage format, or persisted metadata.
- Concurrency and lifecycle: current callers remain sequential. The merge context lives through prepare/execute/finish, and the writer copies the reader shared pointers. The helpers mutate per-compaction writer/stat/schema/profile state, so future parallel coordination must not interleave them on the same object without isolating that state.
- Error, write, and transaction safety: output preparation -> merge/build/check -> meta-service commit -> virtual in-memory modify remains unchanged. Status and Doris-exception exits still reach the existing subclass garbage-collection/abort path, including the MoW delete-bitmap lock fallback.
- Parallel paths: local compaction still uses the sequential merge wrapper; Cloud base, cumulative, full, and index-change paths still pass through the Cloud wrapper; cumulative grouped-compaction hooks retain their per-call result state.
- Performance and observability: the reader-vector work and counter/log updates are unchanged.
MonotonicMicros()measures the same interval as the old stopwatch and is safer against wall-clock adjustment. The prior inline concern about rejecting timestamp zero is resolved on this head. - Validation: live formatting, license, and repository checks pass. No build or test execution was reported for this head.
- User focus: no additional focus was supplied; the complete PR was reviewed.
Review completion: complete after one full normal/risk round; all subagents returned no new valuable findings, and all initial risks and existing-thread context were resolved.
Contributor
Author
|
run buildall |
mymeiyi
added a commit
to mymeiyi/doris
that referenced
this pull request
Sep 3, 2026
…paction ### What problem does this PR solve? Issue Number: None Related PR: apache#67466 Problem Summary: Distributed compaction workers use a preferred peer to read input blocks from the coordinator, but the read-through path synchronously duplicated those one-shot blocks in each worker file cache. When a preferred peer is configured, skip cache append and finalize while preserving the existing cache lookup path. Also skip prefetch because it cannot provide value without cache writeback. ### Release note Distributed compaction workers no longer cache input blocks fetched through the coordinator preferred-peer path. ### Check List (For Author) - Test: Unit Test (updated, not run per request) - Behavior changed: Yes (preferred-peer distributed compaction reads no longer write worker file cache) - Does this need documentation: No
mymeiyi
added a commit
to mymeiyi/doris
that referenced
this pull request
Sep 3, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#67466 Problem Summary: Distributed compaction readers need to read cached data from the coordinator even when general peer reads are disabled. Carry an optional preferred peer through RowsetReader and IOContext, try only that peer first, and fall back to remote storage on failure. Because worker compaction reads are one-shot, skip worker file-cache writeback and ineffective prefetch when a preferred peer is configured. Record successful preferred peers in PeerCacheNodes and cover routing, fallback, propagation, and no-write behavior. ### Release note Support per-request preferred peer reads for backend rowset readers. Distributed compaction workers do not cache input blocks fetched through the preferred-peer path. ### Check List (For Author) - Test: Unit Test not completed - ./run-be-ut.sh --run --filter=CachedRemoteFileReaderPeerTest.read_at_uses_preferred_peer_when_global_peer_read_disabled -j4 was attempted previously; configuration failed because thirdparty/installed/arrow-24.0.0 is incomplete - Per request, the merged changes were not compiled or run - Behavior changed: Yes (a configured preferred peer is tried before remote storage, reported in PeerCacheNodes, and does not write worker file cache) - Does this need documentation: No
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 16887 ms |
Contributor
TPC-DS: Total hot run time: 82018 ms |
Contributor
ClickBench: Total hot run time: 14.61 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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.
Later, we will support parallel compaction. This PR extracts the cloud compaction execution into reusable functions.