github-actions[bot] commented on code in PR #65453:
URL: https://github.com/apache/doris/pull/65453#discussion_r3600113137
##########
regression-test/suites/index_p0/test_ngram_bloomfilter_index_change.groovy:
##########
@@ -306,8 +289,7 @@ suite("test_ngram_bloomfilter_index_change") {
// Add ngram bf index before data insertion
sql "ALTER TABLE ${tableName} ADD INDEX
idx_ngram_customer_name(customer_name) USING NGRAM_BF PROPERTIES('bf_size' =
'1024', 'gram_size' = '3');"
- wait_for_latest_op_on_table_finish(tableName, timeout)
- wait_for_last_build_index_finish(tableName, timeout)
+ wait_for_last_col_change_finish(tableName, timeout)
Review Comment:
The strict migration still stops before Case 3's final drop at current lines
320-322. This branch has `enable_add_index_for_new_data=true`: in Cloud,
dropping this three-partition NGRAM_BF index submits asynchronous delete
`IndexChangeJob`s, while non-Cloud routes the drop through an asynchronous
column/schema-change job. A fixed two-second sleep proves neither path reached
`FINISHED` and hides a later `CANCELLED`, so the lifecycle suite can finish
with background work still active. This is distinct from the earlier thread
about the Case 1 direct helper call. Please use the same column wait in both
modes and pre-drop snapshot/all-new-job wait in Cloud here too.
##########
regression-test/suites/inverted_index_p0/index_change/test_index_change_3.groovy:
##########
@@ -75,15 +75,18 @@ suite("test_index_change_3") {
sql """ CREATE INDEX idx_city ON ${tableName}(`city`) using inverted
properties("support_phrase" = "true", "parser" = "english", "lower_case" =
"true") """
wait_for_last_col_change_finish(tableName, timeout)
if (!isCloudMode()) {
Review Comment:
This local physical-build branch is still missing from the direct
`test_index_change_1` and `test_index_change_2` twins. Both insert all rows
before adding parser-bearing indexes in V2 and V1, then wait only for column
jobs. With `enable_add_index_for_new_data=true`, non-Cloud INVERTED adds are
metadata-only: `test_index_change_1` never builds its queried
`idx_note`/`idx_city`, and `test_index_change_2` never builds `idx_city`. Their
MATCH checks still pass through the default missing-index fallback. Cloud's
parser-bearing schema changes do backfill, so the gap is local-only. Please
mirror strict conditional builds in both branches of both suites and add
no-fallback probes for the queried indexes so these tests prove physical files
exist.
##########
regression-test/suites/inverted_index_p0/index_change/test_add_drop_index_with_data.groovy:
##########
@@ -261,8 +242,9 @@ suite("test_add_drop_index_with_data", "inverted_index"){
logger.info("show index from " + indexTbName1 + " result: " + show_result)
assertEquals(show_result.size(), 3)
- build_index_on_table("idx_name", indexTbName1)
- wait_for_build_index_on_partition_finish(indexTbName1, timeout)
+ run_index_change_job_and_wait(indexTbName1, timeout) {
Review Comment:
The final drop is now waited, but in the non-Cloud path it is still hollow
for `idx_id`. With `enable_add_index_for_new_data=true`, the multi-add above
only updates local metadata; this wrapper issues the index-scoped `BUILD INDEX
idx_name`, so only `idx_name` is backfilled. `idx_id` was physically deleted
earlier, is never rebuilt after being re-added, and the later three-index
delete can report `FINISHED` while treating its missing files as a no-op. The
name/description MATCH checks do not exercise `idx_id`. This is distinct from
the prior thread about waiting for the final delete jobs: please strictly build
both re-added indexes locally (Cloud can keep its table-scoped build) and add a
no-fallback pre-drop probe for `idx_id`.
##########
regression-test/plugins/plugin_index_change.groovy:
##########
@@ -23,22 +23,46 @@ import java.util.regex.Pattern;
def delta_time = 1000
-Suite.metaClass.wait_for_last_build_index_finish = {table_name, OpTimeout ->
- def useTime = 0
- for(int t = delta_time; t <= OpTimeout; t += delta_time){
- def alter_res = sql """SHOW BUILD INDEX WHERE TableName =
"${table_name}" ORDER BY CreateTime DESC LIMIT 1;"""
- alter_res = alter_res.toString()
- if(alter_res.contains("FINISHED")) {
- logger.info(table_name + " latest alter job finished, detail: " +
alter_res)
+Suite.metaClass.snapshot_build_index_job_ids = { table_name ->
+ def alter_res = sql """SHOW BUILD INDEX WHERE TableName =
"${table_name}";"""
+ return alter_res.collect { it[0].toString() }.toSet()
+}
+
+Suite.metaClass.wait_for_new_build_index_jobs_finish = { table_name,
OpTimeout, previous_job_ids ->
+ assertTrue(previous_job_ids != null, "previous build index job ids must be
snapshotted before the operation")
+ def finished = false
+ def alter_res = []
+ for (int t = 0; t <= OpTimeout; t += delta_time) {
+ def all_jobs = sql """SHOW BUILD INDEX WHERE TableName =
"${table_name}"
+ ORDER BY CreateTime DESC;"""
+ alter_res = all_jobs.findAll {
!previous_job_ids.contains(it[0].toString()) }
+
+ if (!alter_res.isEmpty() && alter_res.any { it[7] == "CANCELLED" }) {
+ logger.info(table_name + " build index job failed, detail: " +
alter_res)
+ assertTrue(false, "build index job cancelled, result:
${alter_res}")
+ }
+ if (!alter_res.isEmpty() && alter_res.every { it[7] == "FINISHED" }) {
+ // FE registers all sibling jobs before the DDL returns, so the
first
+ // nonempty all-FINISHED observation is complete, even at the
deadline.
+ logger.info(table_name + " build index jobs finished, detail: " +
alter_res)
+ finished = true
+ break
+ }
+ if (t >= OpTimeout) {
break
- } else if (alter_res.contains("CANCELLED")) {
- logger.info(table_name + " latest alter job failed, detail: " +
alter_res)
- assertTrue(false)
}
- useTime = t
sleep(delta_time)
}
- assertTrue(useTime <= OpTimeout, "wait for last build index finish
timeout")
+ assertTrue(finished, "wait for build index finish timeout, latest result:
${alter_res}")
+}
+
+// Run an operation that must create one or more IndexChangeJobs and wait for
+// every job created by that operation. SHOW BUILD INDEX history is retained,
so
+// the pre-operation snapshot is required to isolate the current operation.
+Suite.metaClass.run_index_change_job_and_wait = { table_name, OpTimeout,
Closure operation ->
Review Comment:
This shared replacement still leaves four in-scope sibling suites on the
original false-success waiter: `test_index_change_2` between four consecutive
drops, `test_index_change_with_cumulative_compaction` between three builds,
`test_add_drop_index_on_table_with_mv` after its final build, and
`test_pk_uk_index_change` after iterative builds/drops. In all four, a
`RUNNING` or `CANCELLED` row through `t == OpTimeout` still passes `useTime <=
OpTimeout`; three also accept empty history, while the MV helper instead
assumes retained history always has exactly one row. These paths can race the
next operation or finish without proving index files. The existing thread fixed
only `test_index_change_4`. Please migrate all four helpers to operation-scoped
waits; the cumulative Cloud branch should also mirror the changed
full-compaction twin's single table-scoped build.
--
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]
