----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36816/#review144374 -----------------------------------------------------------
include/mesos/mesos.proto (lines 324 - 346) <https://reviews.apache.org/r/36816/#comment210389> How about this message? ``` // Describes an HTTP health check. message HTTPx { enum Protocol { UNKNOWN = 0; HTTP = 1; HTTPS = 2; } required Protocol protocol = 1 [default = HTTP]; // Port to send the HTTPx request. required uint32 port = 2; // HTTPx request path. optional string path = 3 [default = "/"]; // This field is post-MVP. While adding POST and PUT is simple, // supporting payload is more involved. optional HTTPMethod method = 4 [default = GET]; // This field is post-MVP. Additional HTTP headers that should // be set when sending the health check request. repeated HTTPHeader headers = 5; // Expected response statuses. Not specifying any statuses implies // that any returned status is acceptable. // // NOTE: Nomad uses two sets of statuses: one considered warnings // and other failures. // // NOTE: In the MVP, we can treat return codes between 200 & 399 // as success, and failure otherwise. repeated uint32 statuses = 6; // TODO(benh): Include an 'optional bytes data' field for checking // for specific data in the response. } ``` include/mesos/mesos.proto (line 326) <https://reviews.apache.org/r/36816/#comment210390> I suggest we name it `HTTPx` to reflect we also support HTTPS. include/mesos/mesos.proto (line 335) <https://reviews.apache.org/r/36816/#comment210394> Let's use an enum instead of the flag (see general proposal). include/mesos/mesos.proto (line 342) <https://reviews.apache.org/r/36816/#comment210396> What would this be if not "localhost"? What other values are possible here? How do we deal with virtual networks? Can't we always deduce it and get rid of this field altogether? include/mesos/mesos.proto (lines 366 - 368) <https://reviews.apache.org/r/36816/#comment210399> I suggest we use a "union trick": ``` message HealthCheckInfo { enum Type { UNKNOWN = 0; COMMAND = 1; HTTPX = 2; TCP = 3; } ... required Type type = 8; optional CommandInfo command = 7; optional HTTPx http = 1; optional Socket socket = 9; } include/mesos/v1/mesos.proto (line 339) <https://reviews.apache.org/r/36816/#comment210487> I'm not sure we should tackle this now. It seems that we may want to support ranges here. Additionally, we may want to use two separate ranges for "unhealthy" and "not that healthy tasks". I would say this needs more thinking and hence propose to punt on this in this patch (we will return to it right after). For now my usggestion would be to use 200 and 399 as success and everything else as failure. src/health-check/health_checker.hpp (lines 273 - 276) <https://reviews.apache.org/r/36816/#comment210478> Why do you want to check it every time a health check is performed? It seems like validation for me, which should be done once in the beginning. src/health-check/health_checker.hpp (line 279) <https://reviews.apache.org/r/36816/#comment210483> Will we always have a domain name, I can imagine situations when all we have is an ip, which we have to deduce from `ContainerInfo` and `PortMappings`. Having said that, I think we can go with `localhost`, though it will be slightly complicated for the docker container, but I don't think we should expose it in protobufs. src/health-check/health_checker.hpp (line 281) <https://reviews.apache.org/r/36816/#comment210482> ... HTTP health check ... src/health-check/health_checker.hpp (line 283) <https://reviews.apache.org/r/36816/#comment210480> s/query/httpResponse ? src/health-check/health_checker.hpp (line 301) <https://reviews.apache.org/r/36816/#comment210485> s/code/httpStatusCode src/health-check/health_checker.hpp (lines 302 - 303) <https://reviews.apache.org/r/36816/#comment210486> This can be rather costly. If we want to support allow status codes, we should build an appropriate data structure (map or hash table) in the beginning and look up there. However, see above my comments about this proto field. src/health-check/health_checker.hpp (lines 307 - 308) <https://reviews.apache.org/r/36816/#comment210484> Blank line src/health-check/health_checker.hpp (lines 313 - 315) <https://reviews.apache.org/r/36816/#comment210402> If you accept my "union trick" suggestion, you can check the type here and make sure the appropriate field is set. See what we do for `Resource`, for example. - Alexander Rukletsov On Aug. 1, 2016, 1:15 p.m., haosdent huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36816/ > ----------------------------------------------------------- > > (Updated Aug. 1, 2016, 1:15 p.m.) > > > Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gilbert Song, > Jie Yu, and Timothy Chen. > > > Bugs: MESOS-2533 > https://issues.apache.org/jira/browse/MESOS-2533 > > > Repository: mesos > > > Description > ------- > > Supported HTTP/HTTPS in health check. > > > Diffs > ----- > > include/mesos/mesos.proto 590e169108b2ce5881734ec7f4b01cef9937461a > include/mesos/v1/mesos.proto 94b59dd75abfa9e8601e59f7a20dfd94bc88fa70 > src/health-check/health_checker.hpp > b28a9cf3c6c9217c0b2453ac6e34d88e1f648d61 > > Diff: https://reviews.apache.org/r/36816/diff/ > > > Testing > ------- > > * Add unit test cases: HealthCheckTest.HealthyTaskViaHttp and > HealthCheckTest.HealthyTaskNotMatchHttpStatuses > make check > > > Thanks, > > haosdent huang > >
