Skip to content
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

Move stream management to transport module #12010

Closed
Tracked by #11231
deepthidevaki opened this issue Mar 14, 2023 · 1 comment · Fixed by #12043
Closed
Tracked by #11231

Move stream management to transport module #12010

deepthidevaki opened this issue Mar 14, 2023 · 1 comment · Fixed by #12043
Assignees
Labels
component/broker component/transport kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc. version:8.2.0 Marks an issue as being completely or in parts released in 8.2.0

Comments

@deepthidevaki
Copy link
Contributor

Description

In #11707, we added server side stream management api and implementation in the broker.

Alternate is to add streams as a feature provided by the transport. Advantages of doing this is we can implement both server(broker) and client (gateway) side in one location. This will help us to unit test the feature without fully integrating it with the broker. On the broker side, we will only implement job specific implementation of the stream.

The exact interface between transport and broker will be defined while working on this issue. We have already made the implementation related to stream-management job-agnostic to minimize the dependency between engine and platform.

@deepthidevaki deepthidevaki added the kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc. label Mar 14, 2023
@deepthidevaki deepthidevaki self-assigned this Mar 14, 2023
ghost pushed a commit that referenced this issue Mar 16, 2023
12043: Move stream management to transport r=deepthidevaki a=deepthidevaki

## Description

- Moved remote stream mangement api and its implementation from Broker to transport module. The goal is that the implementations for gateway side stream will be also added to transport. Having both client and server in the same module will help us to test the feature without fully integrating with the broker and gateway.
- Moved all sbe message definitions related to stream from protocol to transport. Since all consumers of these messages will be implemented in the transport module, there is no need to define it in the protocol which is a semi-public module.
- Moved message topic names to transport. To keep it simple, we assume there is only one type of stream (i.e all streams are job streams) will be created. So we don't support different topic names as of now.

While doing the refactoring, we noticed following potential refactoring. They are NOT included in this PR. They will be done in a separate PR. 
- Flatten `GatewayStreamer<M,P>` defined in stream-platform engine api to make it less generic. Eg:- JobGatewayStream without type parameters.
- Move this interface and related ones to the engine instead of stream-platform.
- Inject `GatewayStreamer` to engine via TypedRecordFactory, instead via stream-platform to remove the knowledge of "job activation" from the platform.
- Make `RemoteStreamMetrics` an interface so that metrics can be job specific and injected from the broker.

## Related issues

related #12010



Co-authored-by: Deepthi Devaki Akkoorath <[email protected]>
@npepinpe
Copy link
Member

Done with #12043

@npepinpe npepinpe added the version:8.2.0 Marks an issue as being completely or in parts released in 8.2.0 label Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/broker component/transport kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc. version:8.2.0 Marks an issue as being completely or in parts released in 8.2.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants