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

Batch create and accept stream #2754

Merged
merged 4 commits into from
Oct 9, 2024
Merged

Batch create and accept stream #2754

merged 4 commits into from
Oct 9, 2024

Conversation

jenrryyou
Copy link
Contributor

What problem does this PR solve?

Issue Number: #2733

Problem Summary: 支持一次RPC调用创建多个流

Side effects:

  • Performance effects(性能影响):
    对批量创建stream场景有性能提升

  • Breaking backward compatibility(向后兼容性):

协议改动兼容，支持平滑升级

Check List:

  • Please make sure your changes are compilable(请确保你的更改可以通过编译).
  • When providing us with a new feature, it is best to add related tests(如果你向我们增加一个新的功能, 请添加相关测试).
  • Please follow Contributor Covenant Code of Conduct.(请遵循贡献者准则).

@jenrryyou
Copy link
Contributor Author

jenrryyou commented Sep 3, 2024 •
edited
Loading

看了下macos protobuf22 CI 编译的报错
image
，是因为链接的库libprotobuf.dylib没有
google::protobuf::internal::ThreadSafeArena::thread_cache_定义，但是include 的thread_safe_arena.h文件有这个符号。说明brew编译的libprotobuf.dylib macos版本是开启了PROTOBUF_NO_THREADLOCAL宏（所以有google::protobuf::internal::ThreadSafeArena::thread_cache()定义，但是没有google::protobuf::internal::ThreadSafeArena::thread_cache_定义），brpc编译时候是没有PROTOBUF_NO_THREADLOCAL宏，所以链接时候报错。 附图：image
image

但是也没有在编译时办法传入PROTOBUF_NO_THREADLOCAL宏，protobuf一个头文件做了检查，外部配置PROTOBUF_NO_THREADLOCAL宏就会报错
image

继续分析了下应该是protobuf的BUG，已经在MR protocolbuffers/protobuf#12983 修复, 但是[email protected]版本没有包含该fix，导致在mac上只要在proto文件里使用repeated 类型的Field就会编译出错

wwbmmm
wwbmmm reviewed Sep 4, 2024
View reviewed changes
src/brpc/policy/baidu_rpc_protocol.cpp Outdated Show resolved Hide resolved
src/brpc/stream_impl.h Outdated Show resolved Hide resolved
@wwbmmm
Copy link
Contributor

wwbmmm commented Sep 6, 2024

LGTM

@jenrryyou
Copy link
Contributor Author

LGTM

有空帮忙点下merge，多谢

chenBright
chenBright reviewed Sep 9, 2024
View reviewed changes
src/brpc/policy/baidu_rpc_protocol.cpp Outdated Show resolved Hide resolved
src/brpc/controller.cpp Show resolved Hide resolved
src/brpc/policy/baidu_rpc_protocol.cpp Outdated Show resolved Hide resolved
src/brpc/stream.cpp Show resolved Hide resolved
src/brpc/stream.cpp Outdated Show resolved Hide resolved
@chenBright chenBright linked an issue Sep 13, 2024 that may be closed by this pull request
baidu_rpc协议能否一次RPC调用创建多个Stream #2733
Closed
@jenrryyou
modify code based on the code review feedback and add more tests
58c392a 
modify code based on the code review feedback  and add more tests
chenBright
chenBright approved these changes Oct 8, 2024
View reviewed changes
Copy link
Contributor

@chenBright chenBright left a comment

Choose a reason for hiding this comment

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

LGTM

@jenrryyou
Copy link
Contributor Author

@chenBright @wwbmmm 大佬们有空帮忙点下merge

@chenBright chenBright merged commit 7e5ec4f into apache:master Oct 9, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wwbmmm wwbmmm wwbmmm left review comments

@chenBright chenBright chenBright approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

baidu_rpc协议能否一次RPC调用创建多个Stream
3 participants
@jenrryyou @wwbmmm @chenBright