Copilot commented on code in PR #3189:
URL: https://github.com/apache/brpc/pull/3189#discussion_r2670682065
##########
src/brpc/selective_channel.cpp:
##########
@@ -340,7 +339,7 @@ int Sender::IssueRPC(int64_t start_realtime_us) {
// Forward request attachment to the subcall
sub_cntl->request_attachment().append(_main_cntl->request_attachment());
sub_cntl->http_request() = _main_cntl->http_request();
-
+
Review Comment:
This line contains trailing whitespace at the end. Consider removing the
trailing spaces to maintain code cleanliness.
```suggestion
```
##########
src/brpc/selective_channel.h:
##########
@@ -67,9 +70,10 @@ class SelectiveChannel : public ChannelBase/*non-copyable*/ {
// NOTE: Different from pchan, schan can add channels at any time.
// Returns 0 on success, -1 otherwise.
int AddChannel(ChannelBase* sub_channel, ChannelHandle* handle);
+ int AddChannel(ChannelBase* sub_channel, const std::string& tag,
ChannelHandle* handle);
// Remove and destroy the sub_channel associated with `handle'.
- void RemoveAndDestroyChannel(ChannelHandle handle);
+ void RemoveAndDestroyChannel(const ChannelHandle& handle);
Review 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.
##########
src/brpc/selective_channel.h:
##########
@@ -51,7 +51,10 @@ namespace brpc {
// in `done'.
class SelectiveChannel : public ChannelBase/*non-copyable*/ {
public:
- typedef SocketId ChannelHandle;
+ struct ChannelHandle {
+ SocketId id;
+ std::string tag;
Review 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.
```suggestion
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;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
