[fix](regression) fix index change wait timeout#65453
Conversation
|
run buildall |
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found two remaining timeout-wait paths that can still report success without observing the expected terminal state. Both are in the same index-change regression area this PR is trying to harden, so I am requesting changes.
Critical checkpoint conclusions:
- Goal and proof: The shared build-index wait helper now requires
FINISHED, but the PR does not fully accomplish the stated timeout-hardening goal because two exercised parallel wait paths still use the old timeout-boundary assertion pattern. - Scope: The patch is small and focused, but it leaves related helpers inconsistent.
- Concurrency/lifecycle/config/compatibility/persistence/data writes/FE-BE protocol: Not materially involved; this is regression-test wait logic only.
- Parallel paths: Issue found. The suite-local V1 build-index partition wait and shared column-change wait need the same explicit-finished handling.
- Test coverage/results: No
.outchanges. I did not run the regression suite because this shallow runner is missing.worktree_initializedandthirdparty/installed; I did verify the GitHub PR patch applies cleanly in reverse with whitespace errors enabled. - Observability: The fixed build-index timeout message is better; the remaining helpers still lack the latest-result failure on timeout.
- Performance: No material performance concern beyond the intentional longer wait.
- Other issues: No optimizer/rewrite issue found.
Subagent conclusions:
optimizer-rewrite: no candidates; convergence round 1 returnedNO_NEW_VALUABLE_FINDINGS.tests-session-config: TSC-1 and TSC-2 were independently verified and accepted as M-1 and M-2 inline comments.- Dismissed/duplicates: none; existing inline review threads were empty.
- Final convergence: after M-1 and M-2 were added to the ledger/comment set, both live subagents returned
NO_NEW_VALUABLE_FINDINGSin the same convergence round.
User focus: No additional user-provided review focus was supplied.
There was a problem hiding this comment.
I found two remaining timeout-wait paths that can still report success without observing the expected terminal state. Both are in the same index-change regression area this PR is trying to harden, so I am requesting changes.
Critical checkpoint conclusions:
- Goal and proof: The shared build-index wait helper now requires
FINISHED, but the PR does not fully accomplish the stated timeout-hardening goal because two exercised parallel wait paths still use the old timeout-boundary assertion pattern. - Scope: The patch is small and focused, but it leaves related helpers inconsistent.
- Concurrency/lifecycle/config/compatibility/persistence/data writes/FE-BE protocol: Not materially involved; this is regression-test wait logic only.
- Parallel paths: Issue found. The suite-local V1 build-index partition wait and shared column-change wait need the same explicit-finished handling.
- Test coverage/results: No
.outchanges. I did not run the regression suite because this shallow runner is missing.worktree_initializedandthirdparty/installed; I did verify the GitHub PR patch applies cleanly in reverse with whitespace errors enabled. - Observability: The fixed build-index timeout message is better; the remaining helpers still lack the latest-result failure on timeout.
- Performance: No material performance concern beyond the intentional longer wait.
- Other issues: No optimizer/rewrite issue found.
Subagent conclusions:
optimizer-rewrite: no candidates; convergence round 1 returnedNO_NEW_VALUABLE_FINDINGS.tests-session-config: TSC-1 and TSC-2 were independently verified and accepted as M-1 and M-2 inline comments.- Dismissed/duplicates: none; existing inline review threads were empty.
- Final convergence: after M-1 and M-2 were added to the ledger/comment set, both live subagents returned
NO_NEW_VALUABLE_FINDINGSin the same convergence round.
User focus: No additional user-provided review focus was supplied.
|
|
||
| suite("test_index_change_4") { | ||
| def timeout = 60000 | ||
| def timeout = 300000 |
There was a problem hiding this comment.
This gives the suite more time, but the non-cloud V1 path still uses a local wait helper that can pass after timing out. Below, wait_for_build_index_on_partition_finish(tableName, timeout) loops while t <= OpTimeout, assigns useTime = t for unfinished rows, and finally asserts useTime <= OpTimeout. On the last poll useTime is exactly OpTimeout, so the assertion succeeds even if one or more build-index jobs never reached FINISHED. Please apply the same explicit finished flag and timeout failure message to this local helper, or reuse an equivalent fixed helper, otherwise this suite can still mask the timeout condition this PR is trying to catch.
There was a problem hiding this comment.
Fixed in 537124f: the suite-local V1 partition wait now requires an explicit finished state and reports the latest result on timeout.
| sleep(delta_time) | ||
| } | ||
| assertTrue(useTime <= OpTimeout, "wait for last build index finish timeout") | ||
| assertTrue(finished, "wait for last build index finish timeout, latest result: ${alter_res}") |
There was a problem hiding this comment.
This fixes the build-index wait, but the same shared plugin still has the old timeout-success pattern in wait_for_last_col_change_finish. That helper also polls through t <= OpTimeout, records useTime = t while FINISHED has not been seen, and then asserts useTime <= OpTimeout, so it succeeds on the final poll even when the column alter job never finished. test_index_change_4 calls that helper before later build-index waits, so a stalled column change can still be hidden. Please update the column-change helper with the same explicit finished flag and latest-result timeout failure.
There was a problem hiding this comment.
Fixed in 537124f: the shared column-change wait now requires an explicit finished state, stops at the deadline, and reports the latest result on timeout.
|
/review |
|
run buildall |
There was a problem hiding this comment.
I found one blocking regression in the shared regression-test helper.
Critical checkpoint conclusions:
- Goal/test proof: the PR fixes the previously reported timeout-success waits in the changed helper and suite-local path, but it also makes one shared wrapper fail in schema-only callers.
- Scope/focus: the patch is small and limited to regression helper timeout behavior plus the
test_index_change_4timeout. - Concurrency/lifecycle: no new production concurrency or lifecycle code is modified; the relevant async behavior is regression polling of FE schema/index jobs.
- Configuration/session compatibility:
enable_add_index_for_new_databehavior in the touched suite is unchanged, but shared helper compatibility regresses for callers that usewait_for_last_schema_change_finishwithout submitting a build-index job. - Parallel paths: cloud/non-cloud and build-index/schema-change helper paths were checked; the issue is specifically the wrapper combining both waits unconditionally.
- Test coverage/results: no
.outresult changes are needed for this helper-only change. I did not run regression tests because this checkout is not worktree-initialized andthirdparty/installedis missing. I did rungit diff --checkon the fetched base/head for the two changed files. - Observability/performance/persistence: not materially affected by this regression-test-only PR.
User focus: no additional user-provided review focus was specified.
Subagent conclusions: the optimizer-rewrite subagent found no optimizer-specific candidates. The tests-session-config subagent found no additional candidate beyond the main-agent finding. Existing threads 3557194980 and 3557194977 are fixed at head and were treated as duplicate context. Convergence round 2 ended with both live subagents returning NO_NEW_VALUABLE_FINDINGS for the final ledger/comment set.
| assertTrue(finished, "wait_for_last_col_change_finish timeout, latest result: ${alter_res}") | ||
| } | ||
|
|
||
| Suite.metaClass.wait_for_last_schema_change_finish = {table_name, OpTimeout -> |
There was a problem hiding this comment.
This wrapper now always calls wait_for_last_build_index_finish, but this PR changed that helper to fail unless SHOW BUILD INDEX ... LIMIT 1 observes a FINISHED row. Several shared-plugin callers use wait_for_last_schema_change_finish for ordinary schema-only index changes, for example test_create_index_2.groovy calls it immediately after create index ... without issuing a separate BUILD INDEX, and variant_p0/with_index/load.groovy does the same after drop/create index paths. Those operations complete via the column/schema-change side (SHOW ALTER TABLE COLUMN), not necessarily by creating a SHOW BUILD INDEX row, so after the column wait succeeds this second wait can now run for the full timeout and fail with an empty latest build-index result. Please split the wrapper semantics, or only wait on the build-index helper when the caller actually submitted a build-index job.
There was a problem hiding this comment.
Fixed in 57926e6: the schema-change wrapper now waits only for the column/schema job. The two callers that explicitly submit BUILD INDEX now wait for the column job and build-index job separately.
There was a problem hiding this comment.
Follow-up fixed in 3cd514f after inspecting P0 991501 and Cloud P0 991624. Direct shared-helper callers also legitimately see an empty SHOW BUILD INDEX result when no build-index job exists. The helper now returns immediately for an empty result, while remaining strict for existing nonterminal jobs. Mock coverage includes the empty-result path.
|
/review |
|
run buildall |
|
/review |
|
run buildall |
There was a problem hiding this comment.
I reviewed the refreshed head 3cd514f after the PR moved during review.
The main remaining issue is that the wait helpers still do not encode which job type each caller expects. The current patch fixes some timeout-success cases, but the new empty-result success path can now skip required waits, and the schema wrapper can still miss cloud drop-index cleanup jobs.
Checkpoint summary: the PR goal is not fully met; the change is small, but the shared helper semantics are now too broad for the parallel build-index/schema-change paths. No source concurrency, persistence format, config, or FE/BE protocol changes were introduced. The risk is regression-test correctness and false pass/fail behavior around async index-change jobs. I did not run builds or tests because the review prompt explicitly forbids build/test execution in this runner. There was no additional user-provided review focus. Existing review threads were deduplicated before submitting.
Process note: round 3 found one additional current-head candidate, which I folded into the shared-helper inline comment. Because the instructions cap the loop at 3 rounds, this review should be treated as incomplete with respect to further post-round-3 convergence, but all substantiated current findings are included below.
| for(int t = 0; t <= OpTimeout; t += delta_time){ | ||
| alter_res = sql """SHOW BUILD INDEX WHERE TableName = "${table_name}" ORDER BY CreateTime DESC LIMIT 1;""" | ||
| if (alter_res.isEmpty()) { | ||
| logger.info(table_name + " has no build index job to wait for") |
There was a problem hiding this comment.
Allowing an empty result here fixes callers that really have no build-index job, but this helper is also used in cases where empty is not a completed wait. Some callers invoke it immediately after explicit build submissions, e.g. variant_p0/with_index/load.groovy after build_index_on_table("inv_idx", table_name) and test_create_index_2.groovy in cloud mode after build index on ...; if SHOW BUILD INDEX is still empty, the test now proceeds without proving the requested build ran. Other direct callers should wait on the schema job instead: test_create_index_2.groovy drops a non-cloud NGRAM_BF index at line 110, which routes to the schema-change path rather than IndexChangeJob, so empty-result success skips the real wait. Finally, when build-index rows do exist, ORDER BY CreateTime DESC LIMIT 1 only checks one partition row, but FE creates one IndexChangeJob per selected partition, so one latest FINISHED row can hide sibling rows that are still RUNNING or CANCELLED. Please split the no-job/schema callers from explicit build/delete-index callers, and have the latter pass/check the expected job count or job id across all relevant rows.
There was a problem hiding this comment.
Fixed in 2fb05cb. The helper no longer accepts an empty result as success. Explicit build/delete callers snapshot existing JobIds before the DDL and pass that snapshot to the strict wait, which filters all newly created rows, fails on any new CANCELLED job, and requires the complete new-job set to be FINISHED and stable across two polls. Schema-only callers were moved to the column-change wait.
| sleep(10000) | ||
| logger.info(table_name + " all build index jobs finished, detail: " + alter_res) | ||
| finished = true | ||
| break |
There was a problem hiding this comment.
This helper still has a false-success path after the fix: when SHOW BUILD INDEX WHERE TableName = ... returns no rows, expected_finished_num is 0 and finished_num == expected_finished_num is true, so the newly added finished = true makes the final assertion pass without observing any build-index job. This V1 branch calls the helper immediately after build_index_on_table("idx_note", tableName), so zero matching jobs should fail or keep waiting rather than count as "all jobs finished". Please require at least one observed job, or pass/check the expected job count before accepting the all-finished condition.
There was a problem hiding this comment.
Fixed in 2fb05cb. The suite-local V1 helper now requires a nonempty SHOW BUILD INDEX result before finished_num == expected_finished_num can succeed, so zero rows remain pending and eventually time out.
| } | ||
|
|
||
| Suite.metaClass.wait_for_last_schema_change_finish = {table_name, OpTimeout -> | ||
| wait_for_last_col_change_finish(table_name, OpTimeout) |
There was a problem hiding this comment.
Making this wrapper column-only fixes the schema-only callers, but it also skips cloud full-table drop-index cleanup jobs. In cloud mode, dropping an inverted index with enable_add_index_for_new_data goes through modifyTableLightSchemaChange and then submits per-partition index-change delete jobs via buildOrDeleteTableInvertedIndices; SHOW ALTER TABLE COLUMN can finish before those jobs are done. Callers like variant_p0/with_index/load.groovy line 52 and test_add_drop_index.groovy line 147 then create/build another index on the same table, which can race the unfinished cleanup and hit the existing hasIndexChangeJobOnPartition/hasIndexChangeJobOnTable checks. Please split the wrapper semantics at these drop-index call sites, or add a separate helper so cloud drop-index waits include the relevant build-index/delete-index jobs while schema-only create-index waits remain column-only.
There was a problem hiding this comment.
Fixed in 2fb05cb. Cloud full-table DROP INDEX callers now snapshot existing build-index JobIds before the drop, wait for the column/schema job, and then wait for all newly created delete jobs. Schema-only create-index callers remain column-only.
There was a problem hiding this comment.
Requesting changes for two remaining regression-helper caller gaps.
Critical checkpoint conclusions:
- Goal/test intent: the PR tightens index-change wait helpers so regression tests fail on real timeouts instead of silently passing. The helper fixes are mostly aligned, but two post-drop callers now skip required build-index/delete-job waits.
- Scope: the code changes are small and regression-test scoped; no production code, storage format, FE/BE protocol, config, or persistence behavior is changed by the PR itself.
- Concurrency/lifecycle: the relevant asynchronous lifecycle is FE
IndexChangeJob. Full-table inverted-index drops can enqueue delete jobs; callers must wait for those jobs before starting later schema/index operations on the same table. - Parallel paths: explicit build-index waits and schema-only create-index waits were checked.
test_create_index_2andtest_index_change_4cover their drop/build paths correctly; the two reported callers remain uncovered. - Test coverage/style: this is regression-test helper logic. No tests were run in this review environment per the review prompt; validation was static against the authoritative diff and FE control flow.
- User focus: no additional user-provided focus was supplied.
Subagent convergence: after three rounds, all live subagents returned NO_NEW_VALUABLE_FINDINGS against the current ledger/comment set after M1 and M2 were accepted. Existing review threads were checked to avoid duplicates.
|
|
||
| wait_for_last_schema_change_finish(table_name, timeout) | ||
| wait_for_last_col_change_finish(table_name, timeout) | ||
| wait_for_last_build_index_finish(table_name, timeout) |
There was a problem hiding this comment.
The drop path above still uses wait_for_last_schema_change_finish, but this PR changed that wrapper to wait only on SHOW ALTER TABLE COLUMN. For an inverted-index drop with enable_add_index_for_new_data=true, FE treats the full-table DROP INDEX as a light index change and modifyTableLightSchemaChange enqueues asynchronous delete IndexChangeJobs via buildOrDeleteTableInvertedIndices; those jobs are what wait_for_last_build_index_finish observes. After the line-52 drop wait, this suite can therefore move on to this create/build sequence while the drop job is still WAITING_TXN/RUNNING. In cloud mode the following build can also hit hasIndexChangeJobOnPartition, because any unfinished index-change job on the partition conflicts. Please split the earlier drop wait the same way: wait the column job and then call wait_for_last_build_index_finish(table_name, timeout) before creating inv_idx.
There was a problem hiding this comment.
Fixed in 2fb05cb. The variant drop path now waits for the schema job plus all new Cloud delete jobs. Its create/build path waits for schema completion before BUILD INDEX, snapshots existing JobIds, and waits for every new build job.
|
|
||
| Suite.metaClass.wait_for_last_schema_change_finish = {table_name, OpTimeout -> | ||
| wait_for_last_col_change_finish(table_name, OpTimeout) | ||
| wait_for_last_build_index_finish(table_name, OpTimeout) |
There was a problem hiding this comment.
This wrapper is now column-only, but test_add_drop_index.groovy still calls wait_for_last_schema_change_finish(indexTbName1, timeout) immediately after drop index name_idx in the enable_add_index_for_new_data=true path. That drop is a full-table inverted-index drop, so FE treats it as a light index change and modifyTableLightSchemaChange enqueues asynchronous delete IndexChangeJobs through buildOrDeleteTableInvertedIndices. With the build-index wait removed here, the suite can proceed to the next create index studentInfo_idx while the drop job is still WAITING_TXN/RUNNING; the following schema change path rejects tables with active index-change jobs. Please update that caller too, e.g. wait the column job and then call wait_for_last_build_index_finish(indexTbName1, timeout) after the drop.
There was a problem hiding this comment.
Fixed in 2fb05cb. test_add_drop_index now snapshots existing JobIds before the Cloud full-table drop, waits for the schema job, and then waits for all newly created delete jobs before creating the next index.
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
There was a problem hiding this comment.
I completed another full static review pass for PR 65453 at head 3cd514f11ef62bb0a4727c28d069d0196801392b, including the changed wait helpers and the three changed regression suites. I did not find any new non-duplicate issue to add beyond the existing inline threads.
The remaining correctness concerns are already covered by existing comments: empty SHOW BUILD INDEX success for callers that need to observe a real job, checking only the latest build-index row for multi-partition jobs, column-only schema waits after full-table drop-index cleanup, and the suite-local V1 zero-row false success. I verified the FE mechanics through SchemaChangeHandler.buildOrDeleteTableInvertedIndices, BuildIndexProcDir, and ShowBuildIndexCommand, and those existing comments cover the substantiated cases.
Checkpoint conclusions: the PR goal is to make these regression waits fail on real timeout and avoid waiting on nonexistent build-index jobs; the patch is small and focused, but the existing duplicate threads still cover wait-contract gaps. No production concurrency, persistence, protocol, or FE/BE behavior is changed by this PR. Test coverage is limited to regression-script behavior; no tests were run in this review-only environment.
User focus: no additional user-provided focus was present.
Review status: complete static review; no new inline comments submitted because all verified issues are already raised.
### What problem does this PR solve? Issue Number: None Related PR: apache#65453 Problem Summary: Empty SHOW BUILD INDEX results could mean either a schema-only change or an index job that had not appeared yet. Treating both as success let callers race unfinished schema and delete jobs, while checking only the latest row could miss sibling partition jobs. Snapshot existing job IDs before explicit build or delete operations, wait for every newly observed job to finish, and route schema-only index changes through the column-change wait. ### Release note None ### Check List (For Author) - Test: Manual test - Groovy helper behavior harness and syntax compilation for all changed suites - Behavior changed: No - Does this need documentation: No
|
/review |
|
run buildall |
There was a problem hiding this comment.
Requesting changes. The remaining issues are in the regression wait logic around asynchronous build-index/delete-index jobs: two paths can still mask unfinished jobs, one cloud sequence can race the next drop, and one cloud branch waits for build-index jobs that should not exist.
Critical checkpoint conclusions:
- Goal/test proof: the PR improves several waits, but the current code still does not reliably prove the intended index-change jobs in all changed suites.
- Cloud/non-cloud parallel paths: cloud full-table drop behavior depends on
enable_add_index_for_new_data; the changed code handles some true-mode paths but misses one false-mode path and one two-drop ordering path. - Async lifecycle/concurrency: the relevant lifecycle is FE
IndexChangeJobcreation and completion; unfinished cloud jobs can block later index DDL throughhasIndexChangeJobOnPartition. - Test coverage/results: static review only, per the review prompt. I did not run builds or tests.
- Existing thread dedupe: I suppressed the broad default-helper caller concern already covered by the existing
plugin_index_change.groovy:32thread. - User focus: no additional user-provided review focus was present.
- Subagent convergence: round 2 normal and risk-focused subagents both returned
NO_NEW_VALUABLE_FINDINGSfor this C1-C4 comment set.
| // create inverted index | ||
| sql "CREATE INDEX idx_v2 ON ${tableName1}(v2) USING INVERTED" | ||
| wait_for_last_build_index_finish(tableName1, timeout) | ||
| wait_for_last_col_change_finish(tableName1, timeout) |
There was a problem hiding this comment.
Changing the initial create-index wait to column-only leaves this suite relying on its local wait_for_build_index_on_partition_finish below for the actual DROP INDEX ... PARTITION jobs. That helper still has the old timeout-success pattern: if the new job count is never reached, or if any observed job remains WAITING_TXN/RUNNING or becomes CANCELLED through the final poll, it sets useTime to exactly OpTimeout and assertTrue(useTime <= OpTimeout, ...) passes. The test can then continue after an unfinished or failed partition drop job and still assert the metadata/query checks. Please switch the local helper to an explicit finished flag, fail on CANCELLED, and assert finished with the latest SHOW BUILD INDEX result.
There was a problem hiding this comment.
Fixed in 7e4bb8f. The partition-drop helper now uses an explicit finished flag, fails immediately on CANCELLED, requires the expected job count and all rows FINISHED, and reports the latest result on timeout.
| def previous_job_ids = isCloudMode() ? get_build_index_job_ids(tableName) : null | ||
| sql """ alter table ${tableName} drop index idx_s; """ | ||
| wait_for_last_build_index_finish(tableName, timeout) | ||
| wait_for_last_col_change_finish(tableName, timeout) |
There was a problem hiding this comment.
This file still has the same local build-index wait bug above in the non-cloud renamed-column path. After build_index_on_table("idx_s", tableName), wait_for_build_index_on_partition_finish breaks immediately when SHOW BUILD INDEX is empty because 0 == 0, and if rows stay WAITING_TXN/RUNNING or become CANCELLED through the final poll, useTime == OpTimeout still makes the final assertion pass. The drop wait here is stricter now, but the suite can still run the match query and index-file checks without proving the explicit build completed. Please reuse the strict helper pattern: require a nonempty observed job set for explicit builds, fail on CANCELLED, and assert an explicit finished flag with the latest result.
There was a problem hiding this comment.
Fixed in 7e4bb8f. The renamed-column non-cloud explicit build now snapshots existing JobIds and reuses the shared strict new-job helper; the flawed local helper was removed.
| sql "drop index name_idx_1 on ${indexTbName1}" | ||
| wait_for_last_build_index_finish(indexTbName1, timeout) | ||
| wait_for_last_col_change_finish(indexTbName1, timeout) | ||
| def previous_job_ids = isCloudMode() ? get_build_index_job_ids(indexTbName1) : null |
There was a problem hiding this comment.
This snapshot is taken too late for the first drop in the cloud path. With enable_add_index_for_new_data=true, cloud full-table drops of both NGRAM_BF and INVERTED indexes use the light-index path and enqueue per-partition IndexChangeJobs after the column/schema job. Here name_idx_1 is dropped and only the column job is waited; then this line snapshots the already-created delete jobs as previous_job_ids and the next line drops name_idx_2. If the first drop jobs are still WAITING_TXN/RUNNING, the second drop can hit the existing cloud hasIndexChangeJobOnPartition guard before the later strict wait runs, and that later wait filters the first drop jobs out anyway. Please snapshot before the first drop and wait for its new build-index/delete jobs before issuing the second drop, then snapshot/wait separately for the second drop.
There was a problem hiding this comment.
Fixed in 7e4bb8f. test_create_index_2 now snapshots before the first Cloud drop, waits for the new delete jobs from that drop, then snapshots and waits separately for the second drop.
| wait_for_last_build_index_finish(tableName, timeout) | ||
| wait_for_last_col_change_finish(tableName, timeout) | ||
| if (isCloudMode()) { | ||
| wait_for_last_build_index_finish(tableName, timeout, previous_job_ids) |
There was a problem hiding this comment.
This cloud wait expects new SHOW BUILD INDEX rows, but this suite set enable_add_index_for_new_data = false at the top. In FE, cloud full-table drops of NGRAM_BF/INVERTED indexes only call buildOrDeleteTableInvertedIndices when that session variable is true; with it false, this drop follows the normal schema-change path that the column wait above already observes. The strict helper filters out previous_job_ids and requires a nonempty new job set to finish, so the cloud branch can now wait until timeout for jobs that should not be created. Please keep only the column/schema wait here, or guard the build-index wait on the mode that actually creates IndexChangeJobs.
There was a problem hiding this comment.
Fixed in 7e4bb8f. Because this suite sets enable_add_index_for_new_data=false, the drop now keeps only the column/schema wait and no longer expects new build-index rows.
### What problem does this PR solve? Issue Number: None Related PR: apache#65453 Problem Summary: Static review found two suite-local wait helpers that could still pass at the timeout boundary, a Cloud two-drop sequence that did not wait for the first delete jobs, and a Cloud branch that expected delete jobs while enable_add_index_for_new_data was disabled. Reuse the strict shared job-ID wait for explicit builds, make the partition helper require explicit completion and reject cancellation, serialize both Cloud drops, and keep the disabled-mode drop on the schema wait only. ### Release note None ### Check List (For Author) - Test: Manual test - Groovy syntax compilation for all changed suites - Shared helper behavior harness - git diff --check - Behavior changed: No - Does this need documentation: No
|
/review |
|
run buildall |
There was a problem hiding this comment.
Requesting changes for one remaining regression-wait issue in the shared helper.
Critical checkpoint conclusions:
- Goal/test proof: the PR fixes the changed call sites from the prior review, but the shared helper change still regresses at least one existing caller outside the changed-file list.
- Scope/focus: the patch is otherwise focused on regression wait helpers and affected suites.
- Async lifecycle/concurrency: the relevant lifecycle is FE
IndexChangeJobcreation/completion. FE creates build/delete jobs synchronously before DDL returns, but callers still need to wait the correct job set. - Cloud/non-cloud and config paths: reviewed
enable_add_index_for_new_datatrue/false, cloud and non-cloud full-table drops, schema-only add/drop, explicitBUILD INDEX, and partition drops. The remaining issue is the default shared-helper path used by a multi-partition NGRAM_BF suite and schema-only paths. - Test coverage/results: static review only, per the review prompt; I did not run builds or regression tests.
- Existing thread dedupe: existing review threads were read. This is a distinct remaining shared-helper caller after the latest fixes, not a duplicate of the already-fixed changed-file comments.
- User focus: no additional user-provided review focus was present.
- Subagent convergence: round 2 normal and risk-focused subagents both returned
NO_NEW_VALUABLE_FINDINGSfor the currentFINAL-1comment set. The review completed within 2 rounds.
| def alter_res = [] | ||
| def confirmed_finished_job_ids = null | ||
| for (int t = 0; t <= OpTimeout; t += delta_time) { | ||
| if (previous_job_ids == null) { |
There was a problem hiding this comment.
The default previous_job_ids == null path is still reachable from shared-plugin callers that this PR did not update. For example, regression-test/suites/index_p0/test_ngram_bloomfilter_index_change.groovy still calls wait_for_last_build_index_finish(tableName, timeout) directly after NGRAM_BF add/drop waits on a three-partition table. In the schema-change cases (enable_add_index_for_new_data=false, and non-cloud full-table NGRAM_BF drops), FE does not create new IndexChangeJob rows, so this stricter helper now waits until timeout on an empty SHOW BUILD INDEX. In the cloud true-mode drop case, FE creates one delete job per partition, but this branch still checks only ORDER BY CreateTime DESC LIMIT 1, so a latest finished row can hide sibling jobs. Please either update the remaining direct callers to use the snapshot/all-new-job path only when jobs are expected, or split this helper so schema-only callers do not use the build-index wait.
There was a problem hiding this comment.
Fixed in 773e4e2. test_ngram_bloomfilter_index_change now snapshots existing JobIds before the Cloud true-mode three-partition drop and waits for all new delete jobs. Its schema-only and empty-table add/drop paths use only the shared column wait, and the duplicate local column helper with the old timeout-success assertion was removed.
|
/review |
|
run buildall |
There was a problem hiding this comment.
I found four blocking issues in the regression wait changes.
Review completeness: incomplete by the review protocol. Round 3 produced an additional valuable candidate, and the prompt caps the process at three rounds. I verified and included that candidate as an inline comment, but the final subagent convergence state is not all NO_NEW_VALUABLE_FINDINGS for the final M1-M4 set.
Critical checkpoints: the PR goal is to make index-change waits reliable, but the current patch still misses non-cloud full-table INVERTED delete jobs and leaves stale SHOW BUILD INDEX row hazards in local helpers. The changed scope is focused on regression/plugin Groovy tests; no production code was changed. The relevant lifecycle/concurrency concern is asynchronous IndexChangeJob creation and completion after metadata/column jobs. No FE/BE protocol or persisted-format compatibility change is introduced by this PR. I did not run regression tests in this review-only runner; validation was static against the authoritative diff, existing review threads, and FE SchemaChangeHandler/BuildIndexProcDir code paths.
User focus: no additional user-provided focus was supplied.
| def finished = false | ||
| def alter_res = [] | ||
| for (int t = 0; t <= OpTimeout; t += delta_time) { | ||
| alter_res = sql """SHOW BUILD INDEX WHERE TableName = "${table_name}";""" |
There was a problem hiding this comment.
This still scopes the wait to every retained SHOW BUILD INDEX row with the same table name. SHOW BUILD INDEX exposes JobId, TableName, partition and state, but not the table id, and finished/cancelled IndexChangeJobs are kept for history_job_keep_max_second, 7 days by default. Since this suite reuses fixed names after DROP TABLE IF EXISTS, a stale CANCELLED or still-running row from a previous run can make this helper fail or time out even after the current DROP INDEX ... PARTITION jobs finish. Please snapshot the job ids before each partition drop and wait only for rows not in that snapshot.
There was a problem hiding this comment.
Fixed in f73a0a4. Each successful partition-drop path now snapshots existing JobIds before the DDL and reuses the shared strict wait, so retained rows from earlier tables or runs are excluded. The local table-wide helper was removed.
| sql "drop index name_idx_2 on ${indexTbName1}" | ||
| wait_for_last_build_index_finish(indexTbName1, timeout) | ||
| wait_for_last_col_change_finish(indexTbName1, timeout) | ||
| if (isCloudMode()) { |
There was a problem hiding this comment.
This guard also needs to cover the non-cloud INVERTED drop. name_idx_1 is NGRAM_BF, so the non-cloud first drop is schema-change only, but name_idx_2 is INVERTED; in non-cloud FE sets supportLightIndexChange for full-table INVERTED drops and modifyTableLightSchemaChange enqueues delete IndexChangeJobs. With the wait inside if (isCloudMode()), the non-cloud run can continue to the later same-table CREATE INDEX cases while the name_idx_2 delete job is still running. Please snapshot and wait for the new build-index/delete rows for this second drop in non-cloud mode too.
There was a problem hiding this comment.
Fixed in f73a0a4. The second INVERTED drop now snapshots existing JobIds in every mode and waits for the new delete jobs in both Cloud and non-Cloud runs. The first NGRAM_BF drop remains Cloud-only for the build-index wait.
| // drop inverted index on renamed column | ||
| sql """ alter table ${tableName} drop index idx_s; """ | ||
| wait_for_last_build_index_finish(tableName, timeout) | ||
| wait_for_last_col_change_finish(tableName, timeout) |
There was a problem hiding this comment.
Keeping only the column wait is right for the cloud false-mode case from the earlier thread, but this suite also runs the drop in non-cloud mode after building idx_s. In FE, full-table INVERTED drops in non-cloud still take the light-index path: SchemaChangeHandler sets supportLightIndexChange for INVERTED/ANN when Config.isNotCloudMode(), and modifyTableLightSchemaChange calls buildOrDeleteTableInvertedIndices for drops. That enqueues delete IndexChangeJobs after the metadata/column job, so the non-cloud run can finish this suite without waiting for the physical delete job it just submitted. Please keep this column-only for cloud false-mode, but also snapshot and wait for the new build-index/delete rows when the full-table drop is a non-cloud INVERTED drop.
There was a problem hiding this comment.
Fixed in f73a0a4. The renamed-column suite now snapshots JobIds and waits for new delete jobs for the non-Cloud INVERTED drop, while the Cloud false-mode path keeps only the column wait.
| } | ||
| } | ||
| if (finished_num == expected_finished_num) { | ||
| if (!alter_res.isEmpty() && finished_num == expected_finished_num) { |
There was a problem hiding this comment.
The V1 branch still does not snapshot before its explicit BUILD INDEX. Earlier on the same test_index_change_4_v1 table, the test drops two inverted indexes and waits for those delete jobs to finish. This helper then succeeds whenever SHOW BUILD INDEX WHERE TableName = ... is nonempty and every returned row is FINISHED, so if the later build_index_on_table("idx_note", tableName) path ever fails to create a new job row, the already-finished drop rows are enough for the wait to pass. Please mirror the V2 branch by capturing get_build_index_job_ids(tableName) immediately before the build and using the snapshot-filtered shared wait.
There was a problem hiding this comment.
Fixed in f73a0a4. The V1 explicit build now snapshots existing JobIds immediately before BUILD INDEX and uses the shared snapshot-filtered strict wait. The table drop helper also snapshots in both modes.
|
/review |
|
run buildall |
|
/review |
|
run buildall |
There was a problem hiding this comment.
Requesting changes for two remaining regression-test reliability gaps on head 1814da5680ef5902ac003f2616cc227183f49ab1.
Critical checkpoint conclusions:
- Goal and proof: the PR improves asynchronous index-job waits, but it does not yet prove the goal. The strict snapshot helper can reject a successful final poll, and three positive partition-drop cases can pass without ever creating the physical index files they claim to delete.
- Scope: the patch remains focused on shared Groovy helpers and affected regression suites.
- Concurrency and lifecycle: no production concurrency code changes. I traced FE
IndexChangeJobregistration and execution: sibling jobs are registered synchronously before successful DDL return, then complete asynchronously. No new locks, threads, static initialization, ownership, memory-safety, or BE nullable concerns apply. - Error handling and data correctness:
CANCELLEDand ordinary timeout paths otherwise fail loudly with the latest result. The two inline findings cover the remaining deadline false failure and false-green physical-deletion coverage. - Configuration and parallel paths: no configuration item is added. Cloud/non-cloud behavior and
enable_add_index_for_new_databranches were checked across schema-only changes, explicitBUILD INDEX, full-table drops, and partition drops. Two additional non-cloud full-table drop omissions remain covered by existing discussions and are not duplicated here. - Compatibility and interfaces: the latest commit cleanly separates optional legacy and strict snapshot APIs; a repository-wide symbol audit found no stale helper references. No storage format, rolling-upgrade, FE/BE protocol, persistence, transaction, or data-write implementation change is introduced.
- Tests and results: static review only, as required by the review prompt; no build or regression suite was run. No expected-result file changed. The current tests lack proof of physical file creation before the positive partition-deletion cases.
- Observability: latest-result timeout messages are an improvement and otherwise sufficient for these regression helpers; no production metric change is needed.
- Performance: the strict helper adds at most one one-second confirmation poll; no other material CPU, memory, or complexity issue was found.
- Other issues: none beyond the two inline findings and already-existing discussions.
- User focus: no additional user-provided review focus was present.
Review completeness: three subagent rounds converged with NO_NEW_VALUABLE_FINDINGS on the prior head and exact two-comment set. The PR head then moved after the third and final allowed round. I fetched and fully re-read the new 622-line diff, revalidated both findings and anchors, audited renamed helper references, and found no additional candidate. Under the three-round cap, same-head subagent convergence for this late head is therefore incomplete, although all substantiated current-head findings are included.
| } else { | ||
| confirmed_finished_job_ids = null | ||
| } | ||
| if (t >= OpTimeout) { |
There was a problem hiding this comment.
The snapshot path still produces a false timeout at the deadline. If the new jobs are first observed all FINISHED on the t == OpTimeout poll, confirmed_finished_job_ids is only initialized; this branch immediately breaks, and the final assertion fails despite the last permitted query having observed success. FE registers every sibling IndexChangeJob synchronously before a successful DDL returns, so please either accept the nonempty all-finished set immediately or allow/reserve the confirmation poll beyond the deadline.
| // create inverted index | ||
| sql "CREATE INDEX idx_v2 ON ${tableName1}(v2) USING INVERTED" | ||
| wait_for_last_build_index_finish(tableName1, timeout) | ||
| wait_for_last_col_change_finish(tableName1, timeout) |
There was a problem hiding this comment.
In the non-cloud run, these positive cases insert rows before CREATE INDEX, but local INVERTED add is a light metadata change and does not backfill files; this changed column wait therefore proceeds to DROP INDEX ... PARTITION without ever issuing BUILD INDEX. BE then treats a drop with no candidate rowsets as successful, and the fallback queries cannot distinguish that no file was deleted. Please explicitly build and wait in non-cloud before taking each pre-drop snapshot (and ideally assert the index file exists), so cases 1, 2, and 10 exercise physical partition deletion.
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
There was a problem hiding this comment.
Automated review summary for head 1814da5680ef5902ac003f2616cc227183f49ab1.
Outcome: no new inline comments were added after deduplication. The current head still has substantiated issues, but they are already precisely covered by existing review context: the strict helper's final-poll false timeout (thread), the non-Cloud partition-drop cases that never build physical index files before testing deletion (thread), the two exact non-Cloud full-table drop wait gaps (test_add_drop_index thread, variant thread), and the remaining compatibility-helper caller contract (helper threads, caller-audit thread). Those threads remain the authoritative places to resolve the concerns; this review does not restate them inline.
Critical checkpoint conclusions:
- Goal and proof: the PR materially improves false-success handling, operation-scoped JobId snapshots, cancellation detection, and failure diagnostics, but the goal is not fully proved until the already-commented final-deadline bug and physical partition-deletion coverage gap are fixed. Existing suite paths exercise the helpers, but no new automated helper test was added.
- Scope and clarity: the patch is focused on the shared Groovy helper and the affected index-change suites. The strict versus compatibility helper split is understandable, though remaining direct compatibility callers are already tracked in prior threads.
- Concurrency: no production concurrency primitive changes. FE registers every selected-partition
IndexChangeJobsynchronously before a successful DDL returns, while execution completes asynchronously; the snapshot correctly scopes retained rows. No new lock-order or deadlock risk was introduced. - Lifecycle and memory safety: terminal job-history retention safely spans the strict helper's confirmation interval, and partial registration failure makes the DDL fail before a post-operation wait. No new static initialization, ownership, memory-tracking, nullable-column, or resource-lifecycle concern applies.
- Configuration: no configuration item was added. Existing Cloud/non-Cloud routing and
enable_add_index_for_new_datatrue, false, and default paths were checked. - Compatibility and interfaces: no storage format, rolling-upgrade, symbol, FE/BE protocol, persistence, or transmitted-variable change is introduced. The legacy two-argument helper remains callable, with its known limitations already covered by existing threads.
- Parallel and conditional paths: explicit builds, schema-only adds/drops, light full-table drops, partition drops, Cloud/non-Cloud modes,
NGRAM_BF, andINVERTEDpaths were traced. The remaining conditional-routing omissions are exact-thread duplicates and were not re-commented. - Test coverage and results: positive/negative partition cases, deterministic query ordering, and expected-error patterns remain intact, but the existing live comment correctly shows that non-Cloud positive partition-delete cases do not prove physical deletion. No expected-result file changed. Static review only: the review contract prohibited builds and test execution, and this checkout lacks
.worktree_initializedandthirdparty/installed/bin/protoc. - Error handling and observability:
CANCELLEDjobs and ordinary timeouts fail loudly, and latest-result messages improve diagnosis. The final permitted poll still has the already-commented false-negative edge. No production metric change is needed for this test-only patch. - Transactions, persistence, and data writes: no production transaction, edit-log, visibility, delete-bitmap, or data-write implementation is modified.
- Performance: the strict path adds at most one one-second confirmation poll and table-scoped
SHOW BUILD INDEXpolling; no material CPU, memory, or algorithmic regression was found. - Other concerns: no additional correctness, syntax/scope, result-file, security, or documentation issue survived review. No extra user-provided focus was present.
Review completeness: three real subagent rounds covered the shared helper, all nine changed files, all reachable helper callers, and focused FE/BE routing. In the final round, both normal full-review agents and the risk-focused agent returned NO_NEW_VALUABLE_FINDINGS for this exact head, ledger, live 37-comment set, and empty new-inline set.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated review completed for head 25e5b66c45a0fa49bba05ca598748a11792bd404.
Outcome: requesting changes for two regression-test reliability gaps: terminal column-job cancellation is not handled fail-fast, and one custom-analyzer path no longer proves that local physical index files were built.
Critical checkpoint conclusions:
- Goal and proof: the operation-scoped JobId snapshot design fixes stale-history and sibling-job ambiguity for explicit build/delete operations, and the current deadline ordering accepts success on the final poll. The goal is not fully proved while the two inline findings remain: cancelled column jobs can hide their cause behind a five-minute delay, and the custom-analyzer MATCH assertions can pass through default fallback without a local index file.
- Scope and clarity: the patch is test-only and focused on the shared Groovy helper plus affected callers. The broad caller migration is justified by removing the legacy helper, and a repository-wide exact-symbol search found no stale references.
- Concurrency: FE connection threads register every selected-partition
IndexChangeJobsynchronously before successful DDL return; schema-change worker threads execute them asynchronously. The pre-operation JobId set therefore contains only prior history and the first nonempty all-FINISHEDnew set is complete. No production lock, atomic, deadlock, or thread-context change is introduced. - Lifecycle and memory safety: retained job history, terminal states, table-name reuse, and periodic expiry were checked. No new static initialization, ownership, allocator, nullable-column, or resource-lifetime concern applies to this test-only patch.
- Configuration: no configuration item is added or changed.
enable_add_index_for_new_data, Cloud/non-Cloud routing, parser mode, and default missing-index fallbacks were traced at every modified caller. - Compatibility and interfaces: no storage format, rolling-upgrade, FE/BE protocol, persistence format, or transmitted-variable change is present. The removed regression-plugin method has no remaining repository caller.
- Parallel and conditional paths: explicit builds, schema-only changes, light full-table drops, partition drops,
INVERTED,NGRAM_BF, Cloud table-scoped builds, local named builds, and true/false/default session-variable paths were checked. Apart from the two inline findings, the selected wait matches the job type actually created. - Tests and results: positive and negative paths, deterministic result ordering, and expected-error patterns remain intact; no
.outresult change is needed for synchronization-only edits. Static review only: the review contract prohibited builds and test execution, so no regression suite was run in this review. - Error handling and observability: build-job cancellation and timeouts fail loudly with current rows. MAIN-001 covers the remaining column-job cancellation delay; no production metric change is needed.
- Transactions, persistence, and data writes: no production transaction, EditLog, visibility, delete-bitmap, or data-write implementation is modified.
- Performance: polling is bounded and test-only, but repeatedly querying a terminally cancelled column job for up to 300 seconds is avoidable and is covered inline. No production hot-path regression was found.
- Other concerns and user focus: existing discussions already cover the current partition physical-file/history concerns and were not duplicated. No additional user-provided focus was present.
Review completeness: the main pass read all 18 changed files and relevant FE/BE control flow. Two normal full-review agents plus a separate risk-focused agent converged on the current two-comment set after reconciliation; the live head and 37-comment set were refreshed immediately before freezing this payload.
| for (int t = 0; t <= OpTimeout; t += delta_time) { | ||
| alter_res = sql """SHOW ALTER TABLE COLUMN WHERE TableName = "${table_name}" ORDER BY CreateTime DESC LIMIT 1;""" | ||
| alter_res = alter_res.toString() | ||
| if (alter_res.contains("FINISHED")) { |
There was a problem hiding this comment.
The column waiter still treats a terminal CANCELLED job like a transient state. Once the latest column job is cancelled, every caller keeps issuing SHOW and sleeping until its full timeout; test_index_change_4 passes 300000 here, so the real failure can be delayed by five minutes. Please fail immediately on the exact cancelled state and include the current row/error, as the new build-index waiter already does.
|
|
||
| sql """ alter table ${indexTbName1} add index idx_ch_default(`ch`) using inverted; """ | ||
| wait_for_last_build_index_finish("${indexTbName1}", 60000) | ||
| wait_for_last_col_change_finish("${indexTbName1}", 60000) |
There was a problem hiding this comment.
In non-Cloud mode an INVERTED add is a light metadata change, so the row inserted above is not backfilled by either of these changed column waits. Because missing-index MATCH fallback is enabled by default, the assertions below can scan and pass without ever exercising a custom-analyzer index file. This is a separate instance from the partition-drop thread: please explicitly BUILD INDEX and snapshot/wait for both new indexes in non-Cloud before running these MATCH queries; the Cloud schema-change path can keep the column wait.
2 participants
What problem does this PR solve?
Issue Number: DORIS-24947
Related PR: #65197
Problem Summary:
Cloud P0
test_index_change_4can spend the full 60-second timeout waiting for an asynchronous index-change job. The sharedwait_for_last_build_index_finishhelper previously asserteduseTime <= OpTimeout, which was still true at the timeout boundary even whenFINISHEDhad never been observed. The case then submitted a secondDROP INDEXwhile the first job was unfinished and failed withpartition test_index_change_4 has been built specified index.This change makes the shared build-index and column-change helpers, plus the suite-local V1 partition helper, succeed only after explicitly observing
FINISHED. The helpers avoid an extra sleep after the deadline and include the latest query result in timeout failures. It also raises the affected case timeout to 300 seconds, matching the Cloud-safe wait used bytest_index_change_6.The shared schema-change wrapper now waits only for the column/schema job. Callers that explicitly submit
BUILD INDEXwait for both the column job and the build-index job, preserving their previous sequencing without making schema-only callers require a nonexistent build-index row.Schema-only callers now use the column-change wait. Callers that explicitly build or delete index data snapshot the table's existing build-index JobIds before the operation, then wait for a nonempty, stable set of new jobs. Every new partition job must reach
FINISHED; any newCANCELLEDjob fails immediately, and timeout failures include the latest new-job rows.The follow-up also removes the same timeout-success behavior from the remaining suite-local partition helper, serializes consecutive Cloud drops with independent JobId snapshots, and avoids waiting for delete jobs in a suite that explicitly disables index construction for new data.
The NGRAM bloom-filter lifecycle suite now follows the same contract: its Cloud three-partition drop snapshots and waits for all new delete jobs, while schema-only and empty-table index changes wait only for the column job. Its duplicate local column helper with the old timeout-success assertion was removed.
The final caller audit scopes partition drops, non-Cloud full-table INVERTED drops, and the V1 explicit build to JobIds created by the current operation. Retained jobs from an earlier table with the same name can no longer satisfy or fail these waits.
The plugin API now makes the two contracts explicit. The legacy two-argument helper remains a compatibility path for callers where a build-index job is optional and observes only the latest row. Strict build/delete paths use
snapshot_build_index_job_idsbefore the operation andwait_for_new_build_index_jobs_finishafterward, which requires a nonempty stable set of new jobs and checks every new row.Release note
None
Check List (For Author)
Test
test_index_change_4.RUNNING -> FINISHEDand timeout paths.git diff --checkpassed.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)