SEZ9 commented on PR #12081: URL: https://github.com/apache/seatunnel/pull/12081#issuecomment-6116588927
@Rangsh thanks for the detailed recap against `ab04db33a`. The approach you describe for each point sounds right to me: - F1: keeping untimed `get()` as the plain `Future` contract, with the unbounded behaviour documented on the method, is fine given no production caller uses it. - F3/F7: declaring `TimeoutException` on `get(long, TimeUnit)` itself is what I was after. - F2: moving the cache lookup inside the try with `catch (Exception)` in `26ce5cb26` should close the worker-death path from the timeout+remove race. - F6: a single shared `deadlineNanos` across the batch is the right fix. - F8: one WARN line per timeout with the stack at DEBUG, and ERROR only for the unexpected `catch (Exception)` branch, is the split I wanted. - F5: understood that `mockito-junit-jupiter` landed in `26ce5cb26` with the comment updated in `f08bdf0ac`, so it won't show in the `ab04db33a`-only diff. I'll confirm against the full PR diff. I haven't yet verified these against the changed files, so I'll go through the diff for each before marking them resolved. - F4: the sticky `appendBlockedAfterWriteFailure` flag in `1de6f1b88` matches the fail-closed behaviour I was looking for. Could you point me to (or add) a test that drives one failing APPEND and asserts a subsequent APPEND fails without invoking the writer, so this is pinned? Once I've checked the diff and the F4 test is in place, I'll update here. <!-- streview-comment:1685 --> -- 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]
