airborne12 commented on PR #69729: URL: https://github.com/apache/doris/pull/69729#issuecomment-6485646589
Closing this PR. The review is right on both points, and a complete fix is bigger than this change is worth. What I verified in the code: **P1: the FE change does not reach the read path.** This PR makes the create-tablets request and the `PartitionInfo` entry carry the format the backed-up files were written in. But on restore commit: - `CloudSnapshotMgr::_create_rowset_meta` (BE) builds the restored rowsets from a `RowsetWriterContext` that sets neither `inverted_index_storage_format` nor `persist_inverted_index_storage_format`, so the rowset metas carry no format override. That override is what normal writes persist when the tablet meta has the top-level format marker. - `MetaServiceImpl::commit_restore_job` then removes each restored rowset's inline tablet schema and writes it with `put_schema_kv` under `(index_id, schema_version)`. `put_schema_kv` leaves an existing schema KV in place. When the backup and the target table have the same schema version, which is the case this PR targets, the target table's V2 schema is kept, and reads return the V2 schema for SNII files. So MATCH on the restored partition can still open SNII files with the V2 parser. **P2: replay depends on follower-local config.** `recordBackedUpInvertedIndexFileStorageFormat` is gated on `enable_partition_inverted_index_storage_format_rollout`, a mutable, master-only config. A follower where it is unset skips the entry when it replays the restore, so master and follower can disagree about the partition's format. **Why not extend it.** A correct fix needs the format persisted in the restore journal (FE), set as the rowset and tablet override in the restore conversion (BE), and possibly a matching change in `commit_restore_job` (meta service), plus a cross-cluster restore test, because cloud mode cannot BACKUP. It only matters for clusters that run cloud mode with the partition-format rollout (off by default), the experimental cloud restore, and a backup whose partition format differs from the target table's default. An FE-only change would add state and look like a fix without fixing the read path, so I don't want to merge it. **Until then:** restoring a partition into a cloud table whose inverted index storage format differs from the backed-up partition's is not supported by the experimental cloud restore. If someone needs it, it should be one deliberate change across FE, BE and the meta service, with a cross-cluster test. The branch stays on my fork for reference. -- 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]
