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

Streaming support. #146

Merged
merged 15 commits into from
Aug 10, 2022
Merged

Streaming support. #146

merged 15 commits into from
Aug 10, 2022

Conversation

wllenyj
Copy link
Collaborator

@wllenyj wllenyj commented Jun 6, 2022

Introduce support for streaming(https://github.com/containerd/ttrpc/blob/main/PROTOCOL.md#streaming). This is already supported by Golang's TTRPC in containerd/ttrpc#107. But currently only async is implemented. Related issues #144.

This pull request changes include:

  1. Improved message encoding and decoding.
  2. Added shutdown module to handle graceful shutdown.
  3. Refactored connection handling.
  4. Added streaming support, and added an example about streaming.

Usually, streaming supports 3 types:

  • Server streaming
  • Client streaming
  • Bidirectional streaming ( or duplex streaming )

These 3 types are the same as GRPC, see https://grpc.io/docs/what-is-grpc/core-concepts/#server-streaming-rpc for details.

A general Bidirectional streaming process is as follows:

image

The Connection is an abstraction of a connection. One connection is handled as one Write async-task and one Read async-task.

  • The Write task receives the message from the channel and writes it to the connection.
  • The Read task receives the message from the connection and writes it to the channel.

@wllenyj wllenyj force-pushed the stream-1 branch 4 times, most recently from a728401 to bda9829 Compare June 6, 2022 17:17
@Tim-Zhang Tim-Zhang requested review from liubin and lifupan June 7, 2022 02:46
@Tim-Zhang
Copy link
Member

Tim-Zhang commented Jun 7, 2022

@wllenyj Awesome! Thanks a ton for introducing this so important feature. @jodh-intel @mxpv @lifupan @liubin Please take a look at this.

@Tim-Zhang Tim-Zhang requested review from jodh-intel and mxpv June 7, 2022 03:13
@wllenyj
Copy link
Collaborator Author

wllenyj commented Jun 7, 2022

@dmcgowan Any time to help with this? Thanks a lot.

@dmcgowan
Copy link
Member

dmcgowan commented Jun 7, 2022

@wllenyj thanks! I'll take a look

src/error.rs Outdated Show resolved Hide resolved
@wllenyj wllenyj force-pushed the stream-1 branch 2 times, most recently from 84081f0 to d216133 Compare June 10, 2022 08:22
Copy link
Collaborator

@quanweiZhou quanweiZhou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @wllenyj - a few comments

src/sync/client.rs Show resolved Hide resolved
src/proto.rs Show resolved Hide resolved
src/proto.rs Show resolved Hide resolved
src/proto.rs Outdated Show resolved Hide resolved
src/proto.rs Show resolved Hide resolved
&self,
req: Request,
streaming_client: bool,
streaming_server: bool,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think streaming_client: bool and streaming_server: bool are a bit confusing, it's more clear to define them as sendable: bool and receivable: bool, while in this case only 3 conditions sender stream, receiver stream, and duplex stream, maybe an enum is better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to keep this commit consistent with the golang version. Optimization will be done later.

src/asynchronous/server.rs Show resolved Hide resolved
src/asynchronous/server.rs Outdated Show resolved Hide resolved
src/asynchronous/server.rs Show resolved Hide resolved
src/asynchronous/server.rs Show resolved Hide resolved
Copy link
Collaborator Author

@wllenyj wllenyj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@quanweiZhou Thanks for your comments.

@codecov
Copy link

codecov bot commented Jun 24, 2022

Codecov Report

Merging #146 (98d128d) into master (8f2bd91) will increase coverage by 14.34%.
The diff coverage is 37.58%.

@@             Coverage Diff             @@
##           master     #146       +/-   ##
===========================================
+ Coverage   11.68%   26.02%   +14.34%     
===========================================
  Files          13       16        +3     
  Lines        1549     2413      +864     
===========================================
+ Hits          181      628      +447     
- Misses       1368     1785      +417     
Impacted Files Coverage Δ
src/asynchronous/client.rs 0.00% <0.00%> (ø)
src/asynchronous/connection.rs 0.00% <0.00%> (ø)
src/asynchronous/server.rs 0.00% <0.00%> (ø)
src/asynchronous/stream.rs 0.00% <0.00%> (ø)
src/asynchronous/utils.rs 0.00% <ø> (ø)
src/common.rs 34.57% <ø> (-14.76%) ⬇️
src/error.rs 55.55% <ø> (+55.55%) ⬆️
src/lib.rs 100.00% <ø> (ø)
src/sync/channel.rs 0.00% <ø> (ø)
src/sync/client.rs 0.00% <0.00%> (ø)
... and 8 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

src/asynchronous/server.rs Outdated Show resolved Hide resolved
src/asynchronous/server.rs Outdated Show resolved Hide resolved
src/proto.rs Outdated Show resolved Hide resolved
src/asynchronous/shutdown.rs Outdated Show resolved Hide resolved
compiler/src/codegen.rs Show resolved Hide resolved
compiler/src/codegen.rs Show resolved Hide resolved
compiler/src/codegen.rs Outdated Show resolved Hide resolved
compiler/src/codegen.rs Outdated Show resolved Hide resolved
compiler/src/codegen.rs Outdated Show resolved Hide resolved
compiler/src/codegen.rs Outdated Show resolved Hide resolved
compiler/src/codegen.rs Show resolved Hide resolved
src/asynchronous/connection.rs Outdated Show resolved Hide resolved
src/asynchronous/client.rs Outdated Show resolved Hide resolved
src/asynchronous/client.rs Outdated Show resolved Hide resolved
Move the MessageHeader from `common.rs` to the `proto.rs` file.
Move the `compiled::ttrpc` from `common.rs` to `proto.rs` file and re-export.

Signed-off-by: wllenyj <[email protected]>
Introduce Codec trait for protobuf message encoding/decoding.

Signed-off-by: wllenyj <[email protected]>
Added more unit test for proto.rs.

Signed-off-by: wllenyj <[email protected]>
It is used for server-side graceful shutdown.

Signed-off-by: wanglei01 <[email protected]>
The first block uses the standard library.
The second block uses the third party crates,
and the third block uses this crate.

Signed-off-by: wllenyj <[email protected]>
Using shutdown can be used instead of channel composition.

Signed-off-by: wllenyj <[email protected]>
Make message encoding/decoding uniform.

Signed-off-by: wllenyj <[email protected]>
For abstract connections, our connections are handled as one sending
task and one receiving task. We can use the same logic to handle it.

Signed-off-by: wllenyj <[email protected]>
The client and server handle connections almost identically. Both
use a sender side and a receiver side to handle the connection.
Their respective differences are implemented using the delegate.

Signed-off-by: wllenyj <[email protected]>
Added protocol support for streaming.
Added `StreamInner` struct for streaming operations and Streaming
related errors.

Signed-off-by: wllenyj <[email protected]>
Added streaming support for client-side.

Signed-off-by: wanglei01 <[email protected]>
Added streaming support for server-side.

Signed-off-by: wanglei01 <[email protected]>
Added streaming support for generator.

Signed-off-by: wanglei01 <[email protected]>
This example is the same as the golang version.
See
`https://github.com/containerd/ttrpc/tree/main/integration/streaming`
for details.

Signed-off-by: wllenyj <[email protected]>
This is required by unicode-ident crate. For details:

```
error[L001]: failed to satisfy license requirements
  ┌─ unicode-ident 1.0.3 (registry+https://github.com/rust-lang/crates.io-index):4:13
  │
4 │ license = "(MIT OR Apache-2.0) AND Unicode-DFS-2016"
  │            -^^^----^^^^^^^^^^------^^^^^^^^^^^^^^^^
  │            ││      │               │
  │            ││      │               rejected: not explicitly allowed
  │            ││      accepted: license is explicitly allowed
  │            │accepted: license is explicitly allowed
  │            license expression retrieved via Cargo.toml `license`
  │
  = unicode-ident v1.0.3
    ├── proc-macro2 v1.0.43
    │   ├── async-trait v0.1.57
    │   │   └── ttrpc v0.6.1
    │   ├── futures-macro v0.3.21
    │   │   └── futures-util v0.3.21
    │   │       ├── futures v0.3.21
    │   │       │   ├── tokio-vsock v0.3.2
    │   │       │   │   └── ttrpc v0.6.1 (*)
    │   │       │   └── ttrpc v0.6.1 (*)
    │   │       └── futures-executor v0.3.21
    │   │           └── futures v0.3.21 (*)
    │   ├── quote v1.0.21
    │   │   ├── async-trait v0.1.57 (*)
    │   │   ├── futures-macro v0.3.21 (*)
    │   │   ├── syn v1.0.99
    │   │   │   ├── async-trait v0.1.57 (*)
    │   │   │   ├── futures-macro v0.3.21 (*)
    │   │   │   ├── thiserror-impl v1.0.32
    │   │   │   │   └── thiserror v1.0.32
    │   │   │   │       └── ttrpc v0.6.1 (*)
    │   │   │   └── tokio-macros v1.8.0
    │   │   │       └── tokio v1.20.1
    │   │   │           ├── tokio-vsock v0.3.2 (*)
    │   │   │           └── ttrpc v0.6.1 (*)
    │   │   ├── thiserror-impl v1.0.32 (*)
    │   │   └── tokio-macros v1.8.0 (*)
    │   ├── syn v1.0.99 (*)
    │   ├── thiserror-impl v1.0.32 (*)
    │   └── tokio-macros v1.8.0 (*)
    └── syn v1.0.99 (*)
```

Signed-off-by: wanglei01 <[email protected]>
Copy link
Collaborator

@quanweiZhou quanweiZhou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @wllenyj, LGTM.

@Tim-Zhang Tim-Zhang merged commit f63abbe into containerd:master Aug 10, 2022
Tim-Zhang added a commit to Tim-Zhang/ttrpc-rust that referenced this pull request Aug 10, 2022
Bump the major version due to pr containerd#146.

Signed-off-by: Tim Zhang <[email protected]>
@Tim-Zhang Tim-Zhang mentioned this pull request Aug 10, 2022
Tim-Zhang added a commit to Tim-Zhang/ttrpc-rust that referenced this pull request Aug 12, 2022
Bump the major version of compiler to 0.6.0 due to containerd#146.

Signed-off-by: Tim Zhang <[email protected]>
Tim-Zhang added a commit to Tim-Zhang/ttrpc-rust that referenced this pull request Aug 12, 2022
Bump the major version of ttrpc-compiler to 0.6.0 due to containerd#146.

Signed-off-by: Tim Zhang <[email protected]>
Tim-Zhang added a commit to Tim-Zhang/ttrpc-rust that referenced this pull request Aug 12, 2022
Bump the major version of ttrpc-compiler to 0.6.0 due to containerd#146.

Signed-off-by: Tim Zhang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants