-
Notifications
You must be signed in to change notification settings - Fork 184
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
Add grpc-health service implementation #2147
Conversation
Motivation: The gRPC community has defined a health checking standard [1] and API [2]. ServiceTalk should provide a default implementation of this service. [1] https://github.com/grpc/grpc/blob/master/doc/health-checking.md [2] https://github.com/grpc/grpc-proto/blob/master/grpc/health/v1/health.proto
...mples/grpc/health/src/main/java/io/servicetalk/examples/grpc/health/HealthClientExample.java
Outdated
Show resolved
Hide resolved
...mples/grpc/health/src/main/java/io/servicetalk/examples/grpc/health/HealthServerExample.java
Outdated
Show resolved
Hide resolved
servicetalk-grpc-health/src/main/java/io/servicetalk/grpc/health/DefaultHealthService.java
Show resolved
Hide resolved
servicetalk-grpc-health/src/test/java/io/servicetalk/grpc/health/DefaultHealthServiceTest.java
Outdated
Show resolved
Hide resolved
servicetalk-grpc-health/src/main/java/io/servicetalk/grpc/health/DefaultHealthService.java
Show resolved
Hide resolved
servicetalk-grpc-health/src/main/java/io/servicetalk/grpc/health/DefaultHealthService.java
Show resolved
Hide resolved
servicetalk-grpc-health/src/main/java/io/servicetalk/grpc/health/DefaultHealthService.java
Outdated
Show resolved
Hide resolved
servicetalk-grpc-health/src/main/java/io/servicetalk/grpc/health/DefaultHealthService.java
Show resolved
Hide resolved
servicetalk-examples/docs/modules/ROOT/pages/_partials/nav-versioned.adoc
Outdated
Show resolved
Hide resolved
...mples/grpc/health/src/main/java/io/servicetalk/examples/grpc/health/HealthClientExample.java
Outdated
Show resolved
Hide resolved
build failure attributed to #1915 |
public boolean clearStatus(String service) { | ||
final HealthValue healthValue = serviceToStatusMap.remove(service); | ||
if (healthValue != null) { | ||
healthValue.complete(SERVICE_UNKNOWN); |
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.
Here is another problem now. If you do not clear the map on terminated
, clearing status after termination will invoke processor.onComplete()
twice. Our PublisherProcessor
has protection agains that, but still not ideal to violate the spec here. If the PublisherProcessor
impl changes later (or if we let users supply their own processor), it may cause an issue.
I don't think I'm 100% agreed with grpc-java impl to make this method noop after termination. It can still be useful to let users clear NOT_SERVING
status.
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.
Yes our PublisherProcessor
already protects against this. We could add additional protection here but it currently isn't necessary and I would prefer to avoid adding complexity here. Let see start by adding comments and changing the method name.
Motivation:
The gRPC community has defined a health checking standard [1]
and API [2]. ServiceTalk should provide a default implementation
of this service.
[1] https://github.com/grpc/grpc/blob/master/doc/health-checking.md
[2] https://github.com/grpc/grpc-proto/blob/master/grpc/health/v1/health.proto