SEZ9 commented on PR #12161:
URL: https://github.com/apache/seatunnel/pull/12161#issuecomment-5754881293
Thanks for the rework in 93e11ea7f0a1. Based on the summary earlier in this
thread, F1, F2, F3, F5 and F7 look addressed (dispatch-first fan-out against a
shared deadline, configurable timeout with an explicit `error` marker instead
of `{}`, REST docs paragraph, duplicate import removed, `cancel(false)` with a
comment). I'll confirm those against the diff rather than the description
before merging.
On F4, the interrupt handling change is the right fix. One non-blocking
residual: members not yet processed when the interrupt hits are omitted from
the response array rather than getting their own marker entry
(`testInterruptStopsWaitingForRemainingMembers` asserts 0 entries for 2
members). Please open a follow-up issue so we don't lose it.
One open question before merge, F6: does the locale test in
`SeaTunnelHealthMonitorTest` now also exercise the `renderLoad` /
`renderOperationService` sites rather than only the two static helpers, and
does it restore the JVM default locale after mutating it? Restoring the default
locale after the test is enough for me on the second point.
@nzw921rx thanks for the review. Once F6 is confirmed, I'm happy to merge.
<!-- streview-comment:1208 -->
--
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]
