Skip to content

[fix](schema-change) Use actual schema change alter version in cloud capture - #62506

Merged
Yukang-Lian merged 2 commits into
apache:masterfrom
Hastyshell:fix-cloud-sc-alter-version-capture-20260415
Aug 10, 2026
Merged

[fix](schema-change) Use actual schema change alter version in cloud capture#62506
Yukang-Lian merged 2 commits into
apache:masterfrom
Hastyshell:fix-cloud-sc-alter-version-capture-20260415

Conversation

@Hastyshell

@Hastyshell Hastyshell commented Apr 15, 2026

Copy link
Copy Markdown
Collaborator

What problem does this PR solve?

Issue Number: None

Related PR: None

Problem Summary:

Cloud schema change in BE decides whether to capture historical rowsets using the original FE task alter version, but later executes and commits using the alter version registered in meta service. When the FE task is created at version 1 and the registered schema change job uses a later alter version, BE can skip capturing historical rowsets and then commit an empty schema change output.

This change uses the registered schema change alter version consistently when deciding whether historical rowsets need to be captured.

Release note

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason
        - Small conditional fix in cloud schema change capture logic only; no test was run in this session.
  • Behavior changed:

    • No.
    • Yes.
      - Cloud schema change now decides whether to capture historical rowsets using the alter version registered in meta service, avoiding empty schema change output in the version-1 to later-version race.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

### What problem does this PR solve?

Issue Number: None

Related PR: None

Problem Summary: Cloud schema change may register an alter_version greater than the FE task's original visible version. When the task is created at version 1 but starts after new versions arrive, BE can skip capturing historical rowsets and later commit an empty schema change output.

### Release note

None

### Check List (For Author)

- Test: No need to test (small condition fix; not run in this session)
- Behavior changed: Yes (cloud schema change now captures historical rowsets using the registered alter_version to avoid empty output on schema change races)
- Does this need documentation: No
@Thearas

Thearas commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@Hastyshell Hastyshell changed the title [fix](be) Use actual schema change alter version in cloud capture [fix](schema-change) Use actual schema change alter version in cloud capture Apr 15, 2026
gavinchou
gavinchou previously approved these changes Apr 15, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR approved by at least one committer and no changes requested.

@github-actions github-actions Bot added the approved Indicates a PR has been approved by one committer. label Apr 15, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR approved by anyone and no changes requested.

### What problem does this PR solve?

Issue Number: None

Related PR: None

Problem Summary: Add a cloud regression test for the inverted index schema change race where FE creates the alter task at version 1 but BE registers a later alter_version after new data is published. Also document why cloud schema change must use the meta-service alter_version when deciding whether to capture historical rowsets.

### Release note

None

### Check List (For Author)

- Test: No need to test (regression case added but not run in this session)
- Behavior changed: No
- Does this need documentation: No
@Hastyshell

Copy link
Copy Markdown
Collaborator Author

run buildall

@github-actions github-actions Bot removed the approved Indicates a PR has been approved by one committer. label Apr 15, 2026
@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 100.00% (1/1) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 73.53% (27359/37210)
Line Coverage 57.24% (295654/516511)
Region Coverage 54.51% (246492/452223)
Branch Coverage 56.08% (106666/190204)

@Hastyshell

Copy link
Copy Markdown
Collaborator Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found from code review.

This PR fixes the cloud schema-change race described in the PR: CloudSchemaChangeJob::process_alter_tablet() now uses the meta-service registered alter_version (start_resp.alter_version()) when deciding whether to capture historical rowsets, which matches the later commit path that also uses the registered value. That closes the case where an FE task created at version 1 can otherwise skip capturing [2, alter_version] and then commit an empty schema-change output.

