SEZ9 commented on PR #11077: URL: https://github.com/apache/seatunnel/pull/11077#issuecomment-6538859722
Quick status check on this PR. The head is still `8b383c04eeed65c41a14ba756100a33f430124eb`, so the previously requested changes have not landed yet. To keep this moving, here is what still needs to be addressed, together in a new commit: - **PR11077-F1 (HIGH)** – destination-key collisions must not be able to route one table's rows through another destination's writer. Please make the key derivation collision-safe (or fail fast on collision) and add a test covering two tables that would otherwise collide. - **PR11077-F2 (HIGH)** – fanning the shared writer's state out to every aliased identifier and then unioning on restore duplicates state N times on recovery. Snapshot the shared writer's state once per destination (or dedupe on restore) and add a snapshot/restore test with aliased identifiers. - **PR11077-F3 (MEDIUM)** – the shared writer is created from one arbitrary alias's sink instance/`CatalogTable` but receives rows from all aliases. Please either validate schema/config compatibility across the aliased tables and fail clearly, or document that the connector must guarantee compatibility and guard accordingly. - **PR11077-F4 / PR11077-F6 (MEDIUM, Docs)** – `getPhysicalDestinationIdentifier()`, the writer-sharing behavior, and the changed `restoreWriter` contract (merged state from all aliased identifiers passed to a single `SeaTunnelSink#restoreWriter` call) need to be documented under `docs/` and in the Javadoc so connector implementers know what to expect. - **PR11077-F5 (MEDIUM)** – `proxyContexts` only registers the first alias per destination via `containsValue`, leaving the other aliases without a context entry and adding O(n²) startup cost. Please register a context for every identifier and use a direct lookup. - **PR11077-F7 (MEDIUM)** – wrapping `IOException` from `createWriter`/`restoreWriter` in an unchecked `RuntimeException` inside `computeIfAbsent` breaks the declared failure contract. Please move writer creation out of the lambda (or unwrap and rethrow as `IOException`). - **PR11077-F8 (LOW)** – add param/return tags to the `getDestinationKey` Javadoc. Two process items as already noted above: the branch is `diverged` from `dev` with mergeable state `dirty`, so please sync with `dev` and resolve the conflicts while preserving both sides; and the [Build](https://github.com/apache/seatunnel/runs/114987916217) check on this head is `ACTION_REQUIRED`, so please start the existing workflow on the new head rather than pushing an empty trigger commit. Once the new commit with the fixes, docs, and tests is pushed, drop a note here and the resulting head can get a full re-review. Thanks for sticking with this. <!-- streview-comment:2167 --> -- 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]
