Skip to content
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

Merged
merged 14 commits into from
Mar 15, 2022
Merged

Conversation

Scottmitch
Copy link
Member

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

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
@Scottmitch
Copy link
Member Author

build failure attributed to #1915

public boolean clearStatus(String service) {
final HealthValue healthValue = serviceToStatusMap.remove(service);
if (healthValue != null) {
healthValue.complete(SERVICE_UNKNOWN);
Copy link
Member

@idelpivnitskiy idelpivnitskiy Mar 15, 2022

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.

Copy link
Member Author

@Scottmitch Scottmitch Mar 15, 2022

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.

@bondolo bondolo changed the title Add grpc-health service implementaiton Add grpc-health service implementation Mar 15, 2022
@Scottmitch Scottmitch merged commit b274746 into apple:main Mar 15, 2022
@Scottmitch Scottmitch deleted the health branch March 15, 2022 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants