SEZ9 commented on PR #11721: URL: https://github.com/apache/seatunnel/pull/11721#issuecomment-5358318218
Thanks @SEPURI-SAI-KRISHNA and @DanielLeens for keeping the state straight here — and apologies for the confusing phrasing in my earlier comment; you're right that the re-approval it asked for is mine to action. The auto-dismissal is exactly what it looks like: branch protection dismissed my 08-16 `+1 if CI passes / LGTM` when the `upstream/dev` merge commit landed, even though that merge carried no net change to this PR's diff. The head is unchanged at `7cfabc8`, `Build`, `labeler`, and `Notify test workflow` are all `SUCCESS`, so the condition I set is met. I'll re-approve at this head. Before I do, one quick confirmation pass on the three LOW items from my review, since they were all soft asks: 1. **Docs snippet drift** (`docs/en/architecture/features/multi-table.md`) — please confirm the snippet now mirrors the actual `MultiTableSinkWriter` code (`blockingQueues.size()` / `element.getField`) rather than the older `replicaNum` / `extractPrimaryKeyIfPresent` shape. 2. **Shared non-negative-mod helper** — is the `(hash & Integer.MAX_VALUE) % n` idiom in `MultiTableSinkWriter.java` now behind a small shared helper, so the `Math.abs(Integer.MIN_VALUE)` trap can't be reintroduced elsewhere? 3. **Regression test precision** — does the `Integer.MIN_VALUE` test in `MultiTableSinkWriterTest.java` now assert the exact target queue like its sibling test, rather than just no-throw + total count? @DanielLeens noted the fix, docs, and test coverage held up under re-review at `7cfabc8`, so if all three are addressed at this head, nothing further is needed from you — I'll approve and we can merge, which also lets us close out the duplicates as you described. If any of the three is deliberately deferred, just say so and we'll track it as a follow-up rather than block on it. <!-- streview-comment:385 --> -- 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]
