mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rukletsov <ruklet...@gmail.com>
Subject Re: Review Request 50812: Updated `HealthCheck` protobuf for HTTP and TCP health check.
Date Fri, 05 Aug 2016 10:38:12 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message