Skip to content

cloudstorage: make all DateSeparator fields use the enum instead of string - #6078

Open
kennytm wants to merge 1 commit into
pingcap:masterfrom
kennytm:fix-6077
Open

cloudstorage: make all DateSeparator fields use the enum instead of string#6078
kennytm wants to merge 1 commit into
pingcap:masterfrom
kennytm:fix-6077

Conversation

@kennytm

@kennytm kennytm commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: close #6077

What is changed and how it works?

Changes all places that uses the DateSeparator expecting a string to the common enum config.DateSeparator so case-sensitivity no longer appears.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Questions

Will it cause performance regression or break compatibility?

No

Do you need to update user documentation, design documentation or monitoring documentation?

No

Release note

Previously sink.date-seperator accepted non-lowercased values like "DAY" but ignored them. Now they are accepted and treated the same as the lowercased versions.

Summary by CodeRabbit

  • Improvements
    • Standardized date-separator configuration using supported typed values across storage sinks.
    • Added consistent JSON and TOML serialization, including case normalization and validation of unsupported values.
    • Improved date-separator handling when parsing storage paths and generating dated files.
    • Cloud-storage cleanup now reports configured separators more clearly.
  • Bug Fixes
    • Invalid date-separator settings are now rejected with clear configuration errors.
    • Storage consumers consistently apply the configured separator during scans.

@ti-chi-bot ti-chi-bot Bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/needs-triage-completed labels Aug 24, 2026
@coderabbitai

coderabbitai Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3bf86531-bab5-4872-b2e3-405884ad26cb

📥 Commits

Reviewing files that changed from the base of the PR and between ac1039f and b8e93ff.

📒 Files selected for processing (16)
  • api/v2/model.go
  • api/v2/model_test.go
  • cmd/storage-consumer/consumer.go
  • downstreamadapter/sink/cloudstorage/dml_writers_test.go
  • downstreamadapter/sink/cloudstorage/sink.go
  • downstreamadapter/sink/cloudstorage/sink_test.go
  • downstreamadapter/sink/cloudstorage/writer_test.go
  • pkg/cloudstorage/config.go
  • pkg/cloudstorage/config_test.go
  • pkg/cloudstorage/generator.go
  • pkg/cloudstorage/path_key.go
  • pkg/cloudstorage/path_key_test.go
  • pkg/cloudstorage/path_test.go
  • pkg/config/replica_config.go
  • pkg/config/sink.go
  • pkg/config/sink_test.go

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The change replaces string date separators with the typed config.DateSeparator enum. It adds text serialization and validation, updates cloud storage path handling and cleanup, and caches the separator in the storage consumer.

Changes

Date separator configuration contract

Layer / File(s) Summary
Typed configuration and serialization
pkg/config/sink.go, pkg/config/replica_config.go, api/v2/model.go, pkg/config/sink_test.go, api/v2/model_test.go
DateSeparator fields now use the enum type. JSON and TOML conversion accepts case variants, emits normalized values, and rejects unsupported values.

Cloud storage typed path handling

Layer / File(s) Summary
Path parsing and date generation
pkg/cloudstorage/config.go, pkg/cloudstorage/path_key.go, pkg/cloudstorage/generator.go, pkg/cloudstorage/*_test.go
Cloud storage APIs and comparisons now use config.DateSeparator values directly. Date path generation and validation behavior remains unchanged.

Consumer separator propagation

Layer / File(s) Summary
Cached consumer configuration
cmd/storage-consumer/consumer.go
The consumer reads the configured separator during construction and reuses it when parsing DML index paths.

Cloud sink cleanup and tests

Layer / File(s) Summary
Typed cleanup handling
downstreamadapter/sink/cloudstorage/sink.go, downstreamadapter/sink/cloudstorage/*_test.go
Cleanup compares the typed day separator and logs separator values with zap.Stringer. Cloud storage tests use enum values directly.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to b8e93

The change standardizes DateSeparator handling to use the shared enum and adds related unit-test updates; no actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: 3aceshowhand, wk989898

Poem

I’m a small rabbit guarding the day,
Strings hop out; enums lead the way.
JSON speaks clearly, paths parse true,
Cleanup logs typed values too.
“DAY” now lands where DAY should stay!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes normalize wrong-cased date-separator values and reject unsupported values, meeting issue #6077 objectives.
Out of Scope Changes check ✅ Passed All changes support the DateSeparator type migration, validation, serialization, consumer logic, or related tests.
Docstring Coverage ✅ Passed Docstring check was indeterminate for this PR — some files could not be analyzed in time. Not blocking.
Title check ✅ Passed The title clearly describes the main change: replacing string DateSeparator fields with the shared enum in cloud storage configuration.
Description check ✅ Passed The description includes the required issue reference, change summary, test checklist, compatibility and documentation answers, and release note. The stated tests should be verified because the object…
Full details: Description check

Explanation

The description includes the required issue reference, change summary, test checklist, compatibility and documentation answers, and release note. The stated tests should be verified because the objectives indicate that only unit tests were added.

✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ti-chi-bot ti-chi-bot Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 24, 2026
@wk989898

Copy link
Copy Markdown
Collaborator

Please add a manual test 🙏🏻

@kennytm

kennytm commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Manual test (not integration test yes?):

make cdc

tiup playground-ng v8.5.7 --db 1 --kv 1 --pd 1 --tiflash 0 --ticdc 1 --ticdc.binpath ./bin/cdc --ticdc.config ./ticdc-newarch.toml --without-monitor

./bin/cdc cli changefeed create -c test5854 --sink-uri 's3://test5854/?endpoint=http://127.0.0.1:9000&protocol=canal-json&access-key=minioadmin&secret-access-key=minioadmin' --config ./test5854.toml

mysql -u root -h 127.0.0.1 -P 4000 test
# create table a (a bigint primary key);
# insert into a values (4);

The file is created at s3://test5854/test/a/468602811870085128/2026-08-24/CDC00000000000000000001.json yes.

@ti-chi-bot ti-chi-bot Bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Aug 24, 2026
@ti-chi-bot

ti-chi-bot Bot commented Aug 24, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: wk989898
Once this PR has been reviewed and has the lgtm label, please assign nongfushanquan for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot

ti-chi-bot Bot commented Aug 24, 2026

Copy link
Copy Markdown

[LGTM Timeline notifier]

Timeline:

  • 2026-08-24 13:36:14.801980486 +0000 UTC m=+508809.973074627: ☑️ agreed by wk989898.

@kennytm

kennytm commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

/check-issue-triage-complete

@ti-chi-bot ti-chi-bot Bot added needs-cherry-pick-release-nextgen-202603 Should cherry pick this PR to release-nextgen-202603 branch. and removed do-not-merge/needs-triage-completed labels Aug 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-1-more-lgtm Indicates a PR needs 1 more LGTM. needs-cherry-pick-release-nextgen-202603 Should cherry pick this PR to release-nextgen-202603 branch. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wrong-cased date-separator is accepted but ignored

2 participants