Re: [PR] support grpc health check (brpc)
chenBright commented on PR #2493: URL: https://github.com/apache/brpc/pull/2493#issuecomment-1876959889 HealthService好像已经支持了吧。 https://github.com/apache/brpc/blob/master/src/brpc/builtin/health_service.cpp -- 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
Re: [PR] support grpc health check (brpc)
jiangyt-git commented on PR #2493:
URL: https://github.com/apache/brpc/pull/2493#issuecomment-1877034356
> HealthService好像已经支持了吧。
https://github.com/apache/brpc/blob/master/src/brpc/builtin/health_service.cpp
支持的grpc 官方定义的健康检查协议，response是这样的：
message HealthCheckResponse {
enum ServingStatus {
UNKNOWN = 0;
SERVING = 1;
NOT_SERVING = 2;
SERVICE_UNKNOWN = 3; // Used only by the Watch method.
}
optional ServingStatus status = 1;
}
--
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
Re: [PR] support grpc health check (brpc)
jiangyt-git commented on PR #2493:
URL: https://github.com/apache/brpc/pull/2493#issuecomment-1877066322
> > HealthService好像已经支持了吧。
https://github.com/apache/brpc/blob/master/src/brpc/builtin/health_service.cpp
>
> 支持的grpc 官方定义的健康检查协议，response是这样的： message HealthCheckResponse { enum
ServingStatus { UNKNOWN = 0; SERVING = 1; NOT_SERVING = 2; SERVICE_UNKNOWN = 3;
// Used only by the Watch method. } optional ServingStatus status = 1; }
请求对应的path是: /grpc.health.v1.Health/Check
--
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
Re: [PR] support grpc health check (brpc)
chenBright commented on PR #2493: URL: https://github.com/apache/brpc/pull/2493#issuecomment-1877434730 health_reporter满足不了grpc健康检查的要求吧，获取不了HealthCheckRequest和设置不了HealthCheckResponse。 -- 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
Re: [PR] support grpc health check (brpc)
jiangyt-git commented on PR #2493: URL: https://github.com/apache/brpc/pull/2493#issuecomment-1878075189 > health_reporter满足不了grpc健康检查的要求吧，获取不了HealthCheckRequest和设置不了HealthCheckResponse。 grpc和http 都是写的 cntl->response_attachment()，理论上是可以定制的。 -- 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
Re: [PR] support grpc health check (brpc)
chenBright commented on PR #2493: URL: https://github.com/apache/brpc/pull/2493#issuecomment-1878079641 可以补充一下UT吗 -- 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
Re: [PR] support grpc health check (brpc)
jiangyt-git commented on PR #2493: URL: https://github.com/apache/brpc/pull/2493#issuecomment-1878081248 > 可以补充一下UT吗 好的，我补一下。 -- 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
Re: [PR] support grpc health check (brpc)
jiangyt-git commented on PR #2493: URL: https://github.com/apache/brpc/pull/2493#issuecomment-1879503854 > 可以补充一下UT吗 已经加好了 -- 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
Re: [PR] support grpc health check (brpc)
chenBright commented on code in PR #2493:
URL: https://github.com/apache/brpc/pull/2493#discussion_r1443656015
##
src/brpc/server.cpp:
##
@@ -564,6 +565,10 @@ int Server::AddBuiltinServices() {
LOG(ERROR) << "Fail to add GetJsService";
return -1;
}
+if (AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
Review Comment:
如果ServerOptions::enabled_protocols没有h2，是不是没有必要打开这个内置服务呢？
--
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
Re: [PR] support grpc health check (brpc)
jiangyt-git commented on code in PR #2493:
URL: https://github.com/apache/brpc/pull/2493#discussion_r1444134531
##
src/brpc/server.cpp:
##
@@ -564,6 +565,10 @@ int Server::AddBuiltinServices() {
LOG(ERROR) << "Fail to add GetJsService";
return -1;
}
+if (AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
Review Comment:
那是加个协议的检查？还是直接加个gflag呢?
--
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
Re: [PR] support grpc health check (brpc)
chenBright commented on code in PR #2493:
URL: https://github.com/apache/brpc/pull/2493#discussion_r1444138945
##
src/brpc/server.cpp:
##
@@ -564,6 +565,10 @@ int Server::AddBuiltinServices() {
LOG(ERROR) << "Fail to add GetJsService";
return -1;
}
+if (AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
Review Comment:
这是跟协议强相关，加个协议的检查，合适一点吧
--
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
Re: [PR] support grpc health check (brpc)
jiangyt-git commented on code in PR #2493:
URL: https://github.com/apache/brpc/pull/2493#discussion_r1445946792
##
src/brpc/server.cpp:
##
@@ -564,6 +565,10 @@ int Server::AddBuiltinServices() {
LOG(ERROR) << "Fail to add GetJsService";
return -1;
}
+if (AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
Review Comment:
加好了
--
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
Re: [PR] support grpc health check (brpc)
chenBright commented on code in PR #2493:
URL: https://github.com/apache/brpc/pull/2493#discussion_r1446006897
##
src/brpc/server.cpp:
##
@@ -565,13 +565,22 @@ int Server::AddBuiltinServices() {
LOG(ERROR) << "Fail to add GetJsService";
return -1;
}
-if (AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
+if (ProtocolEnabled("h2") &&
+AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
LOG(ERROR) << "Fail to add GrpcHealthCheckService";
return -1;
}
return 0;
}
+bool Server::ProtocolEnabled(const char * name) {
+if (!_options.enabled_protocols.empty() &&
+_options.enabled_protocols.find(name) == std::string::npos) {
Review Comment:
需要StringSplitter按照空格分割之后，再匹配吗？
--
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
Re: [PR] support grpc health check (brpc)
jiangyt-git commented on code in PR #2493:
URL: https://github.com/apache/brpc/pull/2493#discussion_r1446055196
##
src/brpc/server.cpp:
##
@@ -565,13 +565,22 @@ int Server::AddBuiltinServices() {
LOG(ERROR) << "Fail to add GetJsService";
return -1;
}
-if (AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
+if (ProtocolEnabled("h2") &&
+AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
LOG(ERROR) << "Fail to add GrpcHealthCheckService";
return -1;
}
return 0;
}
+bool Server::ProtocolEnabled(const char * name) {
+if (!_options.enabled_protocols.empty() &&
+_options.enabled_protocols.find(name) == std::string::npos) {
Review Comment:
这个需要这么的严格吗？这个要检查，绝对检查的话只检查h2:grpc也可以，但如果用户server 设置了 h2、h2:proto 、h2:json
等应该也可以给调通。以及未来有可能支持的h2:grpc streaming。 当然如果你觉得要需要的话，我也可以改成split之后的严格检查。
--
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
Re: [PR] support grpc health check (brpc)
jiangyt-git commented on code in PR #2493:
URL: https://github.com/apache/brpc/pull/2493#discussion_r1446057250
##
src/brpc/server.cpp:
##
@@ -565,13 +565,22 @@ int Server::AddBuiltinServices() {
LOG(ERROR) << "Fail to add GetJsService";
return -1;
}
-if (AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
+if (ProtocolEnabled("h2") &&
+AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
LOG(ERROR) << "Fail to add GrpcHealthCheckService";
return -1;
}
return 0;
}
+bool Server::ProtocolEnabled(const char * name) {
+if (!_options.enabled_protocols.empty() &&
+_options.enabled_protocols.find(name) == std::string::npos) {
Review Comment:
如果是检查协议格式的话， "h2sss“ 之类协议，会在后面因为找不到协议handler而初始化失败。所以前面的判断其实应该不重要。
--
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
Re: [PR] support grpc health check (brpc)
chenBright commented on code in PR #2493:
URL: https://github.com/apache/brpc/pull/2493#discussion_r1446061274
##
src/brpc/server.cpp:
##
@@ -565,13 +565,22 @@ int Server::AddBuiltinServices() {
LOG(ERROR) << "Fail to add GetJsService";
return -1;
}
-if (AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
+if (ProtocolEnabled("h2") &&
+AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
LOG(ERROR) << "Fail to add GrpcHealthCheckService";
return -1;
}
return 0;
}
+bool Server::ProtocolEnabled(const char * name) {
+if (!_options.enabled_protocols.empty() &&
+_options.enabled_protocols.find(name) == std::string::npos) {
Review Comment:
以h2开头，避免其他协议名字包含了h2，导致误判？
--
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
Re: [PR] support grpc health check (brpc)
chenBright commented on code in PR #2493:
URL: https://github.com/apache/brpc/pull/2493#discussion_r1446061274
##
src/brpc/server.cpp:
##
@@ -565,13 +565,22 @@ int Server::AddBuiltinServices() {
LOG(ERROR) << "Fail to add GetJsService";
return -1;
}
-if (AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
+if (ProtocolEnabled("h2") &&
+AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
LOG(ERROR) << "Fail to add GrpcHealthCheckService";
return -1;
}
return 0;
}
+bool Server::ProtocolEnabled(const char * name) {
+if (!_options.enabled_protocols.empty() &&
+_options.enabled_protocols.find(name) == std::string::npos) {
Review Comment:
是不是可以判断协议是否以h2开头，避免其他协议名字包含了h2，导致误判？
##
src/brpc/server.cpp:
##
@@ -565,13 +565,22 @@ int Server::AddBuiltinServices() {
LOG(ERROR) << "Fail to add GetJsService";
return -1;
}
-if (AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
+if (ProtocolEnabled("h2") &&
+AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
LOG(ERROR) << "Fail to add GrpcHealthCheckService";
return -1;
}
return 0;
}
+bool Server::ProtocolEnabled(const char * name) {
+if (!_options.enabled_protocols.empty() &&
+_options.enabled_protocols.find(name) == std::string::npos) {
Review Comment:
是不是可以判断协议是否以h2开头，避免其他协议名字包含了h2导致误判的问题？
--
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
Re: [PR] support grpc health check (brpc)
jiangyt-git commented on code in PR #2493:
URL: https://github.com/apache/brpc/pull/2493#discussion_r1446065775
##
src/brpc/server.cpp:
##
@@ -565,13 +565,22 @@ int Server::AddBuiltinServices() {
LOG(ERROR) << "Fail to add GetJsService";
return -1;
}
-if (AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
+if (ProtocolEnabled("h2") &&
+AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
LOG(ERROR) << "Fail to add GrpcHealthCheckService";
return -1;
}
return 0;
}
+bool Server::ProtocolEnabled(const char * name) {
+if (!_options.enabled_protocols.empty() &&
+_options.enabled_protocols.find(name) == std::string::npos) {
Review Comment:
okok
--
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
Re: [PR] support grpc health check (brpc)
jiangyt-git commented on code in PR #2493:
URL: https://github.com/apache/brpc/pull/2493#discussion_r1446987436
##
src/brpc/server.cpp:
##
@@ -565,13 +565,22 @@ int Server::AddBuiltinServices() {
LOG(ERROR) << "Fail to add GetJsService";
return -1;
}
-if (AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
+if (ProtocolEnabled("h2") &&
+AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
LOG(ERROR) << "Fail to add GrpcHealthCheckService";
return -1;
}
return 0;
}
+bool Server::ProtocolEnabled(const char * name) {
+if (!_options.enabled_protocols.empty() &&
+_options.enabled_protocols.find(name) == std::string::npos) {
Review Comment:
突然才发现，h2和http一样、是server默认开启的，不能在enabled_protocols里配置的。https://github.com/apache/brpc/blob/master/src/brpc/server.cpp#L598
--
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
Re: [PR] support grpc health check (brpc)
chenBright commented on code in PR #2493:
URL: https://github.com/apache/brpc/pull/2493#discussion_r1447002979
##
src/brpc/server.cpp:
##
@@ -565,13 +565,22 @@ int Server::AddBuiltinServices() {
LOG(ERROR) << "Fail to add GetJsService";
return -1;
}
-if (AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
+if (ProtocolEnabled("h2") &&
+AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
LOG(ERROR) << "Fail to add GrpcHealthCheckService";
return -1;
}
return 0;
}
+bool Server::ProtocolEnabled(const char * name) {
+if (!_options.enabled_protocols.empty() &&
+_options.enabled_protocols.find(name) == std::string::npos) {
Review Comment:
哦哦，没有留意到h2也是默认开启的。那这里应该不需要校验了
--
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
Re: [PR] support grpc health check (brpc)
chenBright commented on code in PR #2493:
URL: https://github.com/apache/brpc/pull/2493#discussion_r1447002979
##
src/brpc/server.cpp:
##
@@ -565,13 +565,22 @@ int Server::AddBuiltinServices() {
LOG(ERROR) << "Fail to add GetJsService";
return -1;
}
-if (AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
+if (ProtocolEnabled("h2") &&
+AddBuiltinService(new (std::nothrow) GrpcHealthCheckService)) {
LOG(ERROR) << "Fail to add GrpcHealthCheckService";
return -1;
}
return 0;
}
+bool Server::ProtocolEnabled(const char * name) {
+if (!_options.enabled_protocols.empty() &&
+_options.enabled_protocols.find(name) == std::string::npos) {
Review Comment:
哦哦，没有留意到h2时默认开启的。那这里应该不需要校验了
--
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
Re: [PR] support grpc health check (brpc)
chenBright commented on PR #2493: URL: https://github.com/apache/brpc/pull/2493#issuecomment-1884469599 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. 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
Re: [PR] support grpc health check (brpc)
wwbmmm commented on PR #2493: URL: https://github.com/apache/brpc/pull/2493#issuecomment-1894833757 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. 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
Re: [PR] support grpc health check (brpc)
wwbmmm merged PR #2493: URL: https://github.com/apache/brpc/pull/2493 -- 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
