----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50812/#review144907 -----------------------------------------------------------
I've talked to BenM offline and we agreed to simplify messages a bit. Please address the issues (in v1 as well) and we should be ready to commit then. include/mesos/mesos.proto (line 328) <https://reviews.apache.org/r/50812/#comment211076> s/HTTPX/HTTP include/mesos/mesos.proto (line 332) <https://reviews.apache.org/r/50812/#comment211075> We should expend this comment saying what we currently support. How about the following: Describes an HTTP health check. Sends a GET request to `scheme://<host>:port/path`. Return codes bewteen 200 and 399 are treated as success, failure otherwise. Note that `<host>` is not configurable and is resolved automatically, in most cases to `localhost`. include/mesos/mesos.proto (line 333) <https://reviews.apache.org/r/50812/#comment211074> `HTTPx` can be confusing. `HTTP` is a bit confusing as well when referred to `HTTPS`, but arguably less. include/mesos/mesos.proto (lines 334 - 338) <https://reviews.apache.org/r/50812/#comment211079> Instead of `Protocol` enum, we should probably opt for an `optional string scheme` field, to be consistent with the `URL` message. include/mesos/mesos.proto (line 340) <https://reviews.apache.org/r/50812/#comment211069> Enum fields should be optional, see: MESOS-4997. Note that we should probably use an `optional string scheme` field instead (see above). include/mesos/mesos.proto (line 346) <https://reviews.apache.org/r/50812/#comment211077> After talking to BenM offline, we should probably remove the default, as they are hard to maintain. include/mesos/mesos.proto (lines 355 - 356) <https://reviews.apache.org/r/50812/#comment211078> No need to repeat it here if we update the comment for the `HTTP` message as suggested above. include/mesos/mesos.proto (line 362) <https://reviews.apache.org/r/50812/#comment211081> How about this: Describes a socket health check, i.e. based on establishing a TCP connection to the specified port. include/mesos/mesos.proto (lines 364 - 368) <https://reviews.apache.org/r/50812/#comment211080> After talking to BenM, it seems unlikely that we introduce UDP in the nearest future. Hence it makes sense to remove the enum altogether. include/mesos/mesos.proto (line 370) <https://reviews.apache.org/r/50812/#comment211070> Enum fields should be optional, see: MESOS-4997. Most likely we should rid of this field altogether (see above). include/mesos/mesos.proto (line 402) <https://reviews.apache.org/r/50812/#comment211068> Enum fields should be optional, see: MESOS-4997. - Alexander Rukletsov On Aug. 4, 2016, 5:30 p.m., haosdent huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50812/ > ----------------------------------------------------------- > > (Updated Aug. 4, 2016, 5:30 p.m.) > > > Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Mahler, and > Gastón Kleiman. > > > Bugs: MESOS-5987 > https://issues.apache.org/jira/browse/MESOS-5987 > > > Repository: mesos > > > Description > ------- > > Updated `HealthCheck` protobuf for HTTP and TCP health check. > > > Diffs > ----- > > include/mesos/mesos.proto 8c74d0bdc1d15074b55d1be84816307bb9478a38 > include/mesos/v1/mesos.proto 94b59dd75abfa9e8601e59f7a20dfd94bc88fa70 > > Diff: https://reviews.apache.org/r/50812/diff/ > > > Testing > ------- > > > Thanks, > > haosdent huang > >
