DanielLeens commented on PR #12301: URL: https://github.com/apache/seatunnel/pull/12301#issuecomment-5661999490
Thanks for looping in on this, @201811510411lw — from a review standpoint your proposed candidate sounds like it targets exactly the two root-cause blockers currently open on this PR (both confirmed against the current head, `ef4f258b`, with no new commit since): - **Issue 1** (High): `sink instanceof SupportSinkDataPartition` is evaluated on the object `tryGenerateMultiTableSink` returns, which is a `MultiTableSink` wrapper for any multi-table-capable sink — Paimon included, even for a single-table job. `MultiTableSink` never implements `SupportSinkDataPartition`, so the partitioner as currently wired is never constructed on a real job, and #12243's data-loss scenario is unchanged. "Per-table bucket routing through the normal `MultiTableSink` creation path" is the right shape of fix for this. - **Issue 2** (High, via @goutamadwant): the partitioning block runs after `BroadcastSchemaSinkOperator` on the same stream, so a zero-field schema-control row hits `RowConverter.reconvert` and throws `ArrayIndexOutOfBoundsException` — and even fixed for the crash, routing a control signal through the data partitioner risks some sink subtasks never receiving it. Routing control rows by `schema_subtask_id` instead (which is what you describe) is the fix @goutamadwant already suggested and I agreed with. On scope: I'd lean toward keeping the core partitioning fix (addressing Issues 1 and 2) as the priority for closing #12243, and treating global-commit recovery as a separate concern unless it's actually required for the fixed-bucket routing fix to be correct — but that's ultimately a call for @zhangshenghang and you to make together, not something I need to gate on. No new commit has landed on this PR since my last review, so I'm not doing a fresh full pass right now. Whichever of you ends up pushing the update — whether incorporated here or as a follow-up — I'll do a full re-review once real commits land, focused on whether the routing decision now actually reaches the `MultiTableSink`-wrapped path and whether schema-control rows are excluded from the data partitioner, plus test coverage that exercises both (the current `PaimonFixedBucketPartitionerTest` unit test can't catch either issue, since it never goes through `tryGenerateMultiTableSink` or a control-row stream). -- 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]
