davidzollo commented on PR #11206: URL: https://github.com/apache/seatunnel/pull/11206#issuecomment-6462211676
@SEZ9 The review fixes are pushed at `03973e5cbd3d7f215f80aa2e38e0fe9f053c00ab`, including an upstream `dev` sync to `cec3276740d989c8cb67c7b8c37e0a817e728059`. GitHub now reports `MERGEABLE`; the PR remains open and non-draft. No PR-specific POM or workflow changes were added. - **F1/F8 and unsupported types:** real MySQL converter tests now cover backticks, semicolons, all-numeric names and unsupported column types, including repeated CREATE records and subsequent DML. Only the unsupported-type error is translated at the MySQL runtime admission boundary; unrelated failures still propagate. Rejected tables emit no CREATE or rows. Docs explicitly describe this WARN-and-skip exclusion. - **F2:** both capability SPIs now document the fail-fast contract, and dispatch rejects unsupported sinks rather than silently dropping CREATE. - **F4:** CREATE identity must match its catalog metadata. CREATE stays source-scoped even when another writer shares the physical target; ALTER retains its existing broadcast behavior. JDBC DDL uses the resolved sink target, options and schema, not the source event's target metadata. - **F5 and save modes:** runtime JDBC CREATE honors schema save modes through the ordered handler. Replay does not drop/truncate existing tables or re-run custom SQL. Registration follows the existing failure policy: immediate task failure for FAIL_FAST, configured retries then table quarantine for CONTINUE_OTHER_TABLES. Writers rejected before installation are closed before retry. - **F6:** the old serialized MultiTableState fixture now asserts an empty runtime-table list. The documentation distinguishes old-state restore from unsupported downgrade of new-feature checkpoints. - **F7:** both language docs and the E2E configuration now use JDBC's actual option, `is_exactly_once = false`, together with `generate_sink_sql = true` and no custom `query`. - **Other review findings:** restored legacy empty/absent `primary_keys` inheritance; appended CREATE_TABLE to preserve existing enum ordinals; replaced the new-table E2E's startup sleep with a replicated mutation barrier and added exact-name unsafe-table cleanup in `finally`. **Correction to F3:** my earlier statement that default `false` preserves existing restore behavior was incorrect. On [upstream dev](https://github.com/apache/seatunnel/blob/cec3276740d989c8cb67c7b8c37e0a817e728059/seatunnel-connectors-v2/connector-cdc/connector-cdc-base/src/main/java/org/apache/seatunnel/connectors/cdc/base/source/IncrementalSource.java#L378), hybrid restore already calls `restore(...)` unconditionally and [adds newly matched tables](https://github.com/apache/seatunnel/blob/cec3276740d989c8cb67c7b8c37e0a817e728059/seatunnel-connectors-v2/connector-cdc/connector-cdc-base/src/main/java/org/apache/seatunnel/connectors/cdc/base/source/IncrementalSource.java#L435). The option therefore defaults to `true` to preserve that behavior, with explicit `false` as the opt-out. Runtime binlog discovery remains default-off. `testRestorePreservesNewTableDiscoveryDefault` exercises the actual restoreEnumerator entry point for omitted/true/false configurations. ### Regression Coverage | Test | Regression guarded | | --- | --- | | `testResolveSinkTableExplicitEmptyPrimaryKeysInheritsSourcePrimaryKey` | Legacy empty primary_keys must not turn upserts into inserts | | `testRuntimeAdmissionUsesRealConverterBeforeEmittingEvents` | Rejected schemas must not emit CREATE/data or poison subsequent tables | | `testUnsupportedRuntimeTypeIsClassifiedWithoutMaskingOtherFailures` | Skip only supported admission failures, not unrelated faults | | `testRuntimeCreateRejectsMismatchedIdentityBeforeFactory` | Source event identity cannot redirect runtime registration | | `createTableDoesNotReinitializeSiblingsSharingThePhysicalSink` | CREATE isolation without regressing ALTER broadcast | | `testRuntimeRegistrationFailurePolicy`, `testRuntimeTableRegistrationRetriesWriterCreation` | Retry exhaustion, quarantine and successful CREATE replay | | `testRuntimeRegistrationRetryClosesUninstalledWriter` | Failed registration attempts must not leak writers | | `testRuntimeCreateHonorsSaveModeAndResolvedTarget`, `testRuntimeCreateReplayKeepsExistingTarget` | Configured target/save modes and non-destructive replay | | `testRestorePreservesNewTableDiscoveryDefault` | Existing hybrid-restore discovery remains the default | | Existing serialized-state fixture and runtime MySQL E2E | Old-state defaults, real unsafe identifiers, valid new-table insert/update/delete | **Validation:** Spotless apply/check passed for API, CDC base, CDC MySQL, JDBC and MySQL CDC E2E; `git diff cec3276740d989c8cb67c7b8c37e0a817e728059 --check` passed. Independent static review of the addressed fixes completed. Local compilation/tests/Docker were not run under this workspace's SeaTunnel rule; these added tests still need exact-head CI execution. At 06:18 UTC, the [fork Build](https://github.com/DanielLeens/seatunnel/actions/runs/40642768471) had Code style running and three initial jobs queued. [Upstream PR signal](https://github.com/apache/seatunnel/actions/runs/40642775355) reports `action_required`. The required Build result is not green yet. I have not rerun or cancelled those jobs, and have not merged the PR. The recommended table-count cap and DROP/RENAME cleanup remain outside this phase; docs now explicitly prohibit unbounded ephemeral-table workloads. This is a documented limitation, not a claim that resource growth is solved. -- 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]
