DanielLeens commented on PR #11069:
URL: https://github.com/apache/seatunnel/pull/11069#issuecomment-5178093362
Following up after @SEZ9's review.
**+1 to @SEZ9's Issue 1** — I traced the same path and want to add one piece
of evidence. On the current head, `convertToTimestampTz()` is:
```java
if (dbzObj instanceof String) {
return java.time.OffsetDateTime.parse((String) dbzObj);
}
throw new IllegalArgumentException(
"Unable to convert to OffsetDateTime from unexpected value '" +
dbzObj + "' of type " + ...);
```
The asymmetry is what makes this worth fixing rather than a nitpick: the
*wrong type* branch produces a good diagnostic naming the value and its class,
while the *wrong format* branch — much the more likely failure in practice —
throws a bare `DateTimeParseException` from deep inside `java.time` with no
column name, no table, and no offending value in the message. For a CDC job
that means a hard failure whose stack trace does not tell the operator which
column or which row broke it, on a connector where the whole point is
unattended replication.
`OffsetDateTime.parse` uses `ISO_OFFSET_DATE_TIME` strictly, so anything
Debezium emits that deviates — a `Z`-less value, a `+00`-style two-digit
offset, or a differently configured `time.precision.mode` — lands there.
Wrapping the parse and rethrowing with the column name plus the raw value is a
few lines and turns an unactionable failure into an actionable one. I would
treat it as blocking for the same reason @SEZ9 did.
I am also +1 on their **Issue 3**: this adds user-visible type support, so
the Postgres CDC connector docs should say `TIMESTAMP_TZ` is now supported, in
both `docs/en` and `docs/zh`. That is the project's standing rule for
user-visible behavior changes, and it is easy to miss on a converter-level fix.
Their Issues 2 and 4 look right to me as well, and I agree those are the
lower-priority ones.
Separately, and unchanged from my last comment: GitHub still reports **merge
conflicts** between this branch and the latest `dev` (`mergeable_state=dirty`),
and the last commit here is from June 18. So the sequence I would suggest is:
sync with the latest `dev` and resolve the conflicts, address Issues 1 and 3,
then push — at which point CI will produce a signal that is actually
attributable to this diff rather than to an 6-week-old base. Right now the
failing checks cannot tell you much either way.
No rush from my side, and thanks for your patience on this one — happy to do
a full re-review of the whole head as soon as you push the refreshed branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
