[fix](ccr) Disallow Row Binlog on MOW tables with cluster keys - #66093
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Review opinion: Request changes
Findings
- Preserve the inherited-binlog test database after execution so assertion failures leave the effective table metadata available for debugging.
- Record the deterministic property and SHOW CREATE results through named regression cases with an auto-generated
.outartifact. - Hardcode the two fixed ordinary table names at their SQL/query use sites.
Critical checkpoint conclusions
- Goal and behavior: the FE implementation correctly rejects explicit ROW-binlog MOW cluster keys and prevents the testing-only random-key injection when ROW binlog is effective through database inheritance. The regression covers both behaviors.
- Scope and clarity: the production change is focused; the only requested changes are in the new regression suite.
- Concurrency: database binlog state is copied under the database read lock; property updates use the write lock. The analysis/execution double snapshot can fail closed under a concurrent database-property change but cannot publish the unsupported state.
- Lifecycle and configuration: no new runtime configuration or special object lifecycle is introduced.
setFeConfigTemporaryrestores the existing mutable test config, but the suite'sonFinishcallback removes diagnostic state even after failure. - Compatibility and parallel paths: CREATE, CTAS, and CREATE LIKE converge on the guarded create path; ALTER cannot introduce the combination; ROW-binlog reorder is rejected as a non-light schema change. The replay-only candidate was dismissed because ROW binlog is experimental, master-only, and present in no release tag or release branch.
- Error handling, persistence, protocol, and data writes: the new invariant raises
AnalysisExceptionand is converted to DDL failure before row-binlog metadata or tablets are created. No new persisted field, FE-BE protocol value, transaction path, or data-write path is added. - Performance and observability: the only added catalog lookup/copy in analysis is behind the testing-only random-key config; no material hot-path or observability issue was found.
- Tests: the negative case uses the required
test { sql; exception }form and the property query is ordered, but the new suite violates the mandatory preserved-debug-state, generated-oracle, and hardcoded-table-name rules described in the inline comments. No build or regression test was run because this review invocation is explicitly review-only; current CI shows FE CheckStyle passed. - User focus: no additional user-provided review focus was supplied; the whole PR was reviewed.
Review completion: INCOMPLETE at the three-round cap. A distinct table-name-contract finding was identified in Round 3, so the final three-comment payload could not receive another convergence round. All substantiated findings identified within the allowed rounds are included here.
12be97e to
efaece7
Compare
|
/review |
There was a problem hiding this comment.
Review opinion: Request changes
Findings
- Regenerate the SHOW CREATE golden under the standard binlog-enabled regression configuration; the committed row omits the effective binlog fields that CI renders.
- Pin
storage_formatin the successful CREATE before recording its full SHOW CREATE output; nonConcurrent fuzzy sessions otherwise choose V2 or V3 randomly while the golden requires V2.
Critical checkpoint conclusions
- Goal and data correctness: the production change correctly prevents enabled ROW binlog from coexisting with MOW cluster keys. Nereids suppresses testing-only random cluster keys when ROW binlog is effective, and the catalog independently recomputes the effective config and rejects explicit or generated cluster keys before table registration or tablet creation.
- Scope and clarity: the Java change is small and reuses one locked database-plus-table config snapshot helper rather than duplicating inheritance logic.
- Concurrency and lifecycle: the database read lock protects only a short config copy; no heavy work, nested lock, escaping shared state, static initialization, or special lifetime is added. Either direction of a database-property change between analysis and execution remains fail-safe at the final catalog guard.
- Compatibility and parallel paths: direct CREATE, CTAS, CREATE LIKE, and supported programmatic CreateTableCommand users converge on the same final guard. ALTER cannot change an existing binlog format or toggle enable once the format is ROW, and replay preserves historical metadata for compatibility. No storage format, edit-log shape, function symbol, or FE-BE protocol changes are introduced.
- Error handling, persistence, and data writes: the incompatibility raises through the existing AnalysisException-to-DdlException boundary before persistent registration or external tablet work. No transaction visibility or data-write path changes.
- Tests: the negative case, inherited database-format/table-enable case, named query cases, pre-test cleanup, and preserved post-test state are structurally appropriate. However, the new SHOW CREATE oracle is deterministically wrong under the standard binlog-enabled FE and independently flaky under fuzzy V3 selection. No build or regression execution was attempted because this review invocation is explicitly review-only.
- Performance and observability: the added work is a small metadata copy plus a fuzz-only catalog lookup. The explicit error and existing table-properties/SHOW CREATE surfaces provide sufficient diagnostics; no new metric is warranted.
- User focus: no additional user-provided focus was supplied, so the whole PR was reviewed.
Review coverage is complete after two convergence rounds: every candidate was accepted, duplicated, or dismissed with concrete evidence, and all normal and risk-focused reviewers returned NO_NEW_VALUABLE_FINDINGS for this exact final payload.
efaece7 to
c70a537
Compare
|
/review |
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Aug 2nd, 2026 1:27 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
run buildall |
TPC-H: Total hot run time: 29234 ms |
TPC-DS: Total hot run time: 178934 ms |
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Aug 2nd, 2026 1:27 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
ClickBench: Total hot run time: 25.21 s |
FE UT Coverage ReportIncrement line coverage |
|
/review |
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Aug 2nd, 2026 1:27 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
|
Codex automated review failed and did not complete. Error: Your access token could not be refreshed because your refresh token was revoked. Please log out and sign in again. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
There was a problem hiding this comment.
Review opinion: No new blocking code-review issues found on c70a537544ead48d9d645084862d00680349c0eb.
Critical checkpoint conclusions:
- Goal and test proof: The final catalog guard rejects enabled ROW binlog on MOW tables with cluster keys before tablet creation, while Nereids suppresses the testing-only random cluster-key injection when ROW binlog is effective. The negative regression and inherited-config golden cases cover those behaviors compositionally.
- Scope, clarity, and reuse: The production change is focused and reuses one database snapshot/effective-config helper in validation and execution.
- Concurrency and lifecycle: Database config copies are taken under the database read lock; mutations use the write lock. Validation/execution snapshot drift is fail-safe at the execution guard and adds no lock-order or object-lifetime hazard.
- Configuration and compatibility: No new config, persisted field, storage format, function symbol, or FE-BE protocol value is introduced. The existing mutable fuzzy-test config is scoped and restored. CREATE, CTAS, CREATE TABLE LIKE, supported programmatic CREATE, ALTER, replay, and restore paths were checked.
- Error handling, persistence, and data writes: The incompatibility raises through the existing analysis-to-DDL failure boundary before row-binlog metadata, tablet work, or table registration. No transaction visibility or data-write path changes.
- Tests and results: The current suite uses the required expected-error form, pre-test cleanup, named golden queries, deterministic ordering, pinned storage format, and preserved post-test state. The generated golden records the effective ROW properties and absence of random ORDER BY. The five prior inline concerns are addressed on the current head.
- Performance and observability: Added work is a short metadata copy plus a lookup behind the fuzzy-only branch; no material hot-path cost or new metric is warranted. The specific error plus existing properties/SHOW CREATE surfaces are sufficient.
- User focus: No additional user-provided focus was supplied; the whole PR was reviewed.
- Validation limit: This invocation is review-only, so no local build or test was run. Live Compile, CheckStyle, FE UT, P0, NonConcurrent, and performance checks pass.
check_coverage_feremains failed, but its TeamCity log returns HTTP 401 here, so this review does not infer a patch defect from that opaque status.
Review completion: COMPLETE after three rounds. Every candidate was accepted, duplicated, or dismissed with concrete evidence, and all normal plus risk-focused reviewers returned NO_NEW_VALUABLE_FINDINGS for the final resolved ledger and empty inline-comment set.
|
PR approved by at least one committer and no changes requested. |
### What problem does this PR solve? Issue Number: None Related PR: apache#66093 Problem Summary: PR apache#66093 rejects Row Binlog on unique MOW tables with cluster keys during table creation. Remove the now-unreachable Segment-format subcase and its two golden files, and restore the historical-row lookup call that was changed only to support that combination. Keep the independent MOW cluster-key cases and all supported Row Binlog, sequence, BEFORE, and compaction coverage. ### Release note None ### Check List (For Author) - Test: Unit Test - `./run-be-ut.sh --run --filter=<all-six-segment-flusher-golden-producers> -j 32` (6 tests passed) - `build-support/check-format.sh` passed with clang-format 16.0.6 - clang-tidy reported no warning for the changed test file; analysis of `base_tablet.cpp` is blocked by the pre-existing unmatched `NOLINTEND` in `be/src/core/types.h:576` - Behavior changed: No - Does this need documentation: No
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)