[GitHub] [incubator-brpc] zyearn commented on a change in pull request #1529: Set hostname rather than ip when channel Init by hostname but not host in http_requst().uri()

[GitBox]

zyearn commented on a change in pull request #1529:
URL: https://github.com/apache/incubator-brpc/pull/1529#discussion_r697889292



##########
File path: src/brpc/channel.cpp
##########
@@ -295,6 +295,7 @@ int Channel::InitSingle(const butil::EndPoint& 
server_addr_and_port,
                      NULL, &_options.mutable_ssl_options()->sni_name, NULL);
         }
     }
+    ParseServiceName(raw_server_address);

Review comment:
       和下面的comment一样，移到前面去可以合并一下

##########
File path: src/brpc/channel.cpp
##########
@@ -568,4 +576,10 @@ int Channel::CheckHealth() {
     }
 }
 
+void Channel::ParseServiceName(const char* server_addr) {
+    if (ParseURL(server_addr, NULL, &_service_name, NULL) != 0) {
+        _service_name.clear();
+    }

Review comment:
       
当时是觉得函数太长，如果目前这个函数这么短了，感觉就不需要封装ParseServiceName了，在调用的地方直接ParseURL(...)即可，如果失败的话_service_name都不会被设置（参考ParseURL的实现），所以_service_name.clear()也不需要了。

##########
File path: src/brpc/channel.cpp
##########
@@ -332,6 +333,7 @@ int Channel::Init(const char* ns_url,
                      NULL, &_options.mutable_ssl_options()->sni_name, NULL);
         }
     }
+    ParseServiceName(ns_url);

Review comment:
       这一整段可以合并一下，少一次parse：
   ```
       Parseurl...
       if (_options.protocol == brpc::PROTOCOL_HTTP &&
           ::strncmp(ns_url, "https://";, 8) == 0) {
           if (_options.mutable_ssl_options()->sni_name.empty()) {
               _options.mutable_ssl_options()->sni_name = _service_name;
           }
       }
   ```

##########
File path: src/brpc/details/http_message.cpp
##########
@@ -567,6 +567,8 @@ void MakeRawHttpRequest(butil::IOBuf* request,
             os << uri.host();
             if (uri.port() >= 0) {
                 os << ':' << uri.port();
+            } else if (remote_side.port != 0) {
+                os << ':' << remote_side.port;

Review comment:
       As comments said, we can't afford to break the backward compatibility.




-- 
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: dev-unsubscr...@brpc.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org
For additional commands, e-mail: dev-h...@brpc.apache.org