SEZ9 commented on PR #12161:
URL: https://github.com/apache/seatunnel/pull/12161#issuecomment-5674157529
Thanks for the follow-up work here. Summarizing where I see things against
my earlier review:
- **F1 / F2 / F4 (timeout drop as `{}`, serial per-member timeout, interrupt
handling)** – the automated review follow-up reports these as resolved in the
latest commit. Before I merge, could you briefly confirm in a comment how each
was addressed (in particular: is the timeout now configurable or at least
surfaced distinctly from an empty node, are the member invocations fired first
and then awaited against a shared deadline, and does the loop now stop/log on
`InterruptedException` instead of returning a silent partial 200)?
- **F3 (docs)** – I don't see any mention of the REST API docs (en/zh,
v1/v2) being updated for the new `/system-monitoring-information` timeout
behaviour. Please confirm whether that was done or add it.
- **F5 (duplicate `TimeUnit` import / Spotless)** – please confirm the
duplicate import is gone and the test source has been formatted so the Spotless
check does not block.
- **F7 (`invocationFuture.cancel(true)`)** – low severity; if it's still in
place, either drop the misleading `true` flag or add a short comment noting it
doesn't cancel the remote operation.
- **F6 (locale test coverage / global default locale mutation)** – fine to
leave as non-blocking; a follow-up issue would be appreciated.
Once you confirm F1/F2/F4 and address F3/F5, I'm happy to take this through
the final merge.
<!-- streview-comment:1062 -->
--
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]
