Workflow [PR], commit [80f2cb0]

Summary: ❌

AI Review

Summary

This PR adds a constructor-time check in StorageMergeTree to reject disks whose metadata backend is MetadataStorageType::Keeper (e.g. s3_with_keeper). High-level verdict: ⚠️ request changes, because the new check executes for all loading modes and can make existing tables unattachable after upgrade.

Findings

❌ Blockers

• [src/Storages/StorageMergeTree.cpp:201-202] The new validation runs in StorageMergeTree constructor for all load modes, including ATTACH/startup paths. That introduces a backward-incompatible behavior change: legacy tables that were previously attachable can fail to start after upgrade.
  Suggested fix: enforce this only for new table creation (mode < LoadingStrictnessLevel::ATTACH), or gate old-object behavior via a compatibility/setting switch.

Tests

• ⚠️ Add a regression test that verifies legacy MergeTree metadata with s3_with_keeper can still be attached under compatibility mode (or, if intentionally disallowed, that the behavior is explicitly gated and documented).
• ⚠️ Add a test that CREATE TABLE ... ENGINE=MergeTree with s3_with_keeper is rejected with BAD_ARGUMENTS.

ClickHouse Rules

Final Verdict

• Status: ⚠️ Request changes
• Minimum required actions:
  • Restrict the new Keeper-metadata prohibition to new table creation, or add a compatibility gate for existing tables.
  • Add focused tests covering both rejection-on-create and attach/startup compatibility behavior.

The Stress test (arm_msan) failure is fixed by #101239, which should be merged first. After it is merged, please update the branch to include the fix.

The test 02859_replicated_db_name_zookeeper is fixed in #101952

The MSan stress test failure (MemorySanitizer: use-of-uninitialized-value, STID 4179-5154 or 4148-3044) is a known pre-existing issue unrelated to this PR. Fix: #102158

LLVM Coverage Report

Changed lines: 90.91% (10/11) | lost baseline coverage: 1 line(s) · Uncovered code

Full report · Diff report