SEPURI-SAI-KRISHNA commented on PR #11721:
URL: https://github.com/apache/seatunnel/pull/11721#issuecomment-5235697656
Thanks for the re-post, and for the depth here. Checking that `x &
Integer.MAX_VALUE` is non-negative for *every* `int` rather than just the
reported value, and tracing that the routing is never persisted through
`MultiTableSink.restoreWriter`, are exactly the two things that decide whether
this fix is safe — I appreciate you verifying both independently rather than
taking the description's word for it.
**Blocker 2 (stale architecture doc) — fixed.** Both snippets in
`docs/en/architecture/features/multi-table.md` now show `(primaryKey.hashCode()
& Integer.MAX_VALUE) % replicaNum`, each with a one-line note on why `Math.abs`
is wrong here. That was a genuine miss: I updated the code and its Javadoc but
never grepped the docs for the formula, which is precisely how a documented bug
outlives the bug. The zh version needed no change — it states the rule as
abstract math (`replica = hash(pk) mod replicaNum`) and never showed the
`Math.abs` form.
**Issue 3 (test only covered `Integer.MIN_VALUE`) — done.** Added
`testOrdinaryNegativeHashRoutesToMaskedIndex`, which uses an ordinary negative
key (`-5`, `replicaNum = 3`) and asserts the *exact* destination writer rather
than just "does not throw":
- old routing: `Math.abs(-5) % 3` → `2`
- new routing: `(-5 & Integer.MAX_VALUE) % 3` → `0`
It asserts the row lands on index `0` **and** that index `2` — where the old
formula would have sent it — receives nothing, so the general bitmask is now
under regression coverage, not just the extreme edge. Both routing tests fail
when the one-line fix is reverted, in different ways:
```
testMinValuePrimaryKeyHashRoutesToValidQueue:577
Unexpected exception thrown: java.lang.IndexOutOfBoundsException: Index
-2 out of bounds for length 3
testOrdinaryNegativeHashRoutesToMaskedIndex:623
expected: <1> but was: <0>
```
Full `seatunnel-api` suite: `Tests run: 385, Failures: 0, Errors: 0,
Skipped: 0`.
**Blocker 1 (undisclosed scope)** — I think this one was already covered,
and I'd rather point at it than silently duplicate it. The PR description has
said this since it was opened, under *Does this PR introduce any user-facing
change?*:
> Keys with a non-negative hash are entirely unaffected — `h &
Integer.MAX_VALUE == h` for every non-negative `h`. Keys with a negative hash
may now land in a different queue than before, since the old expression
computed `-h` and the new one computes `h + 2^31`. […] Queue assignment is
computed per row at write time and is not persisted in checkpoint state, so
restoring a job taken before this change is unaffected.
That's the disclosure and the safety argument you asked to have on the
record. My guess is the section's opening line — "Only in a narrow and
unobservable sense, so: no" — reads as a dismissal and invited a skim, which is
a fair thing to have happen and my fault for framing it that way. I've now also
stated the scope up front in the **Purpose** section, so it can't be missed on
the way in, and the new test pins the behaviour rather than leaving it as prose.
If you'd still like the wording changed rather than just relocated, say the
word and I'll take your phrasing.
--
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]
