SEZ9 commented on PR #11206:
URL: https://github.com/apache/seatunnel/pull/11206#issuecomment-6553353143
Thanks for the update on `d4faf1ea1aab8414aea81371c7c5112078f96739` (merge
of `dev` `3ac2d1cec6a` plus the fix commit). I'll wait for run 41164176211 to
finish before drawing conclusions from it.
On the `testRuntimeRegistrationFailurePolicy` root cause: the explanation is
clear and I agree with the classification — the
`[MULTI_TABLE_ISOLATED_FAILURE]` is the `CONTINUE_OTHER_TABLES`
all-tables-isolated rule firing because the test started with an empty writer
map, so it is neither the unsupported-sink case (F2) nor the retry/idempotency
gap (F5). Starting the test with `db1.healthy` and asserting that the healthy
table keeps writing while no row of the rejected table reaches a writer sounds
like the right fix. One ask: please keep the transient `IOException` during
runtime writer creation (F5) as a separate test case, so the retry/idempotency
contract gets its own coverage rather than being inferred from the
failure-policy test.
The remaining items from the previous review are not covered in this
comment, so a short status per item would help me finish:
- **F1 (HIGH, identifier injection):** is validation/quoting of
runtime-discovered database/table names now enforced in code on the generated
sink SQL and the `${database_name}_${table_name}` template, or still
documentation-only?
- **F2 (HIGH, unsupported sink):** what happens when a runtime
`CreateTableEvent` reaches a sink that does not support newly created tables
via `supportsNewlyCreatedTable()` — fail the job, or drop rows? Please define
it on the interface and cover it with a test.
- **F3 (HIGH, default of `scan.newly-added-table.enabled`):** has the `true`
default been reconsidered, or is there a rationale for changing
checkpoint-restore behaviour of existing wildcard jobs on upgrade?
- **F4 (MEDIUM, sink-side DDL from source events):** where is the
routing/target-scope check that prevents a source-side CREATE TABLE from
creating tables outside the configured sink scope?
- **F6 (MEDIUM, `MultiTableState` format):** how do old checkpoints restore,
and what happens on rollback with a checkpoint that already contains
runtime-registered tables?
- **F7 (MEDIUM, docs):** the JDBC sink documentation still needs the
`exactly_once = false` / `generate_sink_sql = true` constraints.
- **F8 (LOW, allowlist):** `[A-Za-z_][A-Za-z0-9_$]*` rejects legal MySQL
table names starting with a digit — please either widen the pattern or document
the limitation explicitly.
If any of these were handled in the fix commit, a short pointer per item is
enough.
<!-- streview-comment:2198 -->
--
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]
