DanielLeens commented on PR #11512:
URL: https://github.com/apache/seatunnel/pull/11512#issuecomment-5412613694
Thanks for the follow-up, @SEZ9 — same correction as I just had to make on a
sibling PR, so let me be precise here too.
My 2026-08-24 review was **not** cut off. I re-fetched its raw body via the
API: it's 9,278 characters (well under GitHub's ~65,536-char limit) and runs
all the way through a complete `# 5. Merge Recommendation` section ending in a
full "Overall assessment" paragraph. Both of your questions are already
answered explicitly in it:
1. **Enum wire-representation concern**: "SEZ9 Issue 1 (Medium) ...
Resolved, and precisely on-target." The four new tests in
`ReportCdcProgressOperationSerializationTest` —
`testEveryProgressOwnerRoundTripsByName`,
`testEveryProgressLifecycleRoundTripsByName`,
`testEveryProgressAccuracyRoundTripsByName`,
`testEverySnapshotAssignmentStatusRoundTripsByName` — each loop every constant
of their enum and round-trip it through the real serializer, which is exactly
the regression-lock you asked for.
2. **Second open item**: "SEZ9 Issue 2 (Low) ... I consider this fully
resolved." I read the current file directly: `CdcSnapshotSplitProgress` was
already `public final class` with all-`final` fields before `a88058bc0e`; that
commit only added the Javadoc making the existing guarantee explicit.
So: no new push needed on your end for either item — both are closed as of
`82dee7f2b1`.
One thing that *is* new since my last review: the current head's CI has now
finished (it was still `IN_PROGRESS` when I posted), and it's red — but not for
a reason in this PR's own diff. I traced the fork run (`goutamadwant/seatunnel`
run `32676082024`, head `82dee7f2b1`) directly: every failing job — `unit-test`
across all four matrix legs, plus a long list of connector-IT jobs — fails on
the identical error:
```
[ERROR]
.../seatunnel-engine-server/src/test/java/.../event/JobStateEventTest.java:[165,23]
error: cannot find symbol
symbol: variable FAILED_JOB_EVENT_TIMEOUT_SECONDS
```
This PR's diff doesn't touch `JobStateEventTest.java` at all (confirmed via
`git diff dev...HEAD --stat` for that path — empty). It's a pre-existing bug
carried in through the ~100-commit `dev` sync this PR's merge pulled in. `dev`
has since fixed it independently at `43fe63b1fc` ("[Fix][Zeta] Fix undefined
job event timeout constant", #11954), merged 2026-08-24 after this PR's
dev-sync point (the fix simply swaps the undefined
`FAILED_JOB_EVENT_TIMEOUT_SECONDS` for the already-imported
`RESTORE_TO_FAILED_TIMEOUT_SECONDS`).
@goutamadwant — merging `dev` again (or rebasing) should pick that fix up
and should get you a clean run; I don't see anything in the current failures
that traces back to this PR's own `a88058bc0e` commit.
--
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]
