SEPURI-SAI-KRISHNA commented on PR #11721:
URL: https://github.com/apache/seatunnel/pull/11721#issuecomment-5397842179
Reporting back on CI as promised, @SEZ9 @DanielLeens — and thanks for the
fresh from-scratch re-review, @DanielLeens, particularly the repo-wide re-audit
for the same bug class rather than trusting my PR description's count.
**Short version: the red build on `f58b34c6c` was not this PR. It was a
~3-hour window where `dev` itself did not compile. I have re-merged current
`dev` and the branch is now at `aea9854a1`.**
## What actually failed
71 of 91 jobs failed on that run, all with the same error, including plain
`unit-test` on all four JDK/OS matrices:
```
seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/event/JobStateEventTest.java:[165,23]
error: cannot find symbol
symbol: variable FAILED_JOB_EVENT_TIMEOUT_SECONDS
location: class JobStateEventTest
```
`seatunnel-engine-server`'s `testCompile` fails, so every module downstream
of it dies before running a single test.
@DanielLeens — you asked specifically about `all-connectors-it-1`, so to
answer it directly rather than by inference: it failed at that exact line, in
the `run connector-v2 integration test (part-1)` step, with no connector-level
failure of its own. Same for the other 70.
## Why it hit this branch
| Time (UTC) | Event |
|---|---|
| 2026-08-24 08:48 | this branch merges `dev` at `406c66789` — already
broken |
| 2026-08-24 11:50 | `dev` is fixed by `43fe63b1f`, [Fix][Zeta] Fix
undefined job event timeout constant (#11954) |
The merge landed inside the window. `dev` had the bad symbol at `406c66789`
and has `RESTORE_TO_FAILED_TIMEOUT_SECONDS` today, so nothing about this PR is
implicated — its diff touches four files and none of them are in
`seatunnel-engine-server`. For completeness I checked #11927 and #11937 too:
both have bases predating the breakage, so only this branch caught it.
## What I changed
Re-merged `upstream/dev` (now `80b24dc8d`). Only two commits had landed
since the old merge base and neither touches any file in this PR, so the merge
was conflict-free and brought in exactly the `43fe63b1f` fix plus an unrelated
CDC e2e test.
**No source or doc change.** All four files in the diff are byte-identical
to the `f58b34c6c` state you reviewed — I verified this by blob SHA rather than
by eye, so your re-review conclusions carry over to `aea9854a1` unchanged. The
diff is still +120/-24 across the same four files.
Fresh full run underway:
https://github.com/SEPURI-SAI-KRISHNA/seatunnel/actions/runs/32746106023 — I
will follow up here only if anything fails for a reason that is genuinely this
PR's.
## On your non-blocking notes
- **Issue 1** (`RealtimeMetricsService.decodeQueueTargetVertexId`,
`Math.abs` on a `long`): agreed on both counts — same shape, and unreachable
given `actionId` is a bounded DAG-vertex sequence number. Happy to send it as a
separate defense-in-depth change; deliberately not folding it into this diff.
- **Issue 2** (Javadoc on `testOrdinaryNegativeHashRoutesToMaskedIndex`):
happy to add it if you would like it in this PR, though I would rather not push
a commit for a comment-only change while the branch is otherwise ready to
merge. Your call.
- **Issue 3** (shared `nonNegativeMod` helper, +1 from SEZ9): agreed this is
the real fix for the underlying duplication, and your audit finding that all
~13 sites hand-roll the mask is exactly the argument for it. It spans several
unrelated modules, so I would rather land it as its own PR once this one is in,
than widen a one-line bug fix into a repo-wide refactor.
On the PR-description nit: you are right, and I checked all four rather than
taking it on trust — DynamoDB, TiDB CDC, Easysearch and Typesense all use
`return assignCount % numReaders;` over a monotonic `AtomicInteger`, which is a
counter, not a hash, and is safe for a different reason. I have corrected the
description.
Re-running the audit myself to fix the list turned up two things worth
adding to yours. The count of 12 happens to survive, but the membership
changes: dropping those four and adding
`BigtableSourceSplitEnumerator.java:386`, `PulsarSplitEnumerator.java:232`,
`RocketMqSourceSplitEnumerator.java:138`, and the Kafka **sink**'s
`MessageContentPartitioner.java:52` (distinct from the Kafka source enumerator
you already listed) gets back to 12. Bigtable is a genuine routing site spelled
across two lines, which is why a single-line grep misses it, and RocketMQ uses
the `((x * 31) & 0x7FFFFFFF) % n` variant. There is also a thirteenth masking
use in `CatalogTableUtils.java:84`, but it builds a primary-key *name* suffix
rather than routing anything, so I have left it out of the count.
Either way this independently confirms your headline finding: after this PR
there is no `Math.abs(hash) % n` left anywhere in main code. The only surviving
`Math.abs` on a routing-ish value is the `RealtimeMetricsService.java:722` site
you flagged as Issue 1.
--
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]
