rzo1 commented on issue #8583: URL: https://github.com/apache/storm/issues/8583#issuecomment-4421079660
Thanks @GGraziadei — happy to dig in. Three answers, mirroring your numbering: **1. Specialized feedback tuples vs. another transport.** A dedicated stream with direct task-addressed delivery is the right shape — it's the same idiom Storm uses for system tuples (ack, metrics, ticks), it survives the worker-to-worker transport, and unanchored delivery keeps it out of the acker tree. So conceptually, yes, that's an acceptable mechanism. A few constraints I'd want pinned down before it lands: - If feedback only fires on the acked-emit path, you lose the signal exactly where backpressure pressure is most acute: non-reliable topologies, sink bolts that don't re-emit, and any tuple that fails. Predictive backpressure that doesn't cover fire-and-forget pipelines is a hard sell. I'd push for emission to be decoupled from acking — e.g. a periodic tick on the receive side, or every N processed tuples — so spouts, sinks, and unreliable flows all participate. - Just to head this off: `__metrics` is shuffle-grouped into `IMetricsConsumer` bolts on a seconds-scale cadence with retain caps and filtering. None of that fits a sub-second, direct-addressed control signal, and changing it would break every existing metrics consumer. A separate stream is correct. - At per-anchor frequency, the wire/GC cost matters. A narrow dedicated record (e.g. `(srcTaskId, processJitter, executeJitter, completeJitter)` as primitives) will be far cheaper than reusing the v2 metrics DataPoint shape, and makes the contract of this stream explicit rather than entangled with the metrics pipeline. - A fixed sampling ratio is noisy at low rates and expensive at high rates. Either a token-bucket rate cap or an adaptive ratio tied to throughput would be more honest. Either way, please quantify the per-tuple overhead at, say, 100k tuples/s on a representative topology before promoting from draft. - This stream consumes queue capacity and shows up in `__transfer-count`. Make sure it's surfaced cleanly in the UI / metrics so operators can see what fraction of executor budget the control loop is eating. **2. Reorder buffer for proactive backpressure.** This is the part I'm most skeptical about, mainly because the benefit-to-complexity ratio isn't yet demonstrated: - A small reorder window (single-digit elements) on the receive queue is only meaningful when an executor hosts multiple downstream-relevant tasks and each has well-populated jitter stats. In the very common single-task-per-executor case the comparator is effectively a no-op, so the gain depends entirely on topology shape — worth stating explicitly. - Reordering inside one executor's receive queue is a local heuristic. True predictive backpressure usually wants the signal to flow further upstream — i.e. the producer should slow down or pick a different downstream task, not the consumer reshuffle a small window of already-arrived tuples. Have you considered using the jitter signal at emit-time in the producer's grouping/load-balancing decision instead of consumer-side reorder? That would be more potent and avoid head-of-line issues. - The reorder window size should be configurable, and the comparator's invariants (what task IDs it operates on, what happens when stats are missing, what stable ordering it guarantees) need to be spelled out — that's the kind of thing that tends to silently break stream ordering guarantees users rely on. - Tuples within a stream from a single producer to a single task currently arrive in emit order. Reordering can change that. If a topology relies on intra-stream order (very common in stateful bolts), this is a correctness change, not a performance tweak. It must be opt-in, documented as such, and ideally only kick in when the per-tuple destination differs. **3. Validation results.** Encouraging trend, but I don't think the simulator is enough evidence on its own: - A discrete-event simulator validates the control logic, not the cost of carrying the control signal through Storm's actual transport, batching, serialization, and JCQueue dynamics. Before this is "ready," I'd want numbers from a real Storm benchmark — `storm-perf` or a representative topology — showing the same direction. - The metric being optimized (RFC 1889 inter-arrival jitter) is not what most operators actually tune for. They tune tail latency (P99/P999). It's entirely possible to reduce inter-arrival deviation while worsening P99 — smoothing by penalizing outliers. Please include tail-latency alongside the jitter delta. If P99 also improves, the case is strong; if P99 regresses, we should know. - ~1.8% throughput / +2.5% latency is a reasonable cost for a real win, but those numbers will move once acking, network transport, and realistic batch sizes are in the loop. Worth re-measuring under those conditions, and probably worth a load test with feedback enabled but predictive reorder disabled, to isolate the cost of the control signal itself from the cost of the heuristic. In short: I'm supportive of (1) with the caveats above, cautious about (2) and would want to see emit-side use of the signal explored first, and want real Storm benchmarks plus tail-latency numbers before (3) is conclusive. Everything that does land should be opt-in with sane defaults that disable it, given that this touches hot paths and ordering semantics. -- 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]
