DanielLeens commented on PR #11721: URL: https://github.com/apache/seatunnel/pull/11721#issuecomment-5409369643
Thanks for the thorough writeup — and welcome to the project, this is a really solid first contribution to have shepherded through this many rounds. I independently re-verified the two claims in your comment before replying, rather than taking them on trust: 1. **The dev-merge is content-identical.** I diffed `f58b34c6cc0` (the head my last review at 10:34 UTC was against) directly against the current head `aea9854a1bb` — the only files touched are the unrelated `MysqlCDCCheckpointRestoreIT.java` addition and the one-line `JobStateEventTest.java` fix from `#11954`. All four files this PR actually changes (`MultiTableSinkWriter.java`, `MultiTableSinkWriterTest.java`, and the two `multi-table.md` docs) are untouched. So yes — my `Ready to merge` conclusion from that review carries over to `aea9854a1` unchanged, and this doesn't need a fresh full re-review. 2. **The root cause of the earlier red build.** Confirmed independently too: `dev` at `406c66789` is missing `FAILED_JOB_EVENT_TIMEOUT_SECONDS` and `43fe63b1f` (`#11954`) adds it back as `RESTORE_TO_FAILED_TIMEOUT_SECONDS` — that's a `dev`-side breakage window, nothing about this PR's diff. One thing worth flagging before this merges, since the fresh run you linked (`SEPURI-SAI-KRISHNA/seatunnel#32746106023`, head `aea9854a1b`) finished with `conclusion: failure` after your comment posted, and you said you'd only follow up if something failed for a reason that's genuinely this PR's — I checked, so you don't have to duplicate the digging: - `unit-test (8, ubuntu-latest)` failed on exactly one test: `org.apache.seatunnel.engine.server.CoordinatorServiceTest#testPendingJobSchedulerCanAdvanceNextJobWhenPreviousResourceCheckBlocks`, `expected: <true> but was: <false>` at `CoordinatorServiceTest.java:594`. That's a `seatunnel-engine-server` pending-job-scheduler/resource-check test with no relationship to `MultiTableSinkWriter`'s queue-routing hash — reads like a timing-sensitive assertion on the resource-check-blocks path rather than anything this PR's diff could affect. - `kudu-connector-it (11, ubuntu-latest)` was `cancelled` rather than failed, consistent with an infra/container hiccup rather than a real regression. Given neither touches a file in this PR's diff, I'd treat both as environmental rather than something to chase down here — but worth a rerun on your fork before the final merge-readiness check, just so the record shows a clean run at the head that actually lands. My approval stands either way; this is CI-noise triage, not a review blocker. On your responses to the non-blocking notes (Issues 1–3 from my last review): all three of your calls make sense to me — keeping the `RealtimeMetricsService` defense-in-depth fix and the shared `nonNegativeMod` helper as separate follow-ups rather than scope-creeping this one-line fix is the right call, and I'd rather not see a comment-only commit either given the branch is otherwise merge-ready. Thanks also for redoing the sibling-pattern audit yourself and catching the Bigtable/Pulsar/RocketMQ/Kafka-sink membership correction — that's exactly the kind of follow-through that makes a re-review easy to trust. -- 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]
