SEPURI-SAI-KRISHNA commented on PR #11721: URL: https://github.com/apache/seatunnel/pull/11721#issuecomment-5358961311
Thanks @SEZ9, no apology needed, and thanks for picking this up. Confirmation pass on all three, checked against the current head `7cfabc8` rather than from memory. **1. Docs snippet, partially, and I'd like to scope the rest out.** Both `Math.abs` occurrences in `docs/en/architecture/features/multi-table.md` are fixed at this head (the `selectReplica` body at ~L284 and the "Hash-Based" summary at ~L399), each with a comment naming the `Integer.MIN_VALUE` trap. What I did *not* do is reshape the snippet to mirror `MultiTableSinkWriter`, because the drift is much wider than the routing line: the whole block is illustrative pseudocode. It shows a `selectReplica(TablePath, SeaTunnelRow)` method, `extractPrimaryKeyIfPresent(row)`, a `replicaNum` field, `writers.get(identifier)`, and `System.nanoTime() % replicaNum`, none of which exist. `extractPrimaryKeyIfPresent` has **zero** occurrences in the repo, and `replicaNum` is not a field on `MultiTableSinkWriter`. The real `write(SeaTunnelRow)` uses `element.getField(primaryKey.get())`, `blockingQueues.size()`, and `offerRowElement(index, element)`. Rewriting that into a faithful mirror is a docs change of its own, unrelated to this bug, and it would bury a 1-line correctness fix in a page rewrite. I'd rather fix the wrong line here and track the mirror as a follow-up. Happy to open that issue. (The `zh` counterpart needs no change, `docs/zh/architecture/features/multi-table.md` expresses this as the formula $replica = hash(pk) \bmod replicaNum$, with no Java snippet, so it never carried the `Math.abs` bug.) **2. Shared helper, deliberately not, and I think inline is the more consistent choice here.** `(hash & Integer.MAX_VALUE) % n` is already this codebase's established idiom, written inline everywhere it appears, **15 production call sites across 14 modules**, none behind a helper: `connector-jdbc`, `connector-kafka`, `connector-paimon`, `connector-iceberg`, `connector-mongodb`, `connector-hbase`, `connector-typesense`, `connector-amazondynamodb`, `connector-easysearch`, `connector-pulsar`, `connector-rocketmq`, `connector-fluss`, `connector-cdc` (tidb), and `seatunnel-engine-server` (`HazelcastMetricsSnapshotStateStore`). Introducing a helper for what would be its single caller would make this the one site that *doesn't* look like the other fifteen. More to the point on the trap itself: after this PR, `Math.abs(...hashCode()) %` has **zero** remaining occurrences in production code repo-wide, the routing line and its Javadoc were the last two. So the pattern isn't reintroducible-by-copy from anywhere; every remaining example a contributor could copy is already the masked form. If a shared helper is still wanted as a convention, it should land as its own change that converts all 16 sites at once, not as a one-caller helper bolted onto a bugfix. **3. Exact-index assertion, already present at this head.** That assertion exists, in the sibling test this PR adds alongside the MIN_VALUE one. `testOrdinaryNegativeHashRoutesToMaskedIndex` pins the index exactly in both directions: ```java int expectedIndex = (primaryKey & Integer.MAX_VALUE) % replicaNum; // -5 -> 0 Assertions.assertEquals(1, writersByIndex[expectedIndex].getWriteCount()); // Index 2 is where the old Math.abs routing would have sent this row. Assertions.assertEquals(0, writersByIndex[2].getWriteCount()); ``` It asserts both that the row lands where the new routing says and that it does *not* land where `Math.abs(-5) % 3 == 2` would have put it, so it fails if the fix is reverted, not merely if it crashes. `testMinValuePrimaryKeyHashRoutesToValidQueue` is intentionally the crash-path test, since the reported symptom was an `IndexOutOfBoundsException`: it drives both `Integer.MIN_VALUE` and `Long.MIN_VALUE` (both hash to `Integer.MIN_VALUE`) through `write()` and asserts no throw plus a full write count. I'm happy to tighten it to assert queue `0` exactly, `(Integer.MIN_VALUE & Integer.MAX_VALUE) % 3 == 0`, so both rows land there, if you'd prefer it. It's a 2-line change. One scheduling note if you do want that tightening: no valid approval currently stands, so a push right now costs nothing, whereas the same push after you approve would be auto-dismissed again by branch protection and put us back here. So if you want item 3 changed, say so and I'll push it before you approve. If you're happy to take items 1 and 3 as follow-ups, the head is unchanged at `7cfabc8` and ready as-is. -- 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]
