SEZ9 commented on PR #10306: URL: https://github.com/apache/seatunnel/pull/10306#issuecomment-5230721196
Thanks @DanielLeens — this is an excellent deep-dive review, and it matches (and extends) what I found on my own pass. Triaging your points as maintainer: **1. Default JDBC path changes — agree, blocking.** The `clearBatchSilently()` + MultiTableSink continue-on-failure combination turning a visible table failure into silent partial data loss is not acceptable on a path where the user hasn't opted in. Concrete ask to the author: only clear the batch when a `RowErrorCollector` is actually attached, or quarantine the table after clearing so the failure remains observable. Separately, the `JdbcOutputFormat.flush()` fail-fast on `22xxx`/`23xxx` (no retries) is a default-behavior change for all existing JDBC jobs — it needs an entry in `incompatible-changes.md` and a mention in the JDBC sink docs, regardless of whether we keep it. **2. `MultiTableState` serialVersionUID — agree, must be proven, not asserted.** Since this state is Java-serialized into checkpoints, please confirm in the PR description that `5992121739651030596L` was produced by running `serialver` against the pre-PR class, and add a restore test that deserializes a state blob captured from the old class shape. Without that test we can't safely claim savepoint compatibility. **3. `EngineRowErrorCollector` retention — agree, blocking for streaming.** If `recordedTerminalRows` accumulates every successfully flushed row with no consumer, that's a guaranteed OOM on long-running jobs. The author needs either explicit cleanup once outcomes are fully consumed, or a redesign that avoids remembering terminal rows on this path. The O(n²) scans over `terminalOutcomes` / `pendingTerminalWriteRows` should be fixed in the same pass — batch sizes make this a real cost, not a nit. **4. Docs vs. implementation on threshold counters** — your comment appears truncated, but I flagged the same area in my review: the docs and the actual counter semantics (engine-state-backed, scoped by job/pipeline/action/stage) need to agree. Ask to the author: pick the intended semantics, document them precisely, and add a test pinning the behavior. Could you post the rest of point 4 so nothing gets lost? Restating my open asks from the earlier review that still stand: compatibility review for the changed public/serializable surfaces (`JdbcSinkWriter`, `SeaTunnelTask`, `SQLTransform`/`SQLMultiCatalogFlatMapTransform`, `DefaultAuthoritativeStateStores`), and the docs must clearly state this is Zeta-only for now since Flink/Spark translation paths are unwired. Also want to echo the positive: the test breadth here (~40 unit tests + 2 ITs across modes, overflow policies, and thresholds) is genuinely strong, and the awaitility-based rework of the `MultiTableSinkWriterTest` lock probe is the right call. Once items 1–4 are addressed I think this is close. <!-- streview-comment:105 --> -- 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]
