Skip to content

[fix](fe) Route binlog format changes through validation#65484

Open
hutiefang76 wants to merge 1 commit into
apache:masterfrom
hutiefang76:codex/doris-65383-binlog-format-route
Open

[fix](fe) Route binlog format changes through validation#65484
hutiefang76 wants to merge 1 commit into
apache:masterfrom
hutiefang76:codex/doris-65383-binlog-format-route

Conversation

@hutiefang76

Copy link
Copy Markdown

What problem does this PR solve?

Issue Number: close #65383

Related PR: None

Problem Summary:

ALTER TABLE ... SET ('binlog.format'=...) was not recognized as a binlog configuration update. It fell through to the generic schema-change path and returned the ROW-binlog schema-change error before the existing immutable-format validation could run.

This change routes binlog.format through updateBinlogConfig(), where BinlogConfig.mergeFromProperties(..., false) already returns the specific unsupported format-change error.

Release note

Unsupported binlog.format changes now report the immutable binlog-format error instead of the generic ROW schema-change error.

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
      • AlterOperationsTest
      • SchemaChangeHandlerTest#testChangeBinlogFormatReportsBinlogError
      • ./build.sh --fe
    • Manual test
    • No need to test or manual test.
  • Behavior changed:

    • No.
    • Yes. Only the error reported for unsupported binlog.format changes is corrected.
  • Does this need documentation?

    • No.
    • Yes.

I used AI-assisted review during development. I verified the root cause, patch, tests, and build locally.

### What problem does this PR solve?

Issue Number: close apache#65383

Related PR: None

Problem Summary: ALTER TABLE changes to binlog.format were not recognized as binlog configuration updates. They fell through to the generic schema-change path and returned the ROW-binlog schema-change error before the existing immutable-format validation could run. Route binlog.format through updateBinlogConfig so the existing BinlogConfig validation returns the specific format-change error.

### Release note

ALTER TABLE attempts to change binlog.format now report the immutable binlog-format error instead of the generic ROW schema-change error.

- Test: Unit Test
    - AlterOperationsTest
    - SchemaChangeHandlerTest#testChangeBinlogFormatReportsBinlogError
    - ./build.sh --fe
- Behavior changed: Yes. Only the error reported for unsupported binlog.format changes is corrected.
- Does this need documentation: No
@hello-stephen

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?

|| clause.getProperties().containsKey(PropertyAnalyzer.PROPERTIES_BINLOG_MAX_BYTES)
|| clause.getProperties().containsKey(PropertyAnalyzer.PROPERTIES_BINLOG_MAX_HISTORY_NUMS));
|| clause.getProperties().containsKey(PropertyAnalyzer.PROPERTIES_BINLOG_MAX_HISTORY_NUMS)
|| clause.getProperties().containsKey(PropertyAnalyzer.PROPERTIES_BINLOG_FORMAT));

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.

binlog.need_historical_value seems to have the same routing gap as binlog.format. ModifyTablePropertiesOp.validate() treats it as a binlog property, and BinlogConfig.mergeFromProperties(..., false) explicitly rejects changing it, but checkBinlogConfigChange() still routes it through the generic schema-change path. Could we include PROPERTIES_BINLOG_NEED_HISTORICAL_VALUE here and add the parallel test?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] ALTER TABLE changing ROW binlog.format returns wrong error message

3 participants