Critical checkpoints:

  • Goal / correctness: Met. The changed branch now keys off the same alter_version that prepare_tablet_job returns and that schema-change commit later consumes. The new regression regression-test/suites/cloud_p1/schema_change/compaction_optimization/test_inverted_index_sc_version1_race.groovy exercises the intended version-1 race and validates the resulting index/query behavior.
  • Change size / focus: Good. One localized BE logic change plus one targeted regression.
  • Concurrency: No new concurrency risk found. Existing schema_change_lock usage and lock ordering are unchanged, and the change does not move heavy work under new locks.
  • Lifecycle / static initialization: No new lifecycle or static-init concerns.
  • Configuration: No new config added.
  • Compatibility: No protocol, storage-format, or rolling-upgrade compatibility concerns introduced.
  • Parallel code paths: For the cloud schema-change path, this updates the inconsistent capture gate that previously still looked at the FE task version.
  • Special conditional check: The new condition is necessary and the added comment explains why.
  • Test coverage: Targeted cloud docker regression coverage was added for the reported race. I did not run the docker regression in this review session.
  • Observability: Existing logs around tablet ids, alter_version, and job_id remain sufficient for this narrow fix.
  • Transaction / persistence: Good. The BE capture decision now matches the alter_version persisted/returned by meta-service and later used during schema-change commit.
  • Data writes / modifications: Good. This avoids the empty-output commit path that can otherwise recycle double-written rowsets without materializing their historical equivalents.
  • FE/BE variable passing: No new fields or cross-layer propagation changes.
  • Performance: No meaningful regression; this only switches to an already-available response value.
  • Other issues: None blocking found.

Residual risk:

  • The new regression is docker/debug-point based and I did not execute it here, so the remaining risk is limited to test determinism rather than the logic change itself.

@github-actions github-actions Bot added the approved Indicates a PR has been approved by one committer. label Jul 31, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR approved by at least one committer and no changes requested.

@shuke987

shuke987 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for one blocking test issue.

The production change appears correct by static review: process_alter_tablet() now decides whether to capture historical rowsets from the alter version registered and returned by meta-service, which is the same boundary used for reader context, conversion, recycling, and final commit. In the version-1 race, the old guard could produce no conversion outputs while committing at a later boundary; the new guard captures [2, alter_version] and preserves the double-written data. However, the only added regression does not execute as a Docker suite because its nonConcurrent,docker group is classified as SINGLE. The inline comment identifies the required one-line group fix.

Critical checkpoints (code-review skill Part 1.3):

  • Goal and proof: The production condition uses the authoritative registered boundary and accomplishes the intended fix. Proof is currently incomplete because the new regression is skipped by the Docker pass and rejected by docker() if run as a single suite; remove nonConcurrent so the intended race actually runs.
  • Scope and clarity: The C++ edit is focused and its comment explains the non-obvious FE-task/meta-service boundary split. The regression is otherwise targeted; its execution-group declaration is the blocking defect.
  • Concurrency: No production concurrency issue found. Existing base schema-change locking, tablet sync/meta lock order, meta-service compaction boundary checks, and MoW stop/update locks are unchanged and remain aligned with the registered boundary.
  • Lifecycle and static initialization: No new lifecycle, ownership, circular-reference, or cross-TU static-initialization concern.
  • Configuration: No production configuration item was added or changed. The regression's suite-group configuration must be corrected as noted inline.
  • Compatibility: No protocol field, function symbol, storage format, or rolling-upgrade contract changed; the existing start-job response field is reused.
  • Parallel paths: The shared cloud schema-change/rollup conversion path, duplicate-key path, compaction paths, and MoW delete-bitmap retry path were traced. No parallel-path mismatch was found.
  • Special condition: The response-based check is necessary because the FE task version can remain 1 while the executing BE registers a later local maximum. The comment explains the race and capture range.
  • Test coverage: The intended bounded, one-BE debug-point scenario is a deterministic oracle once executable: old code cannot reach FINISHED because meta-service rejects empty outputs for the later boundary. As committed, though, executor precedence prevents the test body from running. This review was static-only as required; no build or test was run locally.
  • Observability: Existing logs include base/new tablet ids, registered alter version, and job id; no additional metric or log is needed for this narrow correction.
  • Transaction and persistence: The captured logical range matches the meta-service recycle range, and output promotion, statistics, cumulative point, tablet state, and job removal remain in the established transaction.
  • Data writes and failure handling: The fix prevents an empty-output attempt at a nontrivial boundary. Status failures remain checked and fail closed; no committed-data visibility, overlap, or MoW delete-bitmap inconsistency was found.
  • FE/BE propagation: No new variable or thrift field is introduced. FE's original task version and meta-service's registered boundary retain their existing roles.
  • Performance: No redundant hot-path work was added; conversion runs only for historical versions that the registered commit boundary requires.
  • Other issues: No other issue was substantiated after full changed-file, retry, cache-replay, compaction, fallback, and test-oracle review.

