SEZ9 commented on PR #12081: URL: https://github.com/apache/seatunnel/pull/12081#issuecomment-6138045063
@Rangsh thanks for the detailed recap against `ab04db33a`; that is a clear map of where each point is meant to be addressed. I will confirm each item against the actual changes rather than closing on the description alone: - F1/F3/F7: the untimed `get()` staying unbounded per the `Future` contract, with the `TimeoutException` throws-clause documented on `get(long, TimeUnit)` itself, sounds right; I will check the method-level Javadoc. - F2: I will confirm in `26ce5cb26` that the cache lookup sits inside the try with the `Exception` catch. - F4: I will confirm the sticky `appendBlockedAfterWriteFailure` fail-closed path in `1de6f1b88`, and check the existing test coverage for the writer being invoked only once after a failure before deciding whether any new test is needed. Fine to keep the `Error`-from-`writer.write()` case tracked separately. - F6: a single shared `deadlineNanos` for the batch is the right fix; I will confirm. - F8: WARN with requestId/elapsed/limit plus stack at DEBUG, with ERROR only on the unexpected `catch (Exception)` branch, is the split I was after; I will confirm. - F5: understood that `mockito-junit-jupiter` landed test-scoped in `26ce5cb26` and only had its comment updated in `f08bdf0ac`, so it does not appear in the `ab04db33a` diff. I will check the full PR diff; no action needed from you. No further changes requested from my side at this point. I will follow up with the review outcome once I have gone through the full diff. <!-- streview-comment:1726 --> -- 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]
