-
Notifications
You must be signed in to change notification settings - Fork 12k
[ISSUE #3905] Support bname in protocol #4615
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.
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
Conversation
zhouxinyu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM~
Codecov Report
@@ Coverage Diff @@
## develop #4615 +/- ##
=============================================
- Coverage 43.42% 43.38% -0.04%
+ Complexity 6203 6196 -7
=============================================
Files 817 817
Lines 57654 57675 +21
Branches 7873 7875 +2
=============================================
- Hits 25036 25024 -12
- Misses 29375 29408 +33
Partials 3243 3243
Continue to review full report at Codecov.
|
| requestHeader.setConsumerGroup(this.groupName); | ||
| requestHeader.setQueueId(mq.getQueueId()); | ||
| requestHeader.setCommitOffset(offset); | ||
| requestHeader.setBname(mq.getBrokerName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @drpmma ,
Why not just simply name it "brokerName" instead of "bname" to make it more straightforward? I felt confused for a second after I saw your description haha.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bname is not in the scope of this pr, see b6ff649 for more information.
I guess bname is shorter than brokerName thus it is more efficient in tranport.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drpmma Oh, okay. That's reasonable.
|
Let's merge this PR first, although there is one ci failed, @RongtongJin is trying to fix this problem in another thread. |
Make sure set the target branch to
developWhat is the purpose of the change
Support
bnamein request protocol.Brief changelog
Support
bnamefor request:PULL_MESSAGE,UPDATE_CONSUMER_OFFSET,QUERY_CONSUMER_OFFSET,SEARCH_OFFSET_BY_TIMESTAMP,GET_MIN_OFFSET,GET_MAX_OFFSET.