SEPURI-SAI-KRISHNA commented on PR #11925:
URL: https://github.com/apache/seatunnel/pull/11925#issuecomment-5380270635
Thanks @DanielLeens — you're right, and this one is squarely on me.
I took **Option A**. Pushed.
All three spots now show what `dev` actually does:
```java
index = Math.abs(object.hashCode()) % blockingQueues.size();
```
and the negative-index defect is flagged as a known issue pointing at
#11720, rather than presented as already fixed:
> **Known issue** — `Math.abs(Integer.MIN_VALUE)` returns
`Integer.MIN_VALUE`, which is still negative, so a key whose hash is exactly
`Integer.MIN_VALUE` produces a negative index whenever the queue count is not a
power of two, and the subsequent `blockingQueues.get(index)` throws
`IndexOutOfBoundsException`. This page documents the behaviour currently on
`dev`; the defect is tracked in #11720, and this section should be updated when
a fix lands.
Same in `zh`, where the §5.3 formula goes back to $replica =
\mathrm{Math.abs}(hash(pk)) \bmod replicaNum$ with the matching callout. Diff
is +12/-4 en, +11/-3 zh; nothing else in the page moved.
I verified the §4.2 snippet is now byte-identical to
`MultiTableSinkWriter.java:619` rather than eyeballing it, and grepped both
files to confirm no `Integer.MAX_VALUE` masking reference survives anywhere.
Two things worth saying plainly:
**Your diagnosis of how it happened is exactly right.** I wrote the page
against #11721 rather than against `dev`, which is precisely the failure mode
the PR description promised it had avoided. I did flag the overlap on #11923
and offered to hold this PR, but that was not good enough — I still shipped the
code sample in the post-fix form and left it to a reviewer to catch, which put
the burden in the wrong place.
**I picked A over B deliberately**, and I want to be explicit about the
tradeoff since you left it open. Option B (hold until one of
#11721/#11725/#11859 lands) would make this page correct with no callout, but
it makes a docs fix hostage to a code fix that has been sitting on
branch-protection approval for a while, and #11720 has three competing PRs with
no signal yet on which one is preferred. Option A is correct *regardless* of
which one merges or when. The cost is that whoever merges the eventual fix
should drop the callout — the note says so in-line, and I'm happy to do that
follow-up myself the moment one lands.
I linked #11720 rather than the three PRs on purpose: the issue is the
stable reference, and pointing readers at three competing fixes from the
architecture docs seemed like the wrong thing to bake into the page.
Thanks for checking the rest of it against source line by line — that's a
lot of verification work, and it's the reason the one wrong claim didn't slip
through.
--
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]
