SEPURI-SAI-KRISHNA commented on PR #11721:
URL: https://github.com/apache/seatunnel/pull/11721#issuecomment-5412447318
Thanks both — @SEZ9 for re-checking the carried-over findings, @DanielLeens
for the CI triage and the kind words.
## F1 — resolved at the current head
You marked this UNCERTAIN pending confirmation that `aea9854a1` reflects the
rewritten listings. It does. Concretely, at that head:
| Check | `docs/en/...multi-table.md` | `docs/zh/...multi-table.md` |
|---|---|---|
| `extractPrimaryKeyIfPresent` occurrences | **0** | **0** |
| `% replicaNum` routing expressions | **0** | 0 (see note) |
| hash routing, walkthrough | `:311` `(object.hashCode() &
Integer.MAX_VALUE) % blockingQueues.size()` | — |
| hash routing, §5.3 | `:405` same expression | `:198` |
| key extraction | `:306` `Object object =
element.getField(primaryKey.get());` | — |
So the two identifiers the finding is about are gone, and the routing line
matches `MultiTableSinkWriter.java:622` token for token. The `int replica = ...
% replicaNum;` line your Evidence block cites at `:403` no longer exists in the
file.
On the second half of the finding — the block claiming to be class source —
that is also already addressed, at `:249-253`, immediately above the listing:
> The listing below keeps the real class, field, and method names so it can
be read side by side with `seatunnel-api/.../MultiTableSinkWriter.java`. It is
**simplified**: schema-change short-circuiting, quarantine checks, retry, and
exception wrapping are elided. It is not a copy of the source.
That is the second remedy you offered ("or clearly label the block as
simplified pseudo-code rather than class source"), so both paths you named are
now satisfied.
**One thing worth surfacing rather than glossing over.** The zh doc's §5.3
formula reads `\bmod replicaNum` where the en doc says `blockingQueues.size()`.
Those are the same number, not a divergence: `MultiTableSink.java:147` passes
`replicaNum` as the `queueSize` argument, and `MultiTableSinkWriter` creates
exactly one queue per `queueSize` (`:220-244`), so `blockingQueues.size() ==
replicaNum` by construction. The zh text describes it in replica terms because
the surrounding section is about replica tuning. Happy to switch it to
`blockingQueues.size()` for cross-language symmetry if you would prefer the
code-level name in both — say the word and I will, it is a one-line change.
## F2 — agreed, and I would like to do it properly rather than partially
No disagreement on the substance: hand-rolling the idiom is how the
`Math.abs` variant survived here in the first place. @DanielLeens raised the
same point independently and we all seem aligned.
My reason for keeping it out of this PR is scope, not reluctance. When I
redid the sibling audit myself I found **12 other hash-routing sites** across
`connector-paimon`, `-hbase`, `-fluss`, `-mongodb`, `-iceberg`, `-jdbc`,
`-google-bigtable`, `-kafka` (source *and* sink), `-pulsar`, `-rocketmq`, and
`seatunnel-engine-server`. A `nonNegativeMod` helper is only worth adding if
those migrate to it, and that is a change touching eleven modules — it needs
its own PR and its own review, not a rider on a one-line bug fix. I will file
it once this lands, with the `Integer.MIN_VALUE` unit test you specified.
Worth noting the two are independent: this PR removes the last live instance
of the bug, and the helper prevents *future* reintroduction. Neither blocks the
other.
## CI
@DanielLeens — thank you for digging so I did not have to. I verified it
independently, reached the same place, and then reran the failed jobs as you
suggested. **The rerun settled it: `unit-test (8, ubuntu-latest)` passed on
attempt 2, and the run now has zero failing jobs** (80 success, 10 skipped, 1
cancelled).
That confirms the flake diagnosis rather than just asserting it. The failing
assertion was:
```
CoordinatorServiceTest.testPendingJobSchedulerCanAdvanceNextJobWhenPreviousResourceCheckBlocks:594
expected: <true> but was: <false>
```
Line 594 is `Assertions.assertTrue(firstScheduleStarted.await(5,
TimeUnit.SECONDS))` — a 5-second latch — and the reported elapsed time was
**5.012 s**, so it timed out rather than observing a wrong value. On attempt 1
the other three `unit-test` matrix legs already passed on identical code; on
attempt 2 the fourth passed too. One test out of 456, in the pending-job
scheduler, with no path to `MultiTableSinkWriter`'s routing hash.
`kudu-connector-it (11, ubuntu-latest)` is `cancelled` rather than failed,
and that one is a job timeout rather than anything test-related: it ran
12:31:05 → 14:01:35 UTC, exactly the `timeout-minutes: 90` set for that job at
`.github/workflows/backend.yml:1454`. Its JDK-8 twin ran **the same suite on
the same commit in 28 minutes and passed**, so this is a slow/stuck runner on
one matrix leg, not a regression. Happy to rerun that leg too if you would like
it green for the record, though it is untouched by this diff either way.
--
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]
