SEPURI-SAI-KRISHNA opened a new pull request, #12395:
URL: https://github.com/apache/seatunnel/pull/12395
### Purpose of this pull request
Relates to #11976.
`AmazonDocumentDBSourceSplitEnumerator.getSplitOwner` still hand-rolls the
masking spelling that #11987 consolidated into `HashUtils.bucketIndex`:
```java
private static int getSplitOwner(Integer splitId, int readerCount) {
return (splitId.hashCode() & Integer.MAX_VALUE) % readerCount;
}
```
It was not covered by #11987 because this connector had not landed at the
time. This is the same one-line migration #12049 made for the AzureCosmosDB
enumerator, which is the closest precedent and has the identical diff shape.
I checked what is left rather than assuming: a code search for the
hand-rolled spellings on current `dev` returns `HashUtils` itself, its test,
`multi-table.md`, `MultiTableSinkWriter` (migrated by the open #12290), and
this file. `AmazonDynamoDBSourceSplitEnumerator` has a `getSplitOwner` too, but
its body is `assignCount % numReaders` with no hash involved, so it is
correctly not a site. Once this and #12290 land, no production site spells it
by hand and #11976 can close.
### Does this PR introduce _any_ user-facing change?
No, and I want to be precise about that rather than lean on "no" alone.
`bucketIndex` applies exactly the same `(hash & Integer.MAX_VALUE) %
bucketCount`, and `Integer.hashCode()` returns the value itself, so every split
id routes to the reader it routed to before. This is a consolidation, not a fix.
The one observable difference is on a non-positive reader count, where
`bucketIndex` calls `checkBucketCount` and raises `IllegalArgumentException`
naming the value, rather than `ArithmeticException: / by zero` at zero or a
quietly wrong index at a negative count. That path is not reachable here, since
`readerCount` comes from `currentParallelism()`.
### How was this patch tested?
`./mvnw -q -DskipTests verify -pl
seatunnel-connectors-v2/connector-amazondocumentdb` on JDK 11 passes, covering
the enforcer checks, `spotless:check` and compilation. `spotless:check` was
also run separately with up-to-date caching disabled. `seatunnel-common`
resolves through the existing `connector-common` dependency, so no `pom.xml`
change was needed.
`./mvnw -pl seatunnel-connectors-v2/connector-amazondocumentdb test`: **22
tests, 0 failures, 0 errors**, with `AmazonDocumentDBSourceSplitEnumeratorTest`
going from 5 to 7.
The two new tests are added to that existing class rather than a new one,
and mirror the pair #12049 added:
* `testSplitOwnerRoutesSplitIdsByBucketIndex`, where split ids 4 and 5
across 3 readers land on readers 1 and 2
* `testSplitOwnerKeepsIntegerMinValueSplitIdInRange`, the input the sign-bit
masking exists for
Since this change is behaviour preserving, an ordinary mutation check proves
nothing, so I ran two that do:
* reverting `getSplitOwner` to the original hand-rolled line leaves **all 22
tests passing**, which is the evidence for the equivalence claimed above
* replacing it with `Math.abs(splitId.hashCode()) % readerCount` fails
**only** `testSplitOwnerKeepsIntegerMinValueSplitIdInRange`, since
`Math.abs(Integer.MIN_VALUE)` is itself negative
So the tests do not demonstrate a fix, because there is no fix to
demonstrate. They pin the routing contract against exactly the regression
`bucketIndex` was introduced to prevent.
--
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]
