health check service

[dapengzhang0]

#647

Implemented health service in grpc-java.

See

https://github.com/grpc/grpc/blob/d0fbba52d6e379b76a69016bc264b96a2318315f/src/ruby/pb/grpc/health/checker.rb

and

https://github.com/grpc/grpc/blob/c236a213ee10f549d328840bc21c307eedd4b545/src/csharp/Grpc.HealthCheck/HealthServiceImpl.cs

for the main part of Ruby and C# implementations.

[ejona86]

We should also update the other copies of this file (so they are all identical):
https://github.com/grpc/grpc/blob/master/src/proto/grpc/health/v1/health.proto
https://github.com/grpc/grpc/blob/master/src/python/grpcio_health_checking/grpc/health/v1/health.proto
https://github.com/grpc/grpc-go/blob/master/health/grpc_health_v1/health.proto

[carl-mastrangelo]

We really should have a grpc/common to put protos, and just submodule this.

I've talked to @ctiller about doing this and he seemed to be in agreement. Maybe we can try it out?

[buchgr]

@carl-mastrangelo I am in favor of that.

[buchgr]

@dapengzhang0 @zhangkun83

I assume the planned usage right now is:

HealthStatusManager manager = 
  new HealthStatusManager(HealthServiceFactory.newHealthService());

serverBuilder
  .addService(new MyService1(manager))
  .addService(new MyService2(manager))
  .build()
  .start();

Wondering if we should add i.e. ServerBuilder.healthStatusManager(...) and integrate this into the ServerServiceDefinition?

[dapengzhang0]

@buchgr
Had a discussion with @zhangkun83 , we don't implement Health feature in ServiceBuilder and/or ServerServiceDefinition right now because we don't want to introduce dependency on protobuff into grpc-core project. (Health depends on protobuff)

[dapengzhang0]

I think I missed this:
/** Unit tests for {@link HealthStatusManager}. */
@RunWith(JUnit4.class)

[buchgr]

@dapengzhang0 yep!

[dapengzhang0]

PTAL

[zhangkun83]

we usually create private methods either for re-using a piece of logic, or breaking down a large method to improve readability. Neither is the case here. It's probably better to merge it with check().

[dapengzhang0]

PTAL again
thx

[zhangkun83]

LGTM

[ejona86]

The package name must include 'v1'. We should probably use io.grpc.health.v1 as the package name. Let me look for the doc that can shed some more light on this.

[ejona86]

The googleapis has some examples. https://github.com/googleapis/googleapis/blob/master/google/bigtable/v1/bigtable_service_messages.proto

The version number is part of the name to allow future versions without colliding. We probably will want to specify java_multiple_files and java_outer_classname as well. The reason it isn't the default is due to backward compatibility. Note that these changes have to be done sooner than later since these names are exposed to the user as part of the API (since ServingStatus is used directly).

The common convention for java_outer_classname is to use FileNameProto. So in this case it would be HealthProto.

[ejona86]

FYI, this can be a normal (non-static import). Doesn't matter which way you do it though.

[ejona86]

@dapengzhang0 LGTM

[asadali]

@dapengzhang0 i am a bit confused about the default behavior here

from the grpc health check doc:

A client can query the server’s health status by calling the Check method, and a deadline should be set on the rpc. The client can OPTIONALLY set the service name it wants to query for health status.

with the current implementation, the server expects a service name to be COMPULSORILY specified in the request and returns a Status.NOT_FOUND for an empty service check. shouldn't it respond with the general health of the server, which should be counted as SERVING independent of the included service's statuses.

[asadali]

nvm: the very next statement from the doc helped clarify it:

The server should register all the services manually and set the individual status, including an empty service name and its status.