SEPURI-SAI-KRISHNA commented on PR #11721:
URL: https://github.com/apache/seatunnel/pull/11721#issuecomment-5367204778
Thanks @SEZ9. Both items done.
**F3 — assertion added, and it turns out it was not redundant.**
Pushed. `testMinValuePrimaryKeyHashRoutesToValidQueue` now pins the target
queue exactly instead of only asserting no-throw plus a total count:
```java
// Under the fix both rows route to (Integer.MIN_VALUE & Integer.MAX_VALUE)
% 3 == 0.
int expectedIndex = (Integer.MIN_VALUE & Integer.MAX_VALUE) % replicaNum;
Assertions.assertEquals(0, expectedIndex);
...
Assertions.assertEquals(2, writersByIndex[expectedIndex].getWriteCount());
for (int i = 0; i < replicaNum; i++) {
if (i != expectedIndex) {
Assertions.assertEquals(0, writersByIndex[i].getWriteCount());
}
}
```
You were right to keep pushing on this rather than accept the sibling test
as cover. I checked whether the assertion actually buys anything by mutating
the routing line to `Math.abs(object.hashCode() % blockingQueues.size())` — a
plausible alternative fix that is *non-throwing* and sends `Integer.MIN_VALUE`
to queue 2 instead of queue 0:
* old test (no-throw + total write count): **passes** — the bad routing
slips straight through
* new test: **fails**, `expected: <2> but was: <0>` at the new assertion
So the gap you identified was real and is now closed. The sibling test does
cover ordinary negative hashes, but it uses key `-5`, which is not the
`Integer.MIN_VALUE` path, so it would not have caught that mutant on this input
either. Both tests fail under it now.
Full class is green at the new head: `Tests run: 28, Failures: 0, Errors:
0`. Only the test file changed, `+14/-1` inside that one method —
`MultiTableSinkWriter.java` and `multi-table.md` are byte-identical to the
previous head, so the reviewed fix itself is untouched.
(Minor: my earlier reply wasn't truncated — item 3 was in it, under **3.
Exact-index assertion**. Doesn't matter now, since the change is in.)
**F1 — follow-up issue filed:
https://github.com/apache/seatunnel/issues/11923**
Tracked as offered, so the deferral isn't lost.
One thing I found while writing it up that's worth flagging here, because it
slightly changes the picture: the same page documents the no-primary-key
strategy as
```java
int replica = (int) (System.nanoTime() % replicaNum);
```
`System.nanoTime()` is explicitly documented as possibly negative, so that
expression can produce a negative index too. It is the *same* bug class this PR
fixes on the line directly above it. I did not fold it into this PR — widening
a 1-line correctness fix that has already been through several review rounds
would restart the clock for no good reason — but it means the page carries two
instances of the negative-index trap and this PR only removes one. It is
covered in the follow-up issue.
**F2** — thanks for withdrawing, and for saying why; that was a fair thing
to ask.
That should settle both remaining asks. CI is re-running on the new head
`11599d9` (the previous head was green, and this delta is test-only), after
which the branch protection re-approval is the only thing left.
--
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]
