-
Notifications
You must be signed in to change notification settings - Fork 75
Dataflow Engine Pipeline configuration stabilization #2031
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
|
||
| /// The URN of a node type. | ||
| pub type NodeUrn = Cow<'static, str>; | ||
| pub use node_urn::NodeUrn; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NodeUrn is now a concrete type
| kind: NodeKind, | ||
| } | ||
|
|
||
| impl NodeUrn { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the methods here were present in other files before. I just group them under this new NodeUrn type.
# Conflicts: # rust/otap-dataflow/crates/config/src/pipeline.rs
| OneOf, | ||
| /// Send each message to every destination. | ||
| Broadcast, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
| // The fanout processor expects each destination port to map to a single downstream node. | ||
| // Multiple targets on the same source port would introduce load-balancing semantics. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the fanout processor is a special case --
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not great. Need to find a better approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introducing a WiringContract directly in the node factory. By default, there would be no additional wiring restrictions beyond those already in place for orphan nodes, cycle detection, and similar safeguards. The fanout processor would be able to declare a constraint such as: "pipeline builder, do not accept more than one connection per output".
| nuc.add_output("main_output"); | ||
| nuc.set_default_output("main_output"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, see!
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2031 +/- ##
==========================================
+ Coverage 86.62% 86.63% +0.01%
==========================================
Files 527 528 +1
Lines 168879 169481 +602
==========================================
+ Hits 146283 146823 +540
- Misses 22062 22124 +62
Partials 534 534
🚀 New features to boost your workflow:
|
Change Summary
Sorry in advance, this is a fairly large PR, but it's for a good reason as it aims to stabilize our configuration model, which we discussed during our SIG meetings.
defaultone_ofbetter reflect the underlying implementation (was never really a round robin strategy as the channel receivers were competing together).To do: update the configuration of our continuous benchmarks.
What issue does this PR close?
How are these changes tested?
All unit tests passed
Are there any user-facing changes?
The structure of the configuration files have changed.