surpport tag for selective channel#3189
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds tag support for SelectiveChannel to enable load balancing plugins to use tags when selecting channels, and fixes a potential null pointer dereference in backup request scenarios.
Key Changes:
- Modified
ChannelHandlefrom a simple typedef to a struct containing both socket ID and tag - Added tag parameter support to
AddChannelmethods - Fixed potential crash when
need_feedbackis true but_lbis null
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/brpc/selective_channel.h | Changed ChannelHandle from typedef to struct with id and tag fields; updated method signatures |
| src/brpc/selective_channel.cpp | Implemented tag support throughout channel balancer; added overloaded AddChannel method; removed debug log statement |
| src/brpc/controller.cpp | Added null check for _lb pointer before calling Feedback to prevent crash |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sub_cntl->request_attachment().append(_main_cntl->request_attachment()); | ||
| sub_cntl->http_request() = _main_cntl->http_request(); | ||
|
|
||
There was a problem hiding this comment.
This line contains trailing whitespace at the end. Consider removing the trailing spaces to maintain code cleanliness.
| typedef SocketId ChannelHandle; | ||
| struct ChannelHandle { | ||
| SocketId id; | ||
| std::string tag; |
There was a problem hiding this comment.
This is a breaking API change. The ChannelHandle type is changed from a simple typedef (SocketId) to a struct containing both id and tag. This breaks backward compatibility for existing code that treats ChannelHandle as a SocketId value. Users who pass ChannelHandle by value or compare it directly with SocketId will experience compilation errors. Consider whether a deprecation period or alternative approach would be more appropriate for this public API change.
| std::string tag; | |
| std::string tag; | |
| // Backward-compatible constructors and conversion operators so that | |
| // ChannelHandle can still be used where a SocketId was expected. | |
| ChannelHandle() | |
| : id(INVALID_SOCKET_ID) {} | |
| ChannelHandle(SocketId sid) | |
| : id(sid) {} | |
| ChannelHandle(SocketId sid, const std::string& t) | |
| : id(sid), tag(t) {} | |
| // Implicit conversion to SocketId for existing code that treats | |
| // ChannelHandle as a SocketId value. | |
| operator SocketId() const { return id; } | |
| // Assignment from SocketId for source compatibility. | |
| ChannelHandle& operator=(SocketId sid) { | |
| id = sid; | |
| tag.clear(); | |
| return *this; | |
| } |
|
|
||
| // Remove and destroy the sub_channel associated with `handle'. | ||
| void RemoveAndDestroyChannel(ChannelHandle handle); | ||
| void RemoveAndDestroyChannel(const ChannelHandle& handle); |
There was a problem hiding this comment.
The signature change from passing ChannelHandle by value to passing by const reference is a breaking API change. Existing code that calls RemoveAndDestroyChannel with a SocketId (which was the old ChannelHandle typedef) will fail to compile. Since ChannelHandle is now a struct, consider providing an overload that accepts just a SocketId for backward compatibility, or ensure all users are aware of this breaking change.
|
Can you explain in what scenarios does a selective channel need a tag ? |
I need to implement my own load balancer plugin, which requires passing parameters via load balancer tags. Similarly, if using built-in load balancer plugins like WRR, I also need to pass parameters via tags. |
|
LGTM |
1 similar comment
|
LGTM |
When using the selective channel, I encountered a coredump issue. In the case of a selective channel timeout, the main_cntl rest the lb but does not destroy the call_id. When the sub channel's RPC ends, it can still lock the call_id and call lb's feedback, causing a core dump. I simply added a check for the validity of lb in the feedback function, but I'm not sure if you have better suggestions to resolve this issue. |
I think this will work. Furthermore, Is it possible that if sub_done finds that main_cntl is Failed with the error code ERPCTIMEDOUT in SubDone::Run, it can skip executing OnVersionedRPCReturned function and return directly？ |
If main_cntl times out and SubDone::Run returns directly without calling OnVersionedRPCReturned, it will prevent Sender::Run from being invoked to clear resource. However, if OnVersionedRPCReturned is called, the issue of lb being set to null still persists. |
What problem does this PR solve?
Issue Number: resolve
Problem Summary: To support tags for SelectiveChannel due to the requirements of the loadbalance plugin.
What is changed and the side effects?
Changed:
Side effects:
1.SelectiveChannel 支持tag传递给load balance插件
2.修复一个SelectiveChannel 开启backup request的时候，可能的core问题
Check List: