Repository navigation
Conversation
### What problem does this PR solve? Issue Number: N/A Related PR: apache#65340 Problem Summary: The insert visible-timeout case enabled PublishVersionDaemon.stop_publish only on the FE HTTP endpoint configured for the regression runner. Dynamic pipelines can connect the runner to a follower FE, while PublishVersionDaemon runs only on the master FE, so the debug point may not block publishing and the expected timeout is not returned. Apply and remove the debug point on all FEs so the master is always covered and cleanup remains symmetric. ### Release note None ### Check List (For Author) - Test: Manual test - Built the regression-test framework with Maven. - Compiled the target Groovy suite with FileSystemCompiler. - Ran git diff --check. - Behavior changed: No. Test-only stabilization. - Does this need documentation: No.
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 20th, 2026 12:23 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
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 result: request changes.
Found one PR-scoped issue in the regression test: the new all-FE debug-point path depends on SHOW FRONTENDS addresses and can fail or leak in runner setups where those addresses are loopback or otherwise unreachable. I left one inline comment.
Checkpoint conclusions:
- Goal/test: the test still targets
insert_visible_timeout_return_mode, but the new debug-point targeting can fail before the behavior is exercised in multi-FE runner address setups. - Scope: the change is small and test-only, but it replaces a deliberately reachable endpoint with
SHOW FRONTENDS-derived endpoints. - Concurrency/lifecycle: no product concurrency changed; the debug-point lifecycle and cleanup risk is covered inline.
- Compatibility/persistence/config/FE-BE protocol/data writes: no product behavior changes.
- Parallel paths: nearby publish-stop tests also use all-FE helpers, but this PR uniquely removes the configured-endpoint workaround.
- Test coverage/results: the existing generated
.outstill matches the final ordered result; tests were not run in this review-only environment. - User focus: no additional user-provided review focus.
Contributor
There was a problem hiding this comment.
Automated review result: request changes.
Found one PR-scoped issue in the regression test: the new all-FE debug-point path depends on SHOW FRONTENDS addresses and can fail or leak in runner setups where those addresses are loopback or otherwise unreachable. I left one inline comment.
Checkpoint conclusions:
- Goal/test: the test still targets
insert_visible_timeout_return_mode, but the new debug-point targeting can fail before the behavior is exercised in multi-FE runner address setups. - Scope: the change is small and test-only, but it replaces a deliberately reachable endpoint with
SHOW FRONTENDS-derived endpoints. - Concurrency/lifecycle: no product concurrency changed; the debug-point lifecycle and cleanup risk is covered inline.
- Compatibility/persistence/config/FE-BE protocol/data writes: no product behavior changes.
- Parallel paths: nearby publish-stop tests also use all-FE helpers, but this PR uniquely removes the configured-endpoint workaround.
- Test coverage/results: the existing generated
.outstill matches the final ordered result; tests were not run in this review-only environment. - User focus: no additional user-provided review focus.
| DebugPoint.enableDebugPoint(feHost, feHttpPort, NodeType.FE, debugPoint) | ||
| // PublishVersionDaemon only runs on the master FE. Enable the debug point on every FE so the | ||
| // case does not depend on whether the regression runner is connected to a master or follower FE. | ||
| GetDebugPoint().enableDebugPointForAllFEs(debugPoint) |
Contributor
There was a problem hiding this comment.
This regresses the address handling the deleted code was avoiding. enableDebugPointForAllFEs builds its targets from SHOW FRONTENDS via Suite.getFrontendIpHttpPort(), but those rows are not guaranteed to be HTTP endpoints reachable from the regression runner: the removed code was explicitly using context.config.feHttpAddress because SHOW FRONTENDS may expose loopback addresses, and FrontendsProcNode also reports the connected FE's HttpURLUtil.getHttpPort() for each row. In those multi-FE/container setups this call can post to the wrong or unreachable endpoint before the test reaches the insert checks; if cleanup later aborts in the same all-FE loop, the master can keep PublishVersionDaemon.stop_publish enabled because this call has no timeout and the cleanup exception is only logged. Please target the master through a reachable address mapping, or make the all-FE helper use reachable FE HTTP endpoints and add bounded/robust cleanup for this debug point.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#65340 Problem Summary: PublishVersionDaemon runs only on the master FE. Identify the master from one SHOW FRONTENDS snapshot and apply the stop_publish debug point only to that FE, while keeping the suite JDBC connection unchanged. This avoids injecting the fault into every FE and avoids coupling the test to a master JDBC endpoint. ### Release note None ### Check List (For Author) - Test: Manual test - Built the regression-test framework with Maven. - Compiled the target Groovy suite with FileSystemCompiler. - Ran git diff --check. - Behavior changed: No. Test-only stabilization. - Does this need documentation: No.
shuke987
changed the title
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Automated review follow-up: comment-only.
I reviewed the latest head after the single-master rewrite. I am not adding another inline comment because the remaining substantiated problem is the same one already raised in discussion_r3584194843: the test still takes the master FE HTTP target from SHOW FRONTENDS, while the deleted code used the runner-configured FE HTTP address specifically to avoid unreachable/loopback proc-node addresses. The current patch narrows the target to one FE, but it keeps the same endpoint source and best-effort cleanup behavior, so the existing changes-requested thread still applies.
Checkpoint conclusions:
- Goal/test: the suite is intended to stabilize coverage for insert_visible_timeout_return_mode by blocking master publish; the committed/error assertions and final ordered select cover the intended behavior when the debug point is actually injected.
- Scope: the PR is small and test-only.
- Concurrency/lifecycle: no product concurrency changes; the debug-point lifecycle risk is the existing endpoint/cleanup issue above.
- Compatibility/persistence/config/FE-BE protocol/data writes: no product behavior, storage, or protocol changes.
- Parallel paths: PublishVersionDaemon.stop_publish is master-only, so targeting the master is the right behavioral direction; the remaining gap is using a runner-reachable master HTTP mapping.
- Test coverage/results: the existing generated .out contains the deterministic final rows; I did not run tests in this review-only runner.
- User focus: no additional user-provided review focus.
shuke987
marked this pull request as ready for review
July 15, 2026 12:42
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Gabriel39
pushed a commit
that referenced
this pull request
Nov 6, 2026
…t_generation (#70308) ### Background An auto-partition table with `partition.retention_count = N` keeps only its N latest historical partitions: the FE's `DynamicPartitionScheduler` drops older ones on every run, once per `dynamic_partition_check_interval_seconds` (600 by default). `partition_p0/auto_partition/test_auto_new_recycle` lowers that interval to 1 s and checks the partition count after each load or ALTER. `audit/test_sql_digest_generation` checks that the audit log gets an `sql_digest` only for queries slower than the FE config `sql_digest_generation_threshold_ms`, once with the threshold at 0 and once at 100000. Both suites are in group `nonConcurrent`. ### What problem does this PR solve? Issue Number: None Related PR: #65375 and #65646 (the branch-4.1 recycle-suite fixes by @shuke987, ported here), #65343 (the branch-4.1 digest-suite fix by @shuke987, ported here), #65435 (master's earlier move of the recycle suite's day-boundary guard) PR Type: Test Problem Summary: Both suites rely on timing the product does not promise. The recycle suite checks the partition count after a fixed `sleep(8000)` (5 s after an ALTER), while the drop happens asynchronously at an unbounded time after the config change; branch-4.1's NonConcurrent pipeline failed at the first such check repeatedly (#65375). The digest suite's threshold-0 check needs the query's elapsed time to be above 0 ms. This PR gives both suites their branch-4.1/4.2 form: the recycle suite polls the partition count every second, for at most 1200 s, and widens its day-boundary guard to cover the poll; the digest query adds `sleep(1)`. Both files become identical to branch-4.2. ### Details - **Failure:** On master's public NonConcurrent pipeline neither suite has failed on its own since mid-July: of about 8,000 runs each, the recycle suite failed 3 times and the digest suite 87 times, all in builds whose cluster had died (most suites failing). The failures that motivated the fixes are on branch-4.1's pipeline (#65375); #65343 made the digest query deterministic there. - **Cause:** `ADMIN SET FRONTEND CONFIG` sets the scheduler's new interval at once, but the scheduler sleeps in 5-second slices and notices the change only at the next slice. It then runs `executeDynamicPartition` over every registered table before the drop. So the drop lands 0 to 5 s plus one full pass after the config change, and a pass longer than about 3 s fails the 8-second check. In the digest suite, `AuditLogHelper` writes a digest only when `elapseMs > sql_digest_generation_threshold_ms`, and nothing bounds the elapsed time of `select *` on a two-row table from below (9 to 15 ms locally). With `sleep(1)` on each row it takes about 2 s, still far below the second check's 100000 ms. - **Stability evidence:** The recycle checks now wait for the drop and fail only when the count misses the target for 1200 s. The poll runs on the suite thread, because the framework sets `Awaitility.pollInSameThread()` (#68338), so it does not leak connections. Shortening one of master's waits to 1 s reproduces the branch-4.1 failure shape, and the ported suite passes; mutants show that both suites still catch wrong results (see Testing). ### Testing All runs used `./run-regression-test.sh --run -d partition_p0/auto_partition -s test_auto_new_recycle` or `--run -d audit -s test_sql_digest_generation` against a local single-FE, single-BE cluster built from master. | Suite | Version | Result | |---|---|---| | `test_auto_new_recycle` | master | passes | | `test_auto_new_recycle` | this PR | passes; the polls reached the target after 1, 2, 1, 3 and 3 s | | `test_auto_new_recycle` | master, with the wait before the 10-partition check cut to 1 s | fails as #65375 describes: `expected: <101> but was: <10>` (master's `assertEquals` has its arguments swapped, so 101 partitions were still there) | | `test_auto_new_recycle` | this PR, expecting 11 partitions, poll capped at 20 s | fails as it should: `ConditionTimeoutException` | | `test_sql_digest_generation` | master | passes | | `test_sql_digest_generation` | this PR | passes; the query took 2,024 and 2,025 ms | | `test_sql_digest_generation` | this PR, second threshold 1000 ms | fails as it should: `Count of sql_digest changed` | The PR NonConcurrent pipeline runs both suites; cloud_p0 skips `nonConcurrent` suites. ### Notes for Reviewers - The day-boundary guard before the final `now()`-based section goes from 100 s to 1300 s, as in #65646, so that it covers the 1200 s poll plus the insert and checks. It sleeps only when that section would otherwise start within 1300 s of midnight. - Four more suites fixed in the same branch-4.1 batch need nothing on master: #65340 (master #68449 enables the publish debug point on all FEs, superseding the open port #65556), #65929 (master #68329 fixed the actual cause, a connection leak), #65885 (master #69723 removed the V1 scanner whose message it also accepted), and #67363 (ported as #67402). ### Release note None ### Check List (For Author) - Behavior changed: No - Does this need documentation: No - Pre-submission status: Ready ### AI Disclosure - Tool: Claude Code (Claude Opus 5.5) - Generated: the port (both suites taken from branch-4.2), the local verification runs, and this description - Human review: not stated - Prompts: The author asked to continue moving branch-4.2's changes back to master, where six branch-4.1 test fixes were recorded as one task. After the check showed that master already covers four of them and has not failed on the other two, the author chose to port those two exactly as branch-4.2 has them and to verify them on a local cluster. Co-authored-by: shuke <shuke@selectdb.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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 problem does this PR solve?
Issue Number: N/A
Related PR: #65340
Problem Summary:
test_insert_visible_timeout_return_modeenablesPublishVersionDaemon.stop_publishto exercise the committed and error return modes after the visible wait times out.The case currently adds the debug point only to the FE HTTP endpoint configured for the regression runner. In a multi-FE dynamic pipeline, that endpoint can belong to a follower FE, while
PublishVersionDaemonruns only on the master FE. Publishing then continues normally and the expected timeout error is not returned.Find the master FE from one
SHOW FRONTENDSsnapshot and enable/disable the debug point only on that FE. Keep the suite JDBC connection unchanged so the case does not depend on a separate runner-facing master JDBC mapping.Release note
None
Check List (For Author)
Test
mvn -f regression-test/framework/pom.xml -DskipTests packageorg.codehaus.groovy.tools.FileSystemCompilergit diff --checkBehavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)