Repository navigation
[fix](fe) Treat an unfinished index change job of the same index as a conflict in either direction - #69443
Merged
gavinchou merged 1 commit intoOct 25, 2026
Conversation
Member
Author
|
run buildall |
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
/reivew |
Contributor
TPC-H: Total hot run time: 28738 ms |
Contributor
TPC-DS: Total hot run time: 151358 ms |
Contributor
ClickBench: Total hot run time: 24.45 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Member
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review of #69443 at 617e004 found no new verified defect. All three changed files and the relevant DDL, scheduling, task, replay and test paths were reviewed. There are no existing inline findings to retain and no additional user-provided focus points.
Critical checkpoints:
- Goal and evidence: unfinished same-index jobs now conflict in either direction. The added test covers pending BUILD blocking both full-table DROP and partition DROP, retained index metadata, and successful DROP after FINISHED. This is static inspection of the test; it was not executed.
- Scope and reuse: the change is limited to the existing index-ID overlap predicate, its callers and one focused test; no competing conflict implementation is introduced.
- Concurrency and locks: SchemaChangeHandler.process holds the table write lock across checking, metadata changes and job publication. Concurrent-map snapshots retain safe traversal. No new locks, lock-order changes, RPCs or blocking work are added.
- Lifecycle: job scheduling, synchronized run/cancel, task completion, terminal-state exclusion and replay are preserved. The synthetic test job is kept out of the runnable map and removed in finally.
- Configuration: no additions or changes.
- Compatibility: all callers of the Java-internal method/signature changes are migrated; serialized job fields and storage formats are unchanged.
- Thrift/protobuf: no definition changes or new field IDs.
- Parallel paths: BUILD, partition DROP and full-table light DROP use the shared predicate; local conflicts remain scoped to matching index IDs and partitions, and cloud mode retains its broader per-partition conflict policy.
- Conditions and errors: index identity and job-state filters remain justified; the new comment explains the ordering hazard. Conflicts propagate the existing DDL error rather than silently succeeding.
- Test coverage: the added deterministic JUnit test covers the motivating negative cases and FINISHED retry. Reverse-direction admission, distinct-index/partition independence, CANCELLED and cloud behavior were traced statically; the added test does not exercise those or BE task ordering.
- Test results: no expected-output files changed. No builds or tests were run, as required by this review's static-only contract.
- Observability: existing conflict errors and job logs are sufficient for this predicate change; no new operational state requires metrics.
- Transactions and persistence: transaction watershed handling and the existing creation/completion journal and replay paths are unchanged; no new failover state is introduced.
- Writes and atomicity: full-table DROP preflight runs before metadata mutation. Partition-scoped operations retain the preexisting per-partition submission loop, including possible earlier job publication before a later conflict; this was independently checked and is not a new defect in this change.
- FE/BE propagation: no new transmitted variables; existing task serialization is unchanged.
- Performance: the map snapshot and nested index-ID comparison retain their existing complexity; opposite-direction comparisons are necessary for the intended behavior.
- Other core invariants: no changed committed-data visibility, visible-version handling, delete bitmap behavior, BE memory ownership or nullable handling was identified. No additional new issue was verified.
Coverage limits: review was static only; no runtime concurrency, FE/BE integration, build or test-result validation is claimed.
Review converged after 2 round(s).
gavinchou
previously approved these changes
Oct 24, 2026
… conflict in either direction Issue Number: None Related PR: apache#68674 Problem Summary: In non-cloud mode an unfinished index change job only conflicted with a new one on the same partition when both went in the same direction, so an unfinished `BUILD INDEX idx` did not block `DROP INDEX idx` (full-table or PARTITION), and the other way round. The two jobs run in no particular order (the BE tablet locks only make them mutually exclusive), so a build running after the drop writes the dropped index back into the rowsets although the table no longer has it. Fix: IndexChangeJob#hasAlterInvertedIndex matches a job of any of the indexes in either direction, and SchemaChangeHandler#hasIndexChangeJobOnPartition no longer takes a direction. The light DROP INDEX pre-check, DROP INDEX ... PARTITION and BUILD INDEX all use it. In non-cloud mode DROP INDEX and BUILD INDEX are rejected while an index change job of the same index in the other direction is unfinished on the partition. - Test: Unit Test - New testDropIndexRejectedByPendingBuildOfSameIndex fails without the change and passes with it. - SchemaChangeHandlerTest, CreateTableWithBloomFilterIndexTest, IndexChangeJobTest, CloudIndexTest, SchemaChangeJobV2Test, ChangelogConfigPublicationTest, OlapTableChangelogSchemaTest, VariantColumnNameTest, InternalSchemaAlterTest: 176 tests pass; `mvn checkstyle:check -pl fe-core` reports 0 violations. - Behavior changed: Yes, see release note. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
airborne12
force-pushed
the
fix-drop-index-blocked-by-pending-build
branch
from
October 24, 2026 08:52
617e004 to
95c9d4e
Compare
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review of PR #69443 at 95c9d4e: no substantiated newly introduced defects. The shared conflict predicate now excludes active BUILD and DROP jobs for the same index in either direction. Review converged in 2 rounds with two complete reviewers and one separate risk reviewer.
Critical checkpoint conclusions:
- Goal and evidence: both full-table DROP preflight and partition BUILD/DROP dispatch use the corrected predicate. The new JUnit case checks both DROP forms against a pending BUILD, retained index metadata, and successful DROP after FINISHED; inspected, not executed.
- Scope and reuse: focused change to one shared predicate and its callers, plus a regression test; no stale references to the removed helper/signatures found.
- Concurrency and locks: DDL admission and job publication remain under the table write lock; scheduler workers and user cancellation retain the existing job monitor. No new lock order, blocking work, RPC, or journal wait is introduced by the predicate.
- Lifecycle: identity/index lists are populated before history and runnable-map publication. WAITING_TXN/RUNNING block; FINISHED/CANCELLED permit retries. Cancellation semantics and static initialization are unchanged.
- Configuration: no added settings or dynamic-configuration requirements.
- Compatibility: internal helper signatures have updated callers; serialized fields and storage/request formats remain unchanged.
- Thrift/protobuf IDs: no definition changes.
- Parallel paths: full-table and partition routes are covered, including reverse BUILD-after-partition-DROP. Separate partitions/indexes retain existing local-mode behavior; cloud retains its broader per-partition exclusion.
- Conditions and errors: the new comment explains the unordered build/drop hazard. The existing conflict exceptions still reject the operation; full-table preflight precedes metadata mutation.
- Test coverage: the deterministic history-only fixture cannot be scheduled or time out, and cleanup/job accounting match surrounding tests. Reverse direction, RUNNING/CANCELLED, independent-scope and replay admission cases are statically traced but lack new dedicated tests.
- Test results: no expected-result/output files change; no build or test result is claimed.
- Observability: existing conflict errors, job logs and SHOW job state cover this admission change; no new asynchronous mechanism needs instrumentation.
- Transactions and persistence: existing transaction watershed, EditLog records and replay registration remain unchanged; replayed active jobs enter the history used by admission.
- Writes, atomicity and crashes: full-table rejection precedes metadata publication. The existing incremental partition scheduling and dispatched-task cancellation behavior remain outside this fix; no new recovery or resource-ownership path is added.
- FE/BE variables: no new transferred fields; operation direction is still sent independently to BE.
- Performance: retained-job scans and index-ID overlap retain the existing complexity; no new data-path allocation or avoidable expensive work identified.
- Other invariants: no modified transaction visibility, partition visible-version, delete-bitmap or BE memory-accounting logic. No additional introduced issue substantiated after upstream/downstream rechecks.
There were no existing inline threads to revalidate and no additional user-provided focus points. All three changed files and identified risks were covered. Remaining limits: this was static review only; builds, unit/regression tests, runtime concurrency and failover were not exercised. No unresolved patch-specific candidate remains.
Review converged after 2 round(s).
Contributor
TPC-H: Total hot run time: 28545 ms |
Contributor
TPC-DS: Total hot run time: 150594 ms |
Contributor
ClickBench: Total hot run time: 24.24 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
gavinchou
approved these changes
Oct 25, 2026
airborne12
added a commit
to airborne12/apache-doris
that referenced
this pull request
Oct 26, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#69444, apache#69442, apache#69443 Problem Summary: Merge master into the PR branch. Keep the original heavy schema change fallback and its test, together with the upstream pending-build and partition-scoped index drop checks and tests. Both sides added tests at the same position in SchemaChangeHandlerTest. ### Release note None ### Check List (For Author) - Test: fe-core Checkstyle passed. SchemaChangeHandlerTest and IndexChangeJobTest are running; their final results will be checked before pushing. - Behavior changed: No additional behavior change from resolving the conflict. - Does this need documentation: No.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: #68674
Problem Summary:
In local (non-cloud) mode, an unfinished index change job only conflicts with a new one on the same partition when both go in the same direction. An unfinished
BUILD INDEX idxtherefore does not blockDROP INDEX idx(full-table orPARTITION), and an unfinishedDROP INDEX idx PARTITION (p)does not blockBUILD INDEX idxonp:The two jobs are submitted to the schema change thread pool independently, and the BE tablet locks only make them mutually exclusive, not ordered. If the build runs after the drop, it writes
idx_v1back into the rowsets, although the table metadata no longer has that index (or, for the opposite order, the index the user just built is deleted again). Cloud mode already treats any unfinished job on the partition as a conflict.Root cause:
SchemaChangeHandler#hasIndexChangeJobOnPartitionmatches existing jobs in local mode withIndexChangeJob#hasSameAlterInvertedIndex(isDrop, indexes), which only reports a job of the same direction.Fix:
IndexChangeJob#hasAlterInvertedIndex(indexes)matches a job that builds or drops any of the indexes, in either direction, andhasIndexChangeJobOnPartitiondrops its direction parameter. The lightDROP INDEXpre-check,DROP INDEX ... PARTITIONandBUILD INDEXall use this check, so each of them is rejected while a job of the same index is unfinished on the partition, with the existing error message; it can be retried once the job finishes.Release note
In non-cloud mode,
DROP INDEXandBUILD INDEXare rejected while an index change job of the same index in the other direction is unfinished on the partition, instead of running both jobs in no particular order.Check List (For Author)
Test
SchemaChangeHandlerTest#testDropIndexRejectedByPendingBuildOfSameIndexregisters an unfinished build job ofidx_v1onp1(inindexChangeJobsonly, so the scheduler cannot cancel it), checks thatDROP INDEX idx_v1andDROP INDEX idx_v1 PARTITION (p1)are rejected and keep the index, then that the drop succeeds once the job is FINISHED. The test fails without the change.Behavior changed:
In non-cloud mode an unfinished index change job conflicts with a new job of the same index on the same partition in either direction.
Does this need documentation?
Check List (For Reviewer who merge this PR)
🤖 Generated with Claude Code