[dbsp] Gate streaming exchange with a DevTweak.#6307
Conversation
| } else if Runtime::num_workers() == 1 { | ||
| self.dyn_accumulate(factories) | ||
| } else if Runtime::with_dev_tweaks(|d| !d.streaming_exchange()) { | ||
| self.dyn_shard(factories).dyn_accumulate(factories) |
There was a problem hiding this comment.
this is where it takes effect?
mythical-fred
left a comment
There was a problem hiding this comment.
Clean dev-tweak gate; reverts the multihost-sharded-accumulator path to shard().accumulate() by default while keeping the streaming-exchange path reachable for tuning. Tests in sharded_accumulator.rs opt into the new code path explicitly, so the existing coverage stays meaningful.
Two small nits inline; nothing blocking.
| /// Enable streaming exchange. | ||
| /// | ||
| /// `false` | ||
| #[serde(skip_serializing_if = "Option::is_none")] |
There was a problem hiding this comment.
Doc comment /// "false" is a little cryptic — other fields use prose like "The default is false." Worth matching that convention so the rendered openapi help isn't just a backticked literal.
| if config.dev_tweaks.streaming_exchange() { | ||
| info!("dev_tweaks.streaming_exchange enabled"); | ||
| } else { | ||
| info!("dev_tweaks.streaming_exchange disabled"); |
There was a problem hiding this comment.
Logging both branches at info! means every pipeline start emits a dev_tweaks.streaming_exchange disabled line forever, even though that's the default and uninteresting. Consider debug! for the disabled branch, or log only when explicitly set (config.dev_tweaks.streaming_exchange.is_some()). Soft.
Add a dev tweak to disable streaming exchange. This was motivated by a suspected performance regression, which ended up not being a real issue. I think it is still helpful to be able to disable this new feature. Signed-off-by: Leonid Ryzhyk <ryzhyk@gmail.com>
cb4c167 to
497baef
Compare
mythical-fred
left a comment
There was a problem hiding this comment.
New commit 497baef4 flips DevTweaks::streaming_exchange() default from false to true (dev_tweaks.rs:351). That contradicts both the PR title ("Gate streaming exchange with a DevTweak") and the PR description ("puts it behind a streaming_exchange dev tweak (disabled by default)"). The commit message still says "streaming exchange dev tweak (disabled by default)".
If the intent of this PR is in fact to enable streaming exchange by default and gate the legacy path, the description, title, commit message and rustdoc need to reflect that — and the rationale ("streaming exchange may need more tuning") no longer holds. If the intent is still to disable by default, the unwrap_or(true) is a bug.
My prior APPROVE was on cb4c1673 where the default was false. I'm not blocking, but please reconcile before merge.
It looks like the streaming exchange operator may need more tuning. This commit puts it behind a
streaming_exchangedev tweak.Describe Manual Test Plan
Checklist
Breaking Changes?
This change will affect multihost performance.