-
-
Notifications
You must be signed in to change notification settings - Fork 0
Generalize ReduceDelegate #120
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
Conversation
b0c89f5 to
be45209
Compare
0e26379 to
b10c52e
Compare
| - The Arroyo Reduce strategy is an Arroyo strategy, so it needs to be | ||
| fed with Arroyo messages, thus the adaptation logic from the | ||
| new Streaming platform message that the Rust code deals with. |
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.
Is the idea here, to make it simpler to adapt existing consumers built as arroyo tasks into the streaming platform? That seems like a great way to make the conversion process simpler.
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.
Yes, it can be used for that as well.
Here the goal was to adapt Arroyo strategies, but a consumer in the end is a chain of strategies so that works as well.
| TStrategyOut = TypeVar("TStrategyOut") | ||
|
|
||
|
|
||
| class OutputRetriever(ProcessingStrategy[TStrategyOut], Generic[TStrategyOut]): |
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.
In a nutshell, how is this OutputRetriever different from the old one?
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.
Actually, I think I got it
| make them available to rust. | ||
| The message flow looks like this: | ||
| 1. The Rust Arroyo Strategy receives a streaming message to process. |
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.
Thanks for outlining the steps here. It is much clearer now
ayirr7
left a comment
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 overall looks good to me. I'll wait for any others' who might have feedback before I approve
Based on #119
While buildign a RustOperatorDelegate for the RunTaskInMultiProcess
I noticed that the delegating logic was very similar to the Reduce
step so I make the
OutputRetrieverand theReduceDelegatereusable.This PR:
functions passed to the two classes
Message, but that is a type Rust cannot understand.