You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by GitBox <gi...@apache.org> on 2020/12/29 12:41:03 UTC
[GitHub] [incubator-brpc] wasphin opened a new pull request #1320: Fix registering multiple addresses to discovery
wasphin opened a new pull request #1320:
URL: https://github.com/apache/incubator-brpc/pull/1320
See API here at https://github.com/bilibili/discovery/blob/master/doc/api.md#%E6%B3%A8%E5%86%8Cregister
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] wasphin commented on a change in pull request #1320: Fix registering multiple addresses to discovery
Posted by GitBox <gi...@apache.org>.
wasphin commented on a change in pull request #1320:
URL: https://github.com/apache/incubator-brpc/pull/1320#discussion_r553103053
##########
File path: test/brpc_naming_service_unittest.cpp
##########
@@ -579,6 +579,14 @@ class DiscoveryNamingServiceImpl : public test::DiscoveryNamingService {
google::protobuf::Closure* done) {
brpc::ClosureGuard done_guard(done);
brpc::Controller* cntl = static_cast<brpc::Controller*>(cntl_base);
+ auto body = cntl->request_attachment().to_string();
+ for (brpc::QuerySplitter sp(body); sp; ++sp) {
+ if (!sp.key().empty()) {
Review comment:
Thanks for the review, and done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] zyearn commented on a change in pull request #1320: Fix registering multiple addresses to discovery
Posted by GitBox <gi...@apache.org>.
zyearn commented on a change in pull request #1320:
URL: https://github.com/apache/incubator-brpc/pull/1320#discussion_r552973735
##########
File path: test/brpc_naming_service_unittest.cpp
##########
@@ -579,6 +579,14 @@ class DiscoveryNamingServiceImpl : public test::DiscoveryNamingService {
google::protobuf::Closure* done) {
brpc::ClosureGuard done_guard(done);
brpc::Controller* cntl = static_cast<brpc::Controller*>(cntl_base);
+ auto body = cntl->request_attachment().to_string();
+ for (brpc::QuerySplitter sp(body); sp; ++sp) {
+ if (!sp.key().empty()) {
Review comment:
I would suggest this line can be deleted.
##########
File path: src/brpc/policy/discovery_naming_service.cpp
##########
@@ -271,9 +272,15 @@ int DiscoveryClient::DoRegister() {
cntl.http_request().set_content_type("application/x-www-form-urlencoded");
butil::IOBufBuilder os;
os << "appid=" << _params.appid
- << "&hostname=" << _params.hostname
- << "&addrs=" << _params.addrs
- << "&env=" << _params.env
+ << "&hostname=" << _params.hostname;
+
+ std::vector<butil::StringPiece> addrs;
+ butil::SplitString(_params.addrs, ',', &addrs);
+ for (size_t i = 0; i < addrs.size(); ++i) {
+ os << "&addrs=" << addrs[i];
+ }
Review comment:
According to the comment of butil::SplitString: If several instances of |c| are contiguous, or if |str| begins with or ends with |c|, then an empty string is inserted, we should check if addrs[i] is empty string.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] gydong commented on pull request #1320: Fix registering multiple addresses to discovery
Posted by GitBox <gi...@apache.org>.
gydong commented on pull request #1320:
URL: https://github.com/apache/incubator-brpc/pull/1320#issuecomment-752297738
LGTM
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] zyearn commented on pull request #1320: Fix registering multiple addresses to discovery
Posted by GitBox <gi...@apache.org>.
zyearn commented on pull request #1320:
URL: https://github.com/apache/incubator-brpc/pull/1320#issuecomment-756295195
Thanks for the contribution.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] zyearn merged pull request #1320: Fix registering multiple addresses to discovery
Posted by GitBox <gi...@apache.org>.
zyearn merged pull request #1320:
URL: https://github.com/apache/incubator-brpc/pull/1320
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] zyearn commented on pull request #1320: Fix registering multiple addresses to discovery
Posted by GitBox <gi...@apache.org>.
zyearn commented on pull request #1320:
URL: https://github.com/apache/incubator-brpc/pull/1320#issuecomment-752683475
Could you add the corresponding Unit Test of this case?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] wasphin commented on a change in pull request #1320: Fix registering multiple addresses to discovery
Posted by GitBox <gi...@apache.org>.
wasphin commented on a change in pull request #1320:
URL: https://github.com/apache/incubator-brpc/pull/1320#discussion_r553103147
##########
File path: src/brpc/policy/discovery_naming_service.cpp
##########
@@ -271,9 +272,15 @@ int DiscoveryClient::DoRegister() {
cntl.http_request().set_content_type("application/x-www-form-urlencoded");
butil::IOBufBuilder os;
os << "appid=" << _params.appid
- << "&hostname=" << _params.hostname
- << "&addrs=" << _params.addrs
- << "&env=" << _params.env
+ << "&hostname=" << _params.hostname;
+
+ std::vector<butil::StringPiece> addrs;
+ butil::SplitString(_params.addrs, ',', &addrs);
+ for (size_t i = 0; i < addrs.size(); ++i) {
+ os << "&addrs=" << addrs[i];
+ }
Review comment:
Thanks for the review, and done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org