Closed teawater closed 4 years ago
LGTM
LGTM
LGTM
LGTM
LGTM
This is awesome. Thanks for the contribution!
LGTM
LGTM
LGTM
@mlaventure @stevvooe @dchen1107 @yujuhong PTAL
This mostly looks good. For now, we should make it clear that the Go implementation is the primary one if there are protocol incompatibilities, since we don’t have a wire spec.
There are a few things that need to be fixed:
Glad to see another language implementation!
One more thing I noticed: the package names in the example are all “grpc”. See https://github.com/alipay/ttrpc-rust/blob/master/example/protocols/protos/oci.proto#L9. The package names should be more descriptive about what is in the package. These might be from the upstream project, so you’ll have to work that out there.
Hi @stevvooe ,
Thanks for your review.
This mostly looks good. For now, we should make it clear that the Go implementation is the primary one if there are protocol incompatibilities, since we don’t have a wire spec.
There are a few things that need to be fixed:
- https://github.com/alipay/ttrpc-rust/blob/master/src/ttrpc.proto#L26 Should not copy paste the status and code definitions. These need to be pulled from their upstreams. Weird things can happen with the metadata if we don’t because the package names don’t match.
- https://github.com/alipay/ttrpc-rust/blob/master/src/ttrpc.proto needs to reference the request type from the go project or we need to make a proto file for both projects to draw from. A small note referencing the go project is sufficient for now.
What I think is make another "https://github.com/alipay/ttrpc-rust/blob/master/example/protocols/hack/update-generated-proto.sh" script to convert ttrpc.proto to ttrpc.rs. Are you OK with it?
- The example in https://github.com/alipay/ttrpc-rust/blob/master/example/protocols/protos/oci.proto#L19 needs to follow protobuf naming conventions, which means snakecase. Use the json_name field option to get the json output to match the oci spec.
Glad to see another language implementation!
One more thing I noticed: the package names in the example are all “grpc”. See https://github.com/alipay/ttrpc-rust/blob/master/example/protocols/protos/oci.proto#L9. The package names should be more descriptive about what is in the package. These might be from the upstream project, so you’ll have to work that out there.
All proto file inside "example/protocols/protos" is fetched by https://github.com/alipay/ttrpc-rust/blob/master/example/protocols/hack/update-generated-proto.sh
get_source_version "github.com/kata-containers/agent" ""
cp $GOPATH/src/github.com/kata-containers/agent/protocols/grpc/agent.proto ./protos/
cp $GOPATH/src/github.com/kata-containers/agent/protocols/grpc/oci.proto ./protos/
cp $GOPATH/src/github.com/kata-containers/agent/protocols/grpc/health.proto ./protos/
mkdir -p ./protos/github.com/kata-containers/agent/pkg/types/
cp $GOPATH/src/github.com/kata-containers/agent/pkg/types/types.proto ./protos/github.com/kata-containers/agent/pkg/types/
# The version is get from https://github.com/kata-containers/agent/blob/master/Gopkg.toml
get_source_version "github.com/gogo/protobuf" "4cbf7e384e768b4e01799441fdf2a706a5635ae7"
mkdir -p ./protos/github.com/gogo/protobuf/gogoproto/
cp $GOPATH/src/github.com/gogo/protobuf/gogoproto/gogo.proto ./protos/github.com/gogo/protobuf/gogoproto/
mkdir -p ./protos/google/protobuf/
cp $GOPATH/src/github.com/gogo/protobuf/protobuf/google/protobuf/empty.proto ./protos/google/protobuf/
@stevvooe ping?
We have the minimum required votes to add this to the containerd organization. I believe @stevvooe's comments are potentially just issues (and PRs if agreed to changes are required) in the normal flow of the project. I don't see anything as a red flag for the proposal to add the project.
Any other comments from @containerd/containerd-maintainers or are we ready to bring the project into the organization?
@mlaventure : I saw you clicked the checkbox, but could say LGTM explicitly here for the recording purpose?
@mlaventure : I saw you clicked the checkbox, but could say LGTM explicitly here for the recording purpose?
my bad, I thought I clicked the comment button.
LGTM!
Looks ready. Feel free to initiate transfer or reach out to me @teawater
@dmcgowan Could you help me with initiate transfer?
Thanks @teawater
Transfer is complete https://github.com/containerd/ttrpc-rust
Anymore comments or issues, file them over there :)
The current containerd/ttrpc project is designed to be the GRPC for low-memory environments. The ttrpc is implemented with Golang and its serialization mechanism is protobuf, which is cross-language. As a result it is reasonable to have ttrpc implementation in other language. In the new guest agent of kata-containers project, we implemented a rust version of the agent and are planning to replace the RPC protocol with ttrpc. The initial version of ttrpc-rust has been put at alipay/ttrpc-rust and been tested in kata environment. We think containerd is the best place to host ttrpc-rust because here is the upstream of ttrpc, and this have discussed in #33.
@teawater and @lifupan have prepared the alipay/ttrpc-rust repository for migration to the containerd organization.
This is specifically proposed as a non-core project per the recent project modes we added to containerd's governance. The maintainers of the repository will be Hui Zhu and Fupan Li. According to the non-core document requirement, we will add the boilerplate to the project ( alipay/ttrpc-rust#4 ).
9 maintainer's LGTM required (2/3)