github-actions[bot] commented on code in PR #65453:
URL: https://github.com/apache/doris/pull/65453#discussion_r3567754806
##########
regression-test/plugins/plugin_index_change.groovy:
##########
@@ -24,21 +24,30 @@ 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;"""
+ 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 to wait for")
Review 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 `CAN
CELLED`. 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.
##########
regression-test/plugins/plugin_index_change.groovy:
##########
@@ -51,25 +60,28 @@ Suite.metaClass.build_index_on_table = {index_name,
table_name ->
}
Suite.metaClass.wait_for_last_col_change_finish = { table_name, OpTimeout ->
- def useTime = 0
+ def finished = false
+ def alter_res = ""
- for (int t = delta_time; t <= OpTimeout; t += delta_time) {
- def alter_res = sql """SHOW ALTER TABLE COLUMN WHERE TableName =
"${table_name}" ORDER BY CreateTime DESC LIMIT 1;"""
+ 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")) {
sleep(3000) // wait change table state to normal
logger.info(table_name + " latest alter job finished, detail: " +
alter_res)
+ finished = true
+ break
+ }
+ if (t >= OpTimeout) {
break
}
- useTime = t
sleep(delta_time)
}
- assertTrue(useTime <= OpTimeout, "wait_for_last_col_change_finish timeout")
+ assertTrue(finished, "wait_for_last_col_change_finish timeout, latest
result: ${alter_res}")
}
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)
Review 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.
##########
regression-test/suites/inverted_index_p0/index_change/test_index_change_4.groovy:
##########
@@ -39,12 +39,15 @@ suite("test_index_change_4") {
if (finished_num == expected_finished_num) {
sleep(10000)
logger.info(table_name + " all build index jobs finished,
detail: " + alter_res)
+ finished = true
+ break
Review 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.
--
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]
