SEZ9 commented on PR #11564: URL: https://github.com/apache/seatunnel/pull/11564#issuecomment-6392877830
Following up on head `d71ee0a433fb`: the five points from the previous review still stand, and there is agreement on the thread that none of them are contradicted. To keep the list in one place, here is what I'd like to see in the next push: **Must-fix before approval** 1. **Executor ownership / backlog (F1).** `ReportMetricsOperation` currently offloads onto Hazelcast's shared, core-sized `OFFLOADABLE_EXECUTOR` with an unbounded queue. Under the heavy-reporting load this PR is meant to handle, the backlog can grow without bound while callers time out and re-send, and it competes with Hazelcast's own offloaded map operations. Please move this to a dedicated, bounded executor owned by the engine (or explain on the thread why the shared executor is acceptable if you disagree — a short note is fine). 2. **Javadoc for the `call()` override (F2).** The single-line comment is misleading about what actually happens. Please replace it with a Javadoc that states the real `Offload` contract: that `call()` returns an `Offload`, that the work and the response are delivered asynchronously from the executor, and why this is needed for metrics reporting. **Can ride in the same push** 3. **Success-path `sendResponse(null)` failure (F3).** If `sendResponse(null)` throws, the exception escapes the runnable without being logged through the operation and without a failure response attempt, so the caller only learns about it via invocation timeout. Please log it through the operation's logger and make sure the failure is surfaced. 4. **`catch (Throwable t)` swallowing `Error`s (F4).** Routing `OutOfMemoryError` and other `Error`s into the normal response path skips Hazelcast's `OutOfMemoryErrorDispatcher` hook that the generic-operation runner applied previously. Please narrow the catch to `Exception` or explicitly rethrow/dispatch `Error`s. 5. **Reflection on `Offload.executionService` in the test (F5).** `shouldAttemptSuccessResponseOnlyOnceWhenResponseDeliveryFails` reaches into Hazelcast's private field by name, which will break on upgrades. Once item 1 lands with an engine-owned executor, the test can inject that executor directly instead. No new commit has appeared since `d71ee0a433fb`, so everything above will be re-checked once you push. Thanks for the work on this. <!-- streview-comment:2018 --> -- 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]
