rzo1 commented on PR #8593: URL: https://github.com/apache/storm/pull/8593#issuecomment-4702482942
Thanks for the thorough turnaround, @GGraziadei — this is a big improvement, and I appreciate you re-running the benchmarks from scratch. I went back through my previous points and almost everything is resolved: - System components are now excluded from the feedback loop (`!Utils.isSystemId(componentId)` guard) - The stream id is a constant (`Constants.FEEDBACK_STREAM_ID`) instead of a loosely-validated config, which removes both the null-default crash and the stream-overwrite footgun - `EwmaFeedbackRecord` is appended at the end of the Kryo list, so existing registration ids don't shift - Routing is now power-of-two-choices with a `LoadAwareShuffleGrouping` fallback, and the "averaged" doc nit is fixed - The two stray imports are gone, the perf claim is out of the example, and the usage/file-open handling is much nicer - The security note on netty auth/TLS is a good addition ### One thing still open The feature still silently depends on `topology.stats.ewma.enable`, which defaults to `false`. The jitter gauges are only populated when `ewmaEnable` is true (`TaskMetrics` complete/process/execute jitter), so with feedback enabled but EWMA left at its default, every `EwmaFeedbackRecord` is VOID and the grouping just falls back to load-aware forever — no error, no log. Your benchmark topology sets both, which is why it doesn't show up in testing. Could we either fail config validation when `topology.upstream.feedback.enable=true` and `topology.stats.ewma.enable=false`, or at minimum log a warning at executor startup? I'd lean toward failing validation since the feature is a no-op without it. ### Benchmarks These now answer my earlier concerns — thank you: - The operating point is below the message timeout (acks ≈ transferred now, instead of ~3%), so we're no longer measuring the timeout cliff. - The baseline is `LoadAwareShuffleGrouping`, which is the right comparison. - The earlier throughput decay across windows is gone, which lines up with P2C replacing the winner-take-all behavior. - The avg/max complete-latency columns are now distinct (max is a proper running max). - The overhead run on `FileReadWordCountTopo` lands entirely within the baseline's own run-to-run spread, which supports "overhead is negligible." My honest read of the result: at `max.spout.pending=500` JitterAware gives a consistent ~5–7% complete-latency reduction over load-aware (with and without the injected network jitter), at comparable throughput. At `max.spout.pending=1000` that advantage disappears — JitterAware is roughly equal-to-slightly-worse there. With 2–3 reps and no variance reported, the ~5% delta is close to the rep-to-rep spread, so I'd describe this as "neutral overhead, directionally better latency at moderate load" rather than a throughput win. That's still a reasonable case for the feature as an opt-in, I just want the PR description / docs to frame it that way rather than as a general improvement. Happy to approve once the EWMA-enable dependency is handled (validation or warning). Nice work iterating on this. -- 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]
