jnioche commented on PR #9175: URL: https://github.com/apache/storm/pull/9175#issuecomment-6360348635
> > Thanks @ErnestasVaiciukeviciusAdform and reviewers. Looks good! > > **Suggestions** > > > > 1. **Share the upstream-task logic.** `WorkerState.computeOutboundTasks` and `Executor.computeUpstreamTaskIds` now repeat the same source filtering. If one changes without the other, the routes and the feedback targets drift apart and the warnings come back. A shared static helper would prevent that. > > 2. **Add a test for the sink-only case.** Assert `hasRemoteOutboundTasks()` is true for a worker hosting only a sink bolt, with feedback on and acking off. That's the case with real impact, and nothing tests it today. > > 1. Extracted shared logic into new `StormCommon.upstreamFeedbackSourceComponents` and added tests for it. > Technically it also added check if target componentId is system one that was not present in `Executor.computeUpstreamTaskIds`. > But that should be fine as system components do not send feedback tuples anyway, don't they ? > 2. Added test cases for sink-only/no-ackers. Thanks @ErnestasVaiciukeviciusAdform, both points addressed nicely. On your question: yes, it's fine — BoltExecutor only schedules the feedback tick for non-system components (!Utils.isSystemId(componentId)), and the tick is delivered only to the executor that scheduled it, so system executors never send feedback and the extra guard is behaviour-neutral. LGTM. -- 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]
