SEZ9 commented on PR #10306:
URL: https://github.com/apache/seatunnel/pull/10306#issuecomment-5077822652
Thanks everyone for the thorough follow-ups. Summarizing where we stand on
head `8c94e4f686e8` (+ `8c367c959`):
**Issue 1 (JdbcSinkWriter public surface)** — @zhangshenghang thanks for the
detailed diff check. Since the existing `SinkWriter.Context` constructor is
kept as-is and the PR only *adds* an overload plus the
`SupportRowLevelErrorClassifier` implementation and `prepareCommit(long)`, I
agree there is no removal/rename here. I'm withdrawing this as a blocker.
Please just make sure the new overload's intended usage is javadoc'd so
downstream connector authors know which one to prefer.
**ROUTE-mode flush semantics** — @DanielLeens's re-check confirms the error
sink flush is chained with the parent writer lifecycle and routed rows are
excluded from normal sink success metrics, which closes my checkpoint/flush
concern. One remaining ask on the classifier side: please confirm (and cover
with a test) the fallback behavior when a sink does **not** implement
`SupportRowLevelErrorClassifier`, or when the classifier itself throws — both
should degrade to system-level fail-fast, never silent drops.
**Remaining review-thread items to close out:**
1. **@chl-wxp's enum suggestion** — +1. Returning an enum (e.g. `ROW_LEVEL`
/ `SYSTEM_LEVEL`, extensible later) instead of a boolean is a cheap change now
and painful after release. Please apply this before merge since the SPI is new
public surface.
2. **@nzw921rx's default threshold** — agreed, `max_error_ratio_min_records
= 100` is too aggressive as a floor for large jobs. Please raise the default
(something in the 1k–10k range seems reasonable) and document the trade-off;
users can lower it when needed.
3. **Error table job attribution** — I'd prefer adding a `job_id` column to
the fixed `st_error_data` schema over per-job `st_error_data_{jobId}` tables,
to avoid table proliferation and keep the schema stable for querying. Please
update the schema and docs accordingly.
4. **FQCN cleanup** — please sweep the PR for fully-qualified references
(`ClassLoaderService`, `java.time.LocalDateTime`, `ExceptionUtils`, etc.) and
convert to imports, matching what was already done for `RowErrorCollector`.
5. **@hawk9821's auto-creation question** — I think auto-creating the
built-in error table is a nice usability win, but I'd treat it as a follow-up
unless it's a small change here; if kept manual for now, the docs must include
the exact DDL.
So: blocker withdrawn on Issue 1, and the concrete remaining asks are the
enum SPI change (2), classifier fallback confirmation/test, default threshold
bump, `job_id` column, and the import cleanup. Once those land I'm good to
approve.
<!-- streview-comment:4 -->
--
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]
