github-actions[bot] commented on code in PR #65453:
URL: https://github.com/apache/doris/pull/65453#discussion_r3592542593
##########
regression-test/suites/inverted_index_p0/index_change/test_drop_index_on_partition.groovy:
##########
@@ -76,27 +44,27 @@ 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:
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.
##########
regression-test/plugins/plugin_index_change.groovy:
##########
@@ -23,22 +23,73 @@ 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()
+}
+
+// Compatibility helper for legacy callers where a build-index job is optional.
+// New explicit build/delete paths must snapshot job ids before the operation
and
+// use wait_for_new_build_index_jobs_finish instead.
+Suite.metaClass.wait_for_last_build_index_finish = { table_name, OpTimeout ->
+ 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}"
+ ORDER BY CreateTime DESC LIMIT 1;"""
+ if (alter_res.isEmpty()) {
+ logger.info(table_name + " has no build index job")
+ finished = true
break
- } else if (alter_res.contains("CANCELLED")) {
- logger.info(table_name + " latest alter job failed, detail: " +
alter_res)
+ }
+ if (alter_res.any { it[7] == "CANCELLED" }) {
+ logger.info(table_name + " latest build index job failed, detail:
" + alter_res)
assertTrue(false)
}
- useTime = t
+ if (alter_res.every { it[7] == "FINISHED" }) {
+ logger.info(table_name + " latest build index job finished,
detail: " + alter_res)
+ finished = true
+ break
+ }
+ if (t >= OpTimeout) {
+ break
+ }
sleep(delta_time)
}
- assertTrue(useTime <= OpTimeout, "wait for last build index finish
timeout")
+ assertTrue(finished, "wait for latest build index finish timeout, latest
result: ${alter_res}")
+}
+
+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 = []
+ def confirmed_finished_job_ids = null
+ 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)
+ }
+ if (!alter_res.isEmpty() && alter_res.every { it[7] == "FINISHED" }) {
+ def finished_job_ids = alter_res.collect { it[0].toString()
}.toSet()
+ if (finished_job_ids == confirmed_finished_job_ids) {
+ logger.info(table_name + " build index jobs finished, detail:
" + alter_res)
+ finished = true
+ break
+ }
+ confirmed_finished_job_ids = finished_job_ids
+ } else {
+ confirmed_finished_job_ids = null
+ }
+ if (t >= OpTimeout) {
Review 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.
--
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]
