DanielLeens commented on PR #12161: URL: https://github.com/apache/seatunnel/pull/12161#issuecomment-5770883252
Thanks @SEZ9 — head is still `93e11ea7f0a1` (no new commit since my last pass), so I re-checked your open items directly against that source rather than against the description, same as before. **F1/F2/F3/F5/F7 — confirmed addressed**, matching your read. This is the same conclusion I already reached independently twice (my full re-review, comment id 5162291171, and the point-by-point confirmation on 09-15, comment id 5674313772), and nothing has changed on the head since. **F4 — confirmed addressed, and the residual is exactly what we already flagged, not a new gap.** I re-pulled `BaseServiceHealthMetricsTest.java` at `93e11ea7f0a1`: `testInterruptStopsWaitingForRemainingMembers` sets the interrupt flag, runs `collectHealthMetrics` against one hanging + one resolved future, and asserts `values.size() == 0` (the loop breaks rather than continuing), that the hanging future is cancelled, and that the interrupt flag is restored (`Assertions.assertTrue(Thread.interrupted())`). That's a real regression test for the break-and-restore behavior, not just a code-path touch. The residual — members not yet processed get no marker entry at all rather than an explicit "skipped" marker — is the same non-blocking note I raised on 09-15 and you restated on 09-21; +1 on asking the author for a follow-up issue rather than blocking this PR on it. **F6 — answering your question directly from source, and it's unchanged from the first round:** `SeaTunnelHealthMonitorTest.java` at `93e11ea7f0a1` still only has the two reflection-based tests (`testPercentageStringIsLocaleStable`, `testNumberToUnitIsLocaleStable`) against the private static helpers `percentageString`/`numberToUnit`. It does **not** exercise `renderLoad`/`renderOperationService`, so that half of F6 is still open — no regression, just never addressed since we agreed it was non-blocking. On the second half of your question: yes, both tests already restore the default locale correctly (`Locale.setDefault(previous)` in a `finally` block), so there's no JVM-global-locale leak risk here even though `@ResourceLock(Resources.LOCALE)` was never added. Net: nothing outstanding here changes the picture. F1–F5 and F7 are resolved and re-verified against the same head twice now; F4's residual and F6 are both pre-agreed non-blocking follow-ups, not new blockers. I already moved to a formal APPROVE on 09-15 and stand by it. `mergeStateStatus` is `UNSTABLE` with all visible checks green (`Build`, `labeler`, `Notify test workflow`, `Label PRs when reviewed` all SUCCESS as of the 09-20 head check) — that reads like a branch-protection/review-count gate, not a code or CI blocker, so that part is for a write-capable maintainer to clear. Please do open the F4 follow-up issue for the omitted-marker case when convenient. -- 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]
