SEZ9 commented on PR #11564: URL: https://github.com/apache/seatunnel/pull/11564#issuecomment-6379897794
Thanks for keeping this up to date with `dev`. As noted on the thread, the only change between `e01712f9` and `d71ee0a433fb` is the merge commit, and `ReportMetricsOperation.java` / `ReportMetricsOperationTest.java` are identical between the two, so the previously raised points are still open. Here is what remains, in priority order: 1. **Executor sizing / backlog (PR11564-F1, MEDIUM).** `Offload.start()` queues `run()` onto Hazelcast's shared `OFFLOADABLE_EXECUTOR`, which is core-sized with an unbounded queue and is also used by Hazelcast's own offloaded map operations. Under the heavy-reporting load this PR targets, callers can time out and re-send while the backlog keeps growing. Please either use a dedicated, bounded executor for metrics updates, or explain in the PR why the shared pool is acceptable here. 2. **Javadoc on the `call()` override (PR11564-F2, MEDIUM).** This is the first `Offload` operation in Zeta, and the current one-line comment is misleading. Please replace it with a Javadoc that states the actual contract: the generic-operation thread returns immediately, `run()` executes on the offload executor, and the response (success or failure) is sent manually from that thread. 3. **Success-path `sendResponse(null)` failure (PR11564-F3, LOW).** If `sendResponse(null)` throws inside the offloaded runnable, nothing is logged through the operation, no failure response is attempted, and the caller waits for the invocation timeout. Please at least log via the operation's logger so this is diagnosable. 4. **`catch (Throwable t)` swallowing `Error`s (PR11564-F4, LOW).** `OutOfMemoryError` and other `Error`s are currently routed into the normal failure-response path, bypassing Hazelcast's `OutOfMemoryErrorDispatcher` hook that the generic-operation runner applied before this change. Please narrow the catch or explicitly inspect/rethrow `OutOfMemoryError`. 5. **Reflection on `Offload.executionService` in the test (PR11564-F5, LOW).** `shouldAttemptSuccessResponseOnlyOnceWhenResponseDeliveryFails` reaches into a Hazelcast internal field by name, which is brittle across Hazelcast upgrades. If a dedicated executor is introduced for item 1, injecting it would let this test avoid reflection. Items 1 and 2 are what I'd like resolved before approval; 3–5 are small and could go into the same push. If you disagree with any of these, a short note on the thread is fine and we can discuss. <!-- streview-comment:2000 --> -- 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]
