github-actions[bot] commented on code in PR #65453:
URL: https://github.com/apache/doris/pull/65453#discussion_r3586653941
##########
regression-test/suites/inverted_index_p0/test_create_index_2.groovy:
##########
@@ -108,9 +108,13 @@ suite("test_create_index_2", "inverted_index"){
// drop index
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
Review 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
`IndexChangeJob`s 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.
##########
regression-test/suites/inverted_index_p0/index_change/test_index_change_on_renamed_column.groovy:
##########
@@ -100,8 +100,12 @@ suite("test_index_change_on_renamed_column") {
check_nested_index_file(ip, port, tablet_id, 3, 1, "V3")
// drop inverted index on renamed column
+ 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)
+ if (isCloudMode()) {
+ wait_for_last_build_index_finish(tableName, timeout, previous_job_ids)
Review 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 `IndexChangeJob`s.
##########
regression-test/suites/inverted_index_p0/index_change/test_drop_index_on_partition.groovy:
##########
@@ -76,7 +76,7 @@ suite("test_drop_index_on_partition", "inverted_index") {
// 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)
Review 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.
##########
regression-test/suites/inverted_index_p0/index_change/test_index_change_on_renamed_column.groovy:
##########
@@ -100,8 +100,12 @@ suite("test_index_change_on_renamed_column") {
check_nested_index_file(ip, port, tablet_id, 3, 1, "V3")
// drop inverted index on renamed column
+ 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)
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
