[fix](regression) Stabilize base compaction file size limit case#65413
[fix](regression) Stabilize base compaction file size limit case#65413shuke987 wants to merge 1 commit into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes. I found two regression-test isolation/cleanup issues in the single changed suite.
Critical checkpoints:
- Goal/test coverage: the PR attempts to stabilize a p2 compaction regression by forcing BE config values; the test intent is clear, but config isolation and cleanup are incomplete.
- Scope: the code change is focused to one suite.
- Concurrency/config: problematic; new BE-global mutable configs are changed while the suite remains in the normal concurrent p2 pool.
- Compatibility/persistence/FE-BE protocol/data writes/lifecycle: not applicable; this is a regression-only change.
- Parallel paths: similar config-mutating compaction suites use nonConcurrent and per-BE config restore maps; this suite does not.
- Test result correctness: no .out changes. Static review only; I did not run the regression because this checkout is missing thirdparty/installed and the normal build/test dependencies.
User focus: no additional user-provided focus.
Subagent conclusions: optimizer-rewrite found no optimizer-related issues. tests-session-config proposed TEST-1, merged into M-2. Convergence round 1 ended with both live subagents returning NO_NEW_VALUABLE_FINDINGS for final comments M-1 and M-2.
| def configList = parseJson(out.trim()) | ||
| assert configList instanceof List | ||
|
|
||
| boolean originalDisableAutoCompaction = false |
There was a problem hiding this comment.
This snapshots the original value from only backendId_to_backendIP.keySet()[0], but set_be_param applies the restore to every backend. Since /api/update_config is sent to each BE separately, the runtime value is per backend; if one BE starts with a different disable_auto_compaction or base_compaction_dup_key_max_file_size_mbytes, the finally block overwrites it with BE0's value instead of restoring it. Nearby compaction tests keep a backendId_to_params map keyed by backend id for this reason. Please capture both original config values per backend and restore each BE's own values.
| // This test expects manual base compaction to be rejected with E-808 after building a | ||
| // single large base rowset. Background auto compaction can reshape those rowsets, so keep | ||
| // it disabled cluster-wide while the case is running. | ||
| set_be_param("disable_auto_compaction", "true") |
There was a problem hiding this comment.
This case now changes BE process-wide compaction settings on every backend, but the suite is still tagged only as p2, so it will run in the normal parallel suite pool. While it is between these calls and the finally restore, any other concurrently running case that relies on automatic compaction or duplicate-key base compaction can observe disable_auto_compaction=true or the temporary 100 MB base-compaction limit and fail or skip compaction for reasons unrelated to that case. The regression framework only routes suites containing nonConcurrent to the single-thread executor, and nearby compaction tests that mutate BE configs use that tag. Please mark this suite as non-concurrent as well, e.g. keep the priority but change the group to p2,nonConcurrent.
shuke987
force-pushed
the
fix-master-base-compaction-file-size-limit
branch
from
9237b58 to
303804d
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated review complete. I did not find a new distinct issue to raise beyond the two existing inline threads, and the current head appears to address both of them: the suite is now tagged p2,nonConcurrent, and the BE config values are captured/restored per backend.
Checkpoint conclusions:
- Goal/test proof: the PR stabilizes this compaction regression by disabling background compaction during the case and lowering
base_compaction_dup_key_max_file_size_mbytesso the final manual base compaction deterministically hits E-808. The changed test is the proof target, but I did not run it in this shallow Actions checkout. - Scope/focus: the change is limited to one regression suite and follows existing BE config mutation patterns used by nearby tests.
- Concurrency/config: the process-wide BE config mutation is now isolated by
nonConcurrent; the regression runner routes that group to the single-thread executor. - Compatibility/lifecycle/persistence: no storage format, protocol, EditLog, or mixed-version behavior is changed.
- Parallel paths: both modified BE configs are set on every backend and restored from each backend's own captured value.
- Test expectations/results: no
.outresult is involved. The E-808 expectation matches the duplicate-key base-compaction filter path for leading rowsets over the mutable size limit. - Observability/performance: no production observability or hot-path performance behavior is changed.
Subagent conclusions:
optimizer-rewrite: no optimizer/rewrite or parallel join/aggregate issue was found; the PR is test/config-only and does not change planner code.tests-session-config: no new issue was found beyond existing threads 3550173072 and 3550173068; both concerns are addressed in the live patch.- Final convergence round 1 ended with both subagents replying
NO_NEW_VALUABLE_FINDINGSfor the same ledger/comment set.
User focus: no additional user-provided review focus was present.
Validation was static only; no local regression test was run because this checkout lacks the normal runtime dependencies/cluster setup.
|
run buildall |
2 participants
Summary
Stabilize
test_base_compaction_with_dup_key_max_file_size_limiton master by making the expected base-compaction size gate deterministic.The case expects manual base compaction on a DUPLICATE KEY tablet to be rejected with
E-808after the first input rowset exceedsbase_compaction_dup_key_max_file_size_mbytes. The TPCH customer load can produce rowsets below the default 1GB threshold, so the manual compaction may succeed instead of hitting the rejection path.This patch:
base_compaction_dup_key_max_file_size_mbytesto 100MB for this casefinallyTesting
git diff --checkE-808and the suite passed.