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.

Sign up for GitHub

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

Jump to bottom

feat: compact protocol implementation #36

Merged
merged 21 commits into from
Dec 22, 2022
Merged

feat: compact protocol implementation #36

merged 21 commits into from
Dec 22, 2022

Conversation

ii64
Copy link
Member

@ii64 ii64 commented Oct 31, 2022 •
edited

Motivation

Support Thrift compact protocol async/RW, although I am still looking for a way to extend the usage of volo to use custom Thrift protocol (it's by default using binary), and transport (it's by default using TCP) but it seem can't be changed at the moment this PR is issued.

Solution

Port Apache Thrift Rust lib to pilota

@PureWhiteWu
Copy link
Member

你好，非常感谢你的贡献！
这个 PR 的变更较大，估计相关的疑问也会比较多，因此可能会需要较长时间 review 和讨论；如果方便的话，你也可以加入飞书群沟通，效率会更高一些。
关于 Volo 扩展 Thrift 实现的需求，我们之前也考虑过，你具体是什么场景呀？我们看看怎么样更好地来支持。

PureWhiteWu
PureWhiteWu reviewed Nov 1, 2022
View reviewed changes
pilota/Cargo.toml Outdated Show resolved Hide resolved
pilota/src/thrift/rw_ext.rs Outdated Show resolved Hide resolved
pilota/src/thrift/rw_ext.rs Outdated Show resolved Hide resolved
@CLAassistant
Copy link

CLAassistant commented Nov 22, 2022 •
edited

CLA assistant check
All committers have signed the CLA.

@PureWhiteWu
Copy link
Member

Hello，Volo 现在已经支持了自定义 Codec 方法啦，并且支持非常灵活的组合，具体可以看下这个 PR：cloudwego/volo#92
简单来说，如果你只是想把底层的 Binary 换成 Compact，那么在 volo_thrift::codec::default::thrift 里面调用一下你在 Pilota 里实现的这个 Protocol 就可以啦～

@ii64
Copy link
Member Author

ii64 commented Nov 22, 2022

好说！谢谢，我会努力的

PureWhiteWu
PureWhiteWu reviewed Nov 30, 2022
View reviewed changes
pilota/src/thrift/compact.rs Outdated Show resolved Hide resolved
pilota/src/thrift/compact.rs Outdated Show resolved Hide resolved
@PureWhiteWu
Copy link
Member

另外，你可以先 rebase 一下你的分支～

@ii64 ii64 force-pushed the feat/compact-protocol branch 3 times, most recently from a66f252 to 33a3891 Compare December 8, 2022 18:24
LYF1999
LYF1999 reviewed Dec 22, 2022
View reviewed changes
Copy link
Member

@LYF1999 LYF1999 left a comment

Choose a reason for hiding this comment

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

应该需要rebase一下

pilota/src/thrift/compact.rs Outdated Show resolved Hide resolved
@PureWhiteWu PureWhiteWu requested review from a team as code owners December 22, 2022 03:35
PureWhiteWu
PureWhiteWu previously approved these changes Dec 22, 2022
View reviewed changes
pilota/src/thrift/compact.rs Outdated Show resolved Hide resolved
@PureWhiteWu PureWhiteWu merged commit fd25042 into cloudwego:main Dec 22, 2022
@PureWhiteWu
Copy link
Member

非常感谢你的贡献！
如果你对 compact 协议有需求，可以去 Volo 里面实现对应的逻辑啦～

@ii64 ii64 deleted the feat/compact-protocol branch January 31, 2023 07:45
@ii64 ii64 mentioned this pull request Feb 7, 2023
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LYF1999 LYF1999 LYF1999 approved these changes

@PureWhiteWu PureWhiteWu PureWhiteWu left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ii64 @PureWhiteWu @CLAassistant @LYF1999