DanielLeens commented on PR #10678: URL: https://github.com/apache/seatunnel/pull/10678#issuecomment-5279186452
Thanks everyone for keeping this thread active. Since there's been no new commit since my last full review, I want to separate out what's actually changed vs. what's still open. **1. CI gate — resolved.** At the time of my last review (https://github.com/apache/seatunnel/pull/10678#pullrequestreview-4922963075) the fork's `Build` run for this exact head (`bbb9245454c0`) was still `in_progress`. It's since completed and shows `SUCCESS`. That item is cleared. **2. @nzw921rx's lifecycle-model proposal and follow-up APPROVED review.** I read the Flink-style "lifecycle-first" suggestion as a genuinely new, forward-looking idea, not a reopening of the two blockers you and I traded earlier (the JDK 9+ `--add-opens` gap and the JDK 8 `useCaches` global-mutation issue) — those are confirmed fixed on this head, which is why you approved them separately. You and @knight6236 both converged on treating the lifecycle-ownership-model direction as a non-blocking follow-up for #10669 rather than a redesign requirement for this PR, and I agree with that framing: opt-in deep-clean as Phase 1/2 preparation and a future explicit ownership/lifecycle model aren't mutually exclusive as implemented today. +1 to continuing that discussion in #10669 rather than gating this PR on it. **3. @knight6236's status-summary comment still leaves one item from my last review open: the documentation gap (my Issue 1, Medium).** I flagged this as a blocker specifically because it's the one item that's been outstanding since @SEZ9 first raised it several rounds ago. "Deliberately deferred to Phase 3, off by default" doesn't fully close it for me: `SEATUNNEL_CLASSLOADER_DEEP_CLEAN` is a real, functioning switch on this head, not a stub — anyone who reads the code (or an ops runbook referencing it) and opts in today gets working deep-clean behavior with genuine JDK-version-dependent caveats (the two `--add-opens` flags, the JDK 8 global `useCaches` trade-off). That risk isn't hypothetical just because the default is off, and it doesn't need to wait for a dedicated Phase 3 documentation PR — a short paragraph in `docs/en`/`docs/zh` (property name, default `false`, both `--add-opens` flags, and a one-line "experimental, subject to change before Phase 3" caveat) would close it now. To summarize where this stands: no source-level blocker remains from my side — both blockers from my last review are confirmed fixed on `bbb9245454c0` — CI is green, and I'm aligned with tracking the lifecycle-model discussion separately in #10669. The one item I'd still like to see land before merge is the docs paragraph for `SEATUNNEL_CLASSLOADER_DEEP_CLEAN`. Happy to approve as soon as that's in. -- 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]