User focus: .code-review.BerB3N/review_focus.txt contains no additional focus, so the entire PR was reviewed without a narrower user-specified concern.

Review completion: Three rounds were completed against head d975f514f9a051d0e2a73d2cc44aaba8ba462f43 and the same final one-comment set. The final round's normal full-coverage and separate risk-focused reviewers returned NO_NEW_VALUABLE_FINDINGS; all candidates and initial risks were independently verified, deduplicated, accepted, or dismissed with concrete evidence.

@Yukang-Lian
Yukang-Lian merged commit 453c254 into apache:master Aug 10, 2026
36 of 37 checks passed
github-actions Bot pushed a commit that referenced this pull request Aug 10, 2026
…capture (#62506)

Problem Summary:

Cloud schema change in BE decides whether to capture historical rowsets
using the original FE task alter version, but later executes and commits
using the alter version registered in meta service. When the FE task is
created at version 1 and the registered schema change job uses a later
alter version, BE can skip capturing historical rowsets and then commit
an empty schema change output.

This change uses the registered schema change alter version consistently
when deciding whether historical rowsets need to be captured.
github-actions Bot pushed a commit that referenced this pull request Aug 10, 2026
…capture (#62506)

Problem Summary:

Cloud schema change in BE decides whether to capture historical rowsets
using the original FE task alter version, but later executes and commits
using the alter version registered in meta service. When the FE task is
created at version 1 and the registered schema change job uses a later
alter version, BE can skip capturing historical rowsets and then commit
an empty schema change output.

This change uses the registered schema change alter version consistently
when deciding whether historical rowsets need to be captured.
yiguolei pushed a commit that referenced this pull request Aug 11, 2026
…on in cloud capture #62506 (#66606)

Cherry-picked from #62506

Co-authored-by: Siyang Tang <tangsiyang@selectdb.com>
@hello-stephen

Copy link
Copy Markdown
Contributor

[community-pipeline-root-cause DORIS-27883]

@Hastyshell @gavinchou#62506 于 2026-08-10 15:26:56 +08:00 合入 master 后,CloudSchemaChangeJobTest.FillVersionHolesBeforeNewTabletRunning 在社区 BE UT 中开始持续失败:最近 300 个返回记录里 28 次有效失败,全部为 cloud_schema_change_job_test.cpp:190 / [INTERNAL_ERROR] failed to find path in version_graph. spec_version: 2-2

因果链比较明确:该 fixture 设置 request.alter_version=1、mock start_resp.alter_version=2,但 base tablet 没有 [2,2] rowset;本 PR 把 capture 判断从 request.alter_version 改为 start_resp.alter_version 后,测试会实际执行 capture_rs_readers({2,2}) 并失败。失败分布在多个无关 PR,不是这些被测 PR 各自引入。

跟踪单:DORIS-27883。请补一个保持该测试原目标、同时与 registered alter version 一致的 fixture 修复,并以修复 PR checkout SHA 下该 exact BE UT 的终态通过作为闭环证据;当前 test 已 mute,整体 build 变绿本身不能作为通过证据。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by one committer. dev/3.1.x dev/4.0.x dev/4.1.4-merged reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants