[fix](cloud) Skip empty partition meta RPCs in cloud restore - #67139
Open
felixwluo wants to merge 3 commits into
Open
[fix](cloud) Skip empty partition meta RPCs in cloud restore#67139felixwluo wants to merge 3 commits into
felixwluo wants to merge 3 commits into
Conversation
… segments in the rowset reader (apache#35484) ## Proposed changes pick apache#35432 ## Further comments If this is a relatively large or complex change, kick off the discussion at [dev@doris.apache.org](mailto:dev@doris.apache.org) by explaining why you chose the solution you did and what alternatives you considered, etc...
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
/review |
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16968 ms |
Contributor
TPC-DS: Total hot run time: 81437 ms |
Contributor
ClickBench: Total hot run time: 14.77 s |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes — capped/incomplete after the third permitted review round. Three blocking correctness issues are substantiated and covered inline. The third was newly accepted in the final round, so I cannot truthfully claim convergence even though every currently known candidate has been adjudicated and reported.
Critical checkpoint conclusions:
- Goal and proof: Supporting a Cloud restore with no selected backup partitions is a valid goal, but the code only avoids the failing partition RPCs; it does not preserve a writable table or complete metadata/vault semantics. The helper-level test does not prove the end-to-end goal.
- Scope and clarity: The two-file change is small and focused, but the broad empty-collection return conflates valid empty RANGE/LIST tables with invalid zero-partition unpartitioned tables and suppresses non-partition side effects.
- Concurrency:
run/cancelserialize job transitions and the vault map is protected by its read/write lock. No new data race, lock-order, or heavy-under-lock issue was found; the vault defect is initialization ordering and authority, not mutual exclusion. - Lifecycle: PENDING/CREATING transitions are unjournaled, so index publication/cleanup must remain replay-safe across failover. BackupHandler can also resume before the asynchronous vault checker has populated its cache.
- Configuration: No configuration is added. The relevant existing gates were checked: storage-vault enablement, FE compatibility-check mode, and Meta Service multi-version states.
- Compatibility: No function-symbol, FE/BE protocol, transmitted-variable, or storage-format change is introduced. Legacy and versioned metadata branches were inspected; the concrete MF-1 deletion path is on supported multi-version metadata.
- Parallel paths: Normal empty-table creation, exclude-all restore, existing-table partition restore, mixed/multi-table jobs, later ADD PARTITION, dynamic/automatic and unpartitioned tables, cancellation, replay, views/resources, and Cloud atomic restore were checked. Cloud atomic restore is rejected upstream; no separate issue remains there.
- Conditions and error handling: The empty guard is too broad. It turns an invalid unpartitioned terminal shape into success, skips required index work, and the vault fallback turns temporary cache unreadiness into permanent cancellation while accepting stale nonempty IDs.
- Test coverage and results: The new test directly injects a warm-cache
SinglePartitionInfotable and calls hooks; it does not cover filtering, the full state machine, registration, replay, index RPCs, authoritative-vault negative cases, later mutation, or post-restore writes. COMPILE, CheckStyle, BE UT, Cloud UT, and performance currently pass; FE UT currently fails and its TeamCity log requires authentication; several regressions remain pending. No local build/test was run because the review prompt prohibits it. - Observability: The bounded INFO log has useful identifiers and creates no separate logging/metrics issue, but it currently describes semantically required lifecycle work as a harmless skip.
- Persistence, transactions, and data writes: The missing versioned index hierarchy/table version, unjournaled replay window, and registration of an unwritable table violate metadata/data correctness. No independent FE/BE crash, memory-safety, or atomicity issue remained beyond the inline findings.
- Performance: No material CPU, memory, allocation, or hot-path regression was found.
- Additional focus: No additional user-provided review focus was supplied.
Please address all three inline findings and add end-to-end coverage for the repaired table types and failure/replay boundaries.
| // set storage vault for new restoring table | ||
| if (((CloudEnv) Env.getCurrentEnv()).getEnableStorageVault()) { | ||
| if (Strings.isNullOrEmpty(storageVaultId)) { | ||
| storageVaultId = Env.getCurrentEnv().getStorageVaultMgr().getVaultIdByName(storageVaultName); |
Contributor
There was a problem hiding this comment.
[P1] Do not treat the asynchronous vault cache as authoritative
For an empty restore there is no create-tablets response to validate the vault, so this lookup is the only source of the ID. After restart/master promotion, storageVaultId is transient and the vault map starts empty; BackupHandler can resume jobs before CloudInstanceStatusChecker populates it, turning a valid restore into a permanent cancellation. The map can also be stale: the checker leaves old entries when Meta Service reports zero vaults, so deleting the last vault can let this code persist a deleted nonempty ID. Please resolve/revalidate the vault from an authoritative source at empty-table creation time, treating transient unavailability as retry/defer and definitive absence as failure before registration; merely serializing the earlier cached ID would not handle deletion.
|
|
||
| private void handleOlapTableMeta(MetaSeriviceOperation operation, OlapTable olapTable, | ||
| Collection<Partition> partitions) throws DdlException { | ||
| if (partitions.isEmpty()) { |
Contributor
There was a problem hiding this comment.
[P1] Preserve the index lifecycle for an empty restored table
This avoids the invalid empty-partition RPCs, but it also skips the index-only lifecycle used by normal empty Cloud table creation. That path still calls prepare/commit materialized-index; commit-index creates the versioned index mappings and initializes the table version. A later ADD PARTITION only commits partition keys, so it does not repair the missing index mappings. The orphan recycler then sees no index-inverted key for the table and can delete the newly added partition and table-version metadata. Please use a replay-safe index PREPARE/COMMIT lifecycle for zero partitions, including cleanup of prepared/committed transient IDs on cancellation or PENDING replay, instead of suppressing all Meta Service work.
| if (partitions.isEmpty()) { | ||
| LOG.info("cloud restore job skip {} partitions, dbId: {}, tableName: {}, vault name: {}", | ||
| operation, dbId, olapTable.getName(), storageVaultName); | ||
| return; |
Contributor
There was a problem hiding this comment.
[P1] Do not register an unpartitioned table without its implicit partition
The new test exercises a SinglePartitionInfo table, removes its only partition, and this return lets the restore register that zero-partition table successfully. It is then permanently unusable: inserts fail with ERR_EMPTY_PARTITION_IN_TABLE, while ADD PARTITION is rejected for unpartitioned tables. The index-only lifecycle needed for valid empty RANGE/LIST tables does not repair this case. Please either reject exclusion of an unpartitioned table's sole implicit partition, or recreate an empty implicit partition/tablets so the restored table remains writable, and cover that outcome end to end.
Contributor
FE Regression Coverage ReportIncrement line coverage |
felixwluo
force-pushed
the
fix-cloud-restore
branch
from
August 26, 2026 02:43
bcb0487 to
87d783e
Compare
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 17072 ms |
Contributor
TPC-DS: Total hot run time: 83734 ms |
Contributor
ClickBench: Total hot run time: 14.68 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
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
3 participants
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: close #xxx
Related PR: #xxx
Problem Summary:
When restoring a backup from an integrated-storage cluster to a storage-compute separated cluster, cloud restore could fail if the backed-up OLAP table had no restore partitions. CloudRestoreJob still sent prepare/commit/drop partition RPCs to the meta service with an empty partition id list. The meta service correctly rejected the request with "empty partition_ids or index_ids or table_id". During cancellation/cleanup, the original failure could be overwritten by the drop-partition cleanup failure, showing an error like:
cloud restore job failed to drop partitions, table=xxx, partitions=[], errMsg: empty partition_ids or index_ids or table_id.
The fix treats empty restore partition collections as a no-op for cloud partition meta operations, because there is no partition metadata to prepare, commit, or drop. It also resolves the storage vault id from the restore storage vault name when no tablets are created, so empty-table restore does not depend on a createTablets response to fill the vault id.
Release note
Fix cloud restore failure when restoring backup metadata with empty OLAP table partitions.
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)