SEPURI-SAI-KRISHNA commented on PR #11721: URL: https://github.com/apache/seatunnel/pull/11721#issuecomment-5381443914
Thanks @SEZ9. I think this pass may have re-run against the pre-resolution finding list, because both remaining items were closed out in your own comment on this thread ([2026-08-21](https://github.com/apache/seatunnel/pull/11721#issuecomment-5366233534)) — F1 with a specific ask that I completed the same day, and F2 explicitly withdrawn. Details below so it's easy to confirm, and I'm happy to act on either if you've changed your mind. ## F1 — the deferral you asked for is filed, linked, and now implemented Your ask was: > **Remaining ask:** please open the follow-up issue for the docs-mirror rewrite as offered and drop the link in this thread so the deferral is tracked, not lost. Done that day: **#11923**, linked in [my reply](https://github.com/apache/seatunnel/pull/11721#issuecomment-5367204778) under *"F1 — follow-up issue filed"*. It has since gone past tracking into implementation: **#11925** rewrites exactly the block you're pointing at, replacing `extractPrimaryKeyIfPresent` / `% replicaNum` with the real `element.getField(primaryKey.get())` and `% blockingQueues.size()`, and relabelling the listing as simplified rather than as class source. It's had a full review round from @DanielLeens; his one blocking finding is addressed and pushed. So your diagnosis is right about the current state of this branch, and the remedy is already open as its own PR rather than buried in this 1-line fix — which was the scoping you agreed with. Your own wording allows for exactly this route: > A tracked follow-up rewrite of the snippet is an acceptable way to close this out, as long as the block stops claiming to be the real implementation. #11925 is that rewrite, and it does make the block stop claiming to be the real implementation. If you'd rather not wait on it, the other remedy you listed — labelling the two blocks here as simplified pseudo-code rather than class source — is a small change I have ready and can push in a few minutes. Say the word. I've held off only because #11925 replaces both blocks outright, so the label would be deleted within a PR or two, and pushing here dismisses @DanielLeens's approval for something cosmetic. Happy to spend both if you want the belt and braces. ## F2 — withdrawn, and the survey holds up Your words: > **F2 (shared non-negative-mod helper):** Withdrawn. Your survey of 15 inline call sites across 14 modules is persuasive — a single-caller helper would make this the one inconsistent site, and the actual trap (`Math.abs(...hashCode()) %`) is now gone from production code, which was the real goal. I re-ran that survey against the current tree to be sure I wasn't leaning on a stale number. It's slightly sharper than what I originally reported: | Inline hash-routing call sites in main source | Count | |---|---| | Total, across **15 maven modules** | 16 | | Already using `& Integer.MAX_VALUE` inline | 13 | | Using `& 0x7FFFFFFF` — the same mask in hex (`KafkaSourceSplitEnumerator`, `PulsarSplitEnumerator`) | 2 | | Using `Math.abs` | **1** — the `MultiTableSinkWriter` line this PR fixes | Once this merges, 16 of 16 use the sign-bit mask inline and none use `Math.abs`. So the inline form is the established convention across `connector-jdbc`, `connector-kafka`, `connector-iceberg`, `connector-paimon`, `seatunnel-engine-server` and nine more; a helper adopted by exactly one of those sixteen would make `MultiTableSinkWriter` the odd one out rather than the model. I do think a `nonNegativeMod` helper is a reasonable idea *as a codebase-wide migration* — 16 sites, one tested invariant, no exceptions. That's a self-contained refactor PR touching 15 modules. I'm willing to open it as a follow-up if you want it; it just shouldn't ride on a 1-line correctness fix that's already been through several review rounds. ## One coordination note, so the two PRs don't look like drift #11925 currently documents the routing line as `Math.abs(object.hashCode()) % blockingQueues.size()` **on purpose**. @DanielLeens asked for that in [his review](https://github.com/apache/seatunnel/pull/11925#pullrequestreview-4999794379), on the grounds that a docs-fidelity PR must describe what `dev` does today, and this fix hasn't merged — with the negative-index defect flagged inline as a known issue pointing at #11720. So the two PRs are deliberately in opposite states right now: this one changes the code, that one documents the code as it stands until this one lands. Whichever merges first, I'll update the other the same day — #11925's callout says so in-line. Flagging it because two reviewers looking at the two PRs in isolation could reasonably read it as the docs drifting again. ## Where that leaves things F1 and F3 are done on this branch, F2 you withdrew. If the helper in F2 is something you now want after all, say so and I'll open it as its own PR across all 16 sites — I'd just rather not bolt a 15-module refactor onto a one-line correctness fix. The branch is unchanged at `11599d9` and green, and @DanielLeens's approval stands on that head. The remaining blocker is an approval under branch protection, which I can't move myself. -- 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]
