DanielLeens commented on PR #11718:
URL: https://github.com/apache/seatunnel/pull/11718#issuecomment-5385689367
Thanks for the deep second pass, @SEZ9 — I re-checked the current head
(`d73c2b90bfdf`) against your eight points rather than taking them at face
value, since a couple are concrete/checkable claims.
**Issue 5 doesn't hold up.** I pulled the raw file at this head:
`org.junit.jupiter.api.TestInstance` is imported on line 37, and the class
carries `@TestInstance(TestInstance.Lifecycle.PER_CLASS)` on line 51. `{@link
TestInstance.Lifecycle#PER_CLASS}` in the method Javadoc (line 117) resolves
against that import exactly as any other in-file `{@link}` would — there's no
missing import here, so I don't think this is a real Javadoc-lint issue.
**Issue 2 is worth a closer look, and it's partially right.** I checked
`TestContainer`'s interface: `executeJob(String, List<String>)`,
`restoreJob(...)`, and the two `restoreJobWithCheckpoint(...)` overloads are
separate entry points from the single-arg `executeJob(String)` this PR
overrides. Today that's inert — Daniel's (my) prior review confirmed all eight
call sites in `PaimonWithS3IT` exclusively use the overridden signature, so
nothing in this class currently bypasses the bound. But you're right that the
Javadoc's "a job submission added later is bounded without anyone having to
remember to wrap it" promise is broader than what's actually guaranteed: it
only holds if that future submission also uses the `executeJob(String)`
signature. I'd treat this as a docs-precision nit (narrow the claim to "a
submission via `executeJob(String)`") rather than a functional gap in this
diff, since there's no such call today to leave unbounded.
Issues 1, 3, 4, 6, 7, and 8 all read as reasonable, non-blocking
robustness/documentation follow-ups to me — in the same spirit as the
trade-offs the PR's own Javadoc already discloses (abandoned worker, suppressed
thread-leak check, teardown noise). None of them contradict the core fix's
correctness: the timeout still fires reliably on a hang, the message still
names the cause, and the 20x healthy-run headroom I verified earlier still
holds. I'd be comfortable seeing 1/2/6 picked up as a quick follow-up commit
(best-effort in-container kill on timeout, scoping the Javadoc claim, and
calling out the IDE/no-ForkedBooter caveat) since they touch the same file and
are cheap, while 3/4/7/8 feel like fine-grained polish that could go either way
without blocking.
To be clear on my own position: this doesn't change the conclusion in my
last review at this exact head — no correctness blockers in the reviewable diff
— but I recognize that's my read, not a merge decision, and I'll leave the
"final approval pending" call to whoever picks this back up. Happy to take
another pass if the author pushes a commit addressing any of these.
--
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]
