Uh oh!
There was an error while loading. Please reload this page.
[regression-test](streaming-job) add cdc cases for source/jdbc timezone and TIMESTAMP/timestamptz pk - #63543
Conversation
hello-stephen
commented
May 22, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
JNSimba
commented
May 22, 2026
run buildall |
JNSimba
commented
May 22, 2026
/review |
There was a problem hiding this comment.
I found test-stability issues that should be fixed before this regression-only PR is merged.
Critical checkpoint conclusions:
- Goal/test proof: The PR aims to add CDC timezone and timestamp/timestamptz PK regression coverage. The coverage is useful, but two suites can fail for reasons unrelated to the intended behavior.
- Scope/focus: The change is focused on regression tests and expected result files.
- Concurrency/lifecycle/config/storage/transaction compatibility: Not applicable; this PR does not change production concurrency, lifecycle, config, persistence, or storage behavior.
- Parallel code paths: MySQL and PostgreSQL variants are mostly covered in parallel; one timezone-output stability concern applies to both jdbc_servertimezone suites.
- Test coverage/results: Coverage is broad, but
test_streaming_postgres_job_source_timezoneincludes atimetzexpectation while current CDC conversion still lacks explicitio.debezium.time.ZonedTimehandling, and the jdbc timezone suites derive runtime timezone-dependent output while committing fixed+08:00.outfiles. - Observability/performance: Not applicable for test-only changes beyond existing Awaitility diagnostics.
User focus: No additional user-provided review focus was supplied.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
5e4c938 to
9db9002CompareJNSimba
commented
May 29, 2026
run buildall |
JNSimba
commented
May 29, 2026
/review |
There was a problem hiding this comment.
I found one additional blocking issue in a distinct PostgreSQL suite. The earlier thread already covers the test_streaming_postgres_job_source_timezonetimetz assertion, so I did not repeat that there; this comment is for the separate JDBC-server-timezone suite, which can fail independently for the same missing converter support.
Critical checkpoint conclusions:
- Goal/test coverage: the PR adds CDC regression coverage for MySQL/PostgreSQL timezone handling and timestamp primary-key chunking. The coverage is useful, but one added assertion still depends on unimplemented
ZonedTimeconversion. - Scope/clarity: test-only and generally focused.
- Concurrency/lifecycle/config/storage/transaction compatibility: no production code changes; no new concurrency, lifecycle, config, storage format, or transaction persistence behavior introduced.
- Parallel code paths: MySQL and PostgreSQL snapshot/binlog paths are both covered, but PostgreSQL
timetzhandling is not implemented consistently with the expected outputs. - Test result correctness: the new PostgreSQL
timetzexpected output in the JDBC timezone suite is not supported by current deserializer behavior. Existing review threads already cover the source-timezonetimetzcase and prior timezone determinism concern. - Performance/observability: not applicable beyond test runtime; the added waits/logs match surrounding CDC tests.
User focus: no additional user-provided review focus was specified.
Uh oh!
There was an error while loading. Please reload this page.
JNSimba
commented
Jun 1, 2026
run buildall |
1 similar comment
JNSimba
commented
Jun 2, 2026
run buildall |
JNSimba
commented
Jun 2, 2026
/review |
There was a problem hiding this comment.
Summary: I found one blocking correctness issue in the new PostgreSQL time with time zone conversion. The previous review-thread concerns about missing ZonedTime support and deterministic JDBC timezone expectations are not repeated here; this head adds a ZonedTime branch and pins the new deterministic test expectations.
Critical checkpoints:
- Goal/test proof: The PR aims to make CDC temporal timezone handling deterministic and cover timestamp split-key paths. The added tests cover UTC/fixed-offset/no-DST cases, but they do not prove correctness for IANA zones with DST, where the new
ZonedTimeconversion is currently wrong. - Scope/focus: The implementation is mostly small and focused, but the
Instant.EPOCHoffset shortcut changes semantics for a real class of supportedZoneIdvalues. - Concurrency/lifecycle: No new shared mutable concurrency or special lifecycle hazards found in the reviewed changes.
- Configuration/compatibility: No new config item is added; existing JDBC timezone parsing accepts IANA zones, so the new conversion must handle them correctly.
- Parallel paths:
ZonedTimestampuses a date-bearing instant and therefore gets date-aware zone rules; the newZonedTimepath does not, creating inconsistent behavior for the same configured timezone. - Tests/results: Regression outputs are ordered and deterministic for the fixed-offset/no-DST cases, but missing DST-zone coverage allowed the bug below.
- Observability/performance/persistence/data-write: No additional issues found beyond the timezone correctness issue.
User focus: No additional user-provided review focus was supplied.
Uh oh!
There was an error while loading. Please reload this page.
JNSimba
commented
Jun 3, 2026
run buildall |
JNSimba
commented
Jun 3, 2026
run external |
JNSimba
commented
Jun 3, 2026
run nonConcurrent |
1 similar comment
JNSimba
commented
Jun 3, 2026
run nonConcurrent |
JNSimba
commented
Jun 3, 2026
/review |
There was a problem hiding this comment.
Summary opinion: request changes. The PR largely focuses on CDC timezone handling and timestamp split-bound reconstruction, and the implementation plus regression outputs now align on preserving PostgreSQL timetz as Debezium's offset-bearing ZonedTime string. However, the added Java unit tests still assert the previous offset-less shifted behavior, so the test coverage does not prove the current behavior and will fail once the module test is run with its generated dependencies available.
Critical checkpoint conclusions:
- Goal/test proof: The runtime behavior for timetz is covered by regression outputs, but the new unit tests contradict it, so the test proof is currently broken.
- Scope/clarity: The changes are focused on CDC temporal conversion and split-bound restoration; no unrelated broad refactor was found.
- Concurrency/lifecycle: No new shared mutable runtime state beyond existing caches and serializer schema propagation appears to introduce a new concurrency or lifecycle issue in the reviewed paths.
- Configuration/compatibility: No new config or storage/protocol format incompatibility requiring compatibility handling was identified.
- Parallel paths: MySQL and PostgreSQL CDC paths were both reviewed for the timestamp-bound changes; no additional parallel-path issue was found beyond the failing unit-test expectations.
- Tests/results: Blocking issue found in DebeziumJsonDeserializerTest expectations. I attempted
mvn -Dtest=DebeziumJsonDeserializerTest testinfs_brokers/cdc_client, but this local invocation stops at missing generated/importedorg.apache.doris.job.cdcclasses before executing tests. - Observability/performance/transactions: No new concern found; this PR does not materially change transaction/persistence or hot-path observability requirements.
- User focus: No additional user-provided review focus was specified.
Uh oh!
There was an error while loading. Please reload this page.
JNSimba
commented
Jun 3, 2026
run buildall |
JNSimba
commented
Jun 3, 2026
/review |
There was a problem hiding this comment.
Summary opinion: Request changes. I found one blocking regression-test issue in the current patch set.
Critical checkpoint conclusions:
- Goal/test proof: The PR aims to harden CDC timezone handling and add timestamp/timetz coverage. The added MySQL jdbc_servertimezone case currently uses a JDBC option that makes the test fail before it can prove the intended behavior.
- Scope/focus: The code changes are generally focused on CDC temporal conversion and tests.
- Concurrency/lifecycle: No new concurrency or lifecycle-sensitive production state was introduced in the reviewed changes.
- Configuration/compatibility: No Doris config or storage/protocol compatibility change found. The JDBC URL option added in the test is the compatibility problem noted inline.
- Parallel paths: MySQL and PostgreSQL CDC paths were both inspected; the issue is specific to the MySQL named-zone URL with forceConnectionTimeZoneToSession.
- Tests/results: Regression tests use ordered query output, but one new MySQL regression cannot run reliably with the documented MySQL container timezone-table state. Existing review threads already cover the prior timetz conversion/unit-test/DST concerns, so I did not duplicate them.
- Observability/transactions/data correctness: No additional observability, transaction, or persistence issue found in this patch.
- User focus: No additional user-provided review focus was supplied.
Uh oh!
There was an error while loading. Please reload this page.
JNSimba
commented
Jun 3, 2026
run buildall |
JNSimba
commented
Jun 3, 2026
/review |
There was a problem hiding this comment.
Review summary:
No additional non-duplicate blocking issues found in the current head. The previously raised concerns around PostgreSQL ZonedTime handling, the unit-test expectations, deterministic JDBC timezone cases, and the MySQL named-zone/tz-table dependency appear addressed by the latest changes.
Critical checkpoint conclusions:
- Goal and proof: The PR adds CDC temporal timezone coverage and fixes PostgreSQL ZonedTime handling plus timestamp split-bound reconstruction. The added regression suites and unit tests cover the intended paths, though I could not execute the focused Maven test in this runner because standalone cdc_client compilation cannot resolve FE job.cdc classes.
- Scope: The implementation is focused on CDC deserialization, split-bound parsing, and related regression coverage.
- Concurrency: No new shared mutable concurrent state or lock ordering changes were introduced in the reviewed diff.
- Lifecycle/static initialization: No cross-translation-unit or non-obvious lifecycle issue found; Java static ObjectMapper usage follows existing module style.
- Configuration/compatibility: No new Doris config item or storage-format/protocol incompatibility found.
- Parallel paths: MySQL and generic JDBC/PostgreSQL split-bound reconstruction paths both call the shared convertBounds helper; ZonedTime is handled in the central Debezium deserializer.
- Tests: Regression outputs are ordered, tables are dropped before use, and the new unit tests cover the pass-through ZonedTime behavior including DST-zone non-shifting. Execution was not completed due the module classpath issue noted above.
- Observability: Existing error logging around conversion failures remains sufficient for this scoped change.
- Transaction/persistence/data write correctness: No FE/BE transaction or persistence path is modified by this PR.
- Performance: The added parsing and schema handling are not on a hot per-row path except the simple ZonedTime schema branch; no material performance concern found.
User focus: No additional user-provided review focus was specified.
JNSimba
commented
Jun 4, 2026
run external |
1 similar comment
JNSimba
commented
Jun 4, 2026
run external |
JNSimba
commented
Jun 4, 2026
run externl |
JNSimba
commented
Jun 4, 2026
run external |
…ne and TIMESTAMP/timestamptz pk
…to source tz and cover pg timetz
e82db0a to
554e2a9CompareJNSimba
commented
Jun 4, 2026
run buildall |
PR approved by at least one committer and no changes requested. |
JNSimba
commented
Jun 5, 2026
run cloud_p0 |
JNSimba
commented
Jun 5, 2026
/review |
There was a problem hiding this comment.
Review result: no new blocking issue found in the current head beyond the already-known inline review context.
Critical checkpoint conclusions:
- Goal and coverage: The PR adds CDC regression coverage for source/JDBC timezone behavior and timestamp/timestamptz chunk-key paths, plus small CDC client handling needed by those tests. The new regression suites and the
DebeziumJsonDeserializerTestadditions cover the main intended behaviors. - Scope: The implementation changes are focused:
ZonedTimeconversion and split-bound timestamp parsing, with tests/outputs for the new scenarios. - Concurrency: The changed code does not introduce new shared mutable state beyond the existing concurrent split-key class cache. No new lock ordering or heavy work under locks was introduced.
- Lifecycle/static initialization: No cross-TU/static lifecycle concern found for these Java changes.
- Configuration: No new config items were added.
- Compatibility: No storage format, FE-BE thrift/protocol, or rolling-upgrade incompatible change was identified. The split-bound parser is limited to reconstructing already-persisted/requested split bounds.
- Parallel paths: MySQL and PostgreSQL CDC paths are both covered by the new timestamp split-key tests; the
ZonedTimechange is specific to Debezium logical time-with-zone schemas. - Special checks: Existing timezone edge cases raised in earlier inline comments were considered and not duplicated; at this head the named MySQL forced-timezone issue and the
ZonedTimeexpected-output mismatch appear addressed. - Test results: Added
.outfiles are deterministic with ordered queries. I attemptedmvn -Dtest=DebeziumJsonDeserializerTest testfromfs_brokers/cdc_client, but this standalone module invocation fails before reaching the test because local reactor dependencies such asorg.apache.doris.job.cdc.*are not available in that invocation. - Observability: Existing failure logging in the regression suites and CDC conversion paths appears sufficient for these tests.
- Transaction/persistence/data writes: No Doris transaction visibility or persistent metadata correctness issue was found in the reviewed changes.
- FE/BE variable passing: No new transmitted execution variable was added.
- Performance: No hot-path performance regression was identified; the added parsing/probing remains on split-bound reconstruction or conversion paths.
User focus: no additional user-provided review focus was specified.
Uh oh!
There was an error while loading. Please reload this page.
What problem does this PR solve?
Add CDC streaming-job regression coverage for timezone behavior and TIMESTAMP / timestamptz chunk-key paths that the existing suites do not exercise.
Cases added
*_source_timezone(mysql + pg)timetz)timetzcolumn kept as a regression guard for the upstream JVM-tz handling*_jdbc_servertimezone(mysql + pg)jdbc_url'sserverTimezone/timezonewith Doris sessiontime_zone(read at runtime so it works on any default tz)*_timestamp_pk(mysql + pg)LocalDateTime/OffsetDateTimechunk-bound restore inAbstractCdcSourceReader.convertBoundRelease note
Add CDC streaming-job regression suites for source/jdbc timezone and TIMESTAMP / timestamptz chunk-key.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?