You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2014/05/03 08:33:27 UTC
Review Request 21052: Introduced a HealthCheck protobuf.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21052/
-----------------------------------------------------------
Review request for mesos, Niklas Nielsen and Vinod Kone.
Bugs: MESOS-741
https://issues.apache.org/jira/browse/MESOS-741
Repository: mesos-git
Description
-------
See summary.
Diffs
-----
include/mesos/mesos.proto e48e50aae248bd9d5289dbaa36753be53b2e592a
src/master/master.cpp c79fdafde6b08d74e6fea71afa79aded41b3a9ab
Diff: https://reviews.apache.org/r/21052/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 21052: Introduced a HealthCheck protobuf.
Posted by Connor Doyle <co...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21052/#review42247
-----------------------------------------------------------
include/mesos/mesos.proto
<https://reviews.apache.org/r/21052/#comment76043>
Could acceptable status codes be represented as a list of ranges? A reasonable default value might be [[200, 399]].
- Connor Doyle
On May 3, 2014, 6:38 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21052/
> -----------------------------------------------------------
>
> (Updated May 3, 2014, 6:38 a.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-741
> https://issues.apache.org/jira/browse/MESOS-741
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e48e50aae248bd9d5289dbaa36753be53b2e592a
> src/master/master.cpp c79fdafde6b08d74e6fea71afa79aded41b3a9ab
>
> Diff: https://reviews.apache.org/r/21052/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 21052: Introduced a HealthCheck protobuf.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21052/#review42111
-----------------------------------------------------------
Patch looks great!
Reviews applied: [21052]
All tests passed.
- Mesos ReviewBot
On May 3, 2014, 6:38 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21052/
> -----------------------------------------------------------
>
> (Updated May 3, 2014, 6:38 a.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-741
> https://issues.apache.org/jira/browse/MESOS-741
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e48e50aae248bd9d5289dbaa36753be53b2e592a
> src/master/master.cpp c79fdafde6b08d74e6fea71afa79aded41b3a9ab
>
> Diff: https://reviews.apache.org/r/21052/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 21052: Introduced a HealthCheck protobuf.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On May 5, 2014, 11:24 p.m., Vinod Kone wrote:
> > include/mesos/mesos.proto, line 151
> > <https://reviews.apache.org/r/21052/diff/2/?file=574065#file574065line151>
> >
> > Do you also want to add optional data that should be in the response?
I could ... given that other implementations I know don't do this I'll just add a TODO for now.
> On May 5, 2014, 11:24 p.m., Vinod Kone wrote:
> > include/mesos/mesos.proto, lines 160-163
> > <https://reviews.apache.org/r/21052/diff/2/?file=574065#file574065line160>
> >
> > the default values for interval_seconds and timeout_seconds seem to indicate that 2 health checks could be in flight. is that intended?
No, the next health check won't fire until the last one completed, so it's (1) health check (2) wait timeout seconds (3) if got health check wait interval seconds else send next health check after timeout seconds. I'll update the comment appropriately.
> On May 5, 2014, 11:24 p.m., Vinod Kone wrote:
> > include/mesos/mesos.proto, lines 212-220
> > <https://reviews.apache.org/r/21052/diff/2/?file=574065#file574065line212>
> >
> > +1 doesn't seem to belong here.
> >
> > also,
> >
> > s/does currently not/currently does not/
This is just some cleanup related to me adding the 'health_check' field. Think of this like adding some new functionality where we move an if statement higher in a function because the code reads better when we do the if check sooner.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21052/#review42217
-----------------------------------------------------------
On May 3, 2014, 6:38 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21052/
> -----------------------------------------------------------
>
> (Updated May 3, 2014, 6:38 a.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-741
> https://issues.apache.org/jira/browse/MESOS-741
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e48e50aae248bd9d5289dbaa36753be53b2e592a
> src/master/master.cpp c79fdafde6b08d74e6fea71afa79aded41b3a9ab
>
> Diff: https://reviews.apache.org/r/21052/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 21052: Introduced a HealthCheck protobuf.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21052/#review42217
-----------------------------------------------------------
Ship it!
include/mesos/mesos.proto
<https://reviews.apache.org/r/21052/#comment76001>
Do you also want to add optional data that should be in the response?
include/mesos/mesos.proto
<https://reviews.apache.org/r/21052/#comment76002>
the default values for interval_seconds and timeout_seconds seem to indicate that 2 health checks could be in flight. is that intended?
include/mesos/mesos.proto
<https://reviews.apache.org/r/21052/#comment76003>
+1 doesn't seem to belong here.
also,
s/does currently not/currently does not/
- Vinod Kone
On May 3, 2014, 6:38 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21052/
> -----------------------------------------------------------
>
> (Updated May 3, 2014, 6:38 a.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-741
> https://issues.apache.org/jira/browse/MESOS-741
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e48e50aae248bd9d5289dbaa36753be53b2e592a
> src/master/master.cpp c79fdafde6b08d74e6fea71afa79aded41b3a9ab
>
> Diff: https://reviews.apache.org/r/21052/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 21052: Introduced a HealthCheck protobuf.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On May 5, 2014, 9:48 p.m., Niklas Nielsen wrote:
> > include/mesos/mesos.proto, line 137
> > <https://reviews.apache.org/r/21052/diff/2/?file=574065#file574065line137>
> >
> > Shouldn't this be a hostname?
I wasn't convinced that a hostname would be sufficient. For example, in the event that we have internal containers that we're trying to query that have private IP addresses (i.e., via Docker) it seemed like it was possible we'd want to run mesos-health-checker both outside of such a container (in which case the NAT'ing might be sufficient) or inside of a container in which case it seemed like having an IP might be nice. I could have both with some semantics about what to do if both are present.
> On May 5, 2014, 9:48 p.m., Niklas Nielsen wrote:
> > include/mesos/mesos.proto, lines 212-220
> > <https://reviews.apache.org/r/21052/diff/2/?file=574065#file574065line212>
> >
> > Should this be a part of the diff? I am a bit confused - so might just be me.
I was just cleaning this up as I went. Not changes here, just a little housekeeping.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21052/#review42207
-----------------------------------------------------------
On May 3, 2014, 6:38 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21052/
> -----------------------------------------------------------
>
> (Updated May 3, 2014, 6:38 a.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-741
> https://issues.apache.org/jira/browse/MESOS-741
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e48e50aae248bd9d5289dbaa36753be53b2e592a
> src/master/master.cpp c79fdafde6b08d74e6fea71afa79aded41b3a9ab
>
> Diff: https://reviews.apache.org/r/21052/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 21052: Introduced a HealthCheck protobuf.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On May 5, 2014, 9:48 p.m., Niklas Nielsen wrote:
> > include/mesos/mesos.proto, line 137
> > <https://reviews.apache.org/r/21052/diff/2/?file=574065#file574065line137>
> >
> > Shouldn't this be a hostname?
>
> Benjamin Hindman wrote:
> I wasn't convinced that a hostname would be sufficient. For example, in the event that we have internal containers that we're trying to query that have private IP addresses (i.e., via Docker) it seemed like it was possible we'd want to run mesos-health-checker both outside of such a container (in which case the NAT'ing might be sufficient) or inside of a container in which case it seemed like having an IP might be nice. I could have both with some semantics about what to do if both are present.
>
> Niklas Nielsen wrote:
> I was thinking along the lines of an full URL instead of ip + port + path. That could accommodate hostname _and_ ip's too, would capture http/https, port, login and so on. Are their downsides I am missing?
>
> Connor Doyle wrote:
> +1 Niklas. A full URL would be the most flexible here, and schedulers could readily derive this value at launchTasks() time.
First, just so everyone is on the same page, the 'ip' field is optional and is meant as an escape hatch for internal usage with the mesos-health-checker command line program that Niklas is working on. The intended behavior here is to make things simple for a scheduler writer by just saying: to check the health of this command do a GET on this path at this port, with these timeouts and expected response statuses (but you figure out the resulting IP/hostname of the command since you're the one running it). I'm almost inclined to remove the 'ip' field and pass it explicitly to mesos-health-checker.
Taking a step back, IMHO requiring that a scheduler constructs a complete URL is less flexible as the containerizer/executor can't "late bind" necessary components of the URL (like the IP/hostname) depending on the execution environment.
I do like that a URL can accommodate a lot of the other fields (like user authentication, https, etc), but I'm not convinced that we won't end up breaking the URL apart anyway in order to (a) do validation or (b) rewrite the URL in order to late-bind things that might have changed based on the environment the mesos-health-checker command line program is run.
So, current proposals:
(1) Use a full URL, validate on the master, and "rewrite" as necessary based on late-binding for mesos-health-checker.
(2) Drop 'ip' completely, add other fields in protobuf (like user authentication) as necessary, let containerizer/executor "late-bind" as necessary, and expect mesos-health-checker to take an argument for the IP/hostname to use for the HTTP(S) request.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21052/#review42207
-----------------------------------------------------------
On May 3, 2014, 6:38 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21052/
> -----------------------------------------------------------
>
> (Updated May 3, 2014, 6:38 a.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-741
> https://issues.apache.org/jira/browse/MESOS-741
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e48e50aae248bd9d5289dbaa36753be53b2e592a
> src/master/master.cpp c79fdafde6b08d74e6fea71afa79aded41b3a9ab
>
> Diff: https://reviews.apache.org/r/21052/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 21052: Introduced a HealthCheck protobuf.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On May 5, 2014, 2:48 p.m., Niklas Nielsen wrote:
> > include/mesos/mesos.proto, line 137
> > <https://reviews.apache.org/r/21052/diff/2/?file=574065#file574065line137>
> >
> > Shouldn't this be a hostname?
>
> Benjamin Hindman wrote:
> I wasn't convinced that a hostname would be sufficient. For example, in the event that we have internal containers that we're trying to query that have private IP addresses (i.e., via Docker) it seemed like it was possible we'd want to run mesos-health-checker both outside of such a container (in which case the NAT'ing might be sufficient) or inside of a container in which case it seemed like having an IP might be nice. I could have both with some semantics about what to do if both are present.
>
> Niklas Nielsen wrote:
> I was thinking along the lines of an full URL instead of ip + port + path. That could accommodate hostname _and_ ip's too, would capture http/https, port, login and so on. Are their downsides I am missing?
>
> Connor Doyle wrote:
> +1 Niklas. A full URL would be the most flexible here, and schedulers could readily derive this value at launchTasks() time.
>
> Benjamin Hindman wrote:
> First, just so everyone is on the same page, the 'ip' field is optional and is meant as an escape hatch for internal usage with the mesos-health-checker command line program that Niklas is working on. The intended behavior here is to make things simple for a scheduler writer by just saying: to check the health of this command do a GET on this path at this port, with these timeouts and expected response statuses (but you figure out the resulting IP/hostname of the command since you're the one running it). I'm almost inclined to remove the 'ip' field and pass it explicitly to mesos-health-checker.
>
> Taking a step back, IMHO requiring that a scheduler constructs a complete URL is less flexible as the containerizer/executor can't "late bind" necessary components of the URL (like the IP/hostname) depending on the execution environment.
>
> I do like that a URL can accommodate a lot of the other fields (like user authentication, https, etc), but I'm not convinced that we won't end up breaking the URL apart anyway in order to (a) do validation or (b) rewrite the URL in order to late-bind things that might have changed based on the environment the mesos-health-checker command line program is run.
>
> So, current proposals:
>
> (1) Use a full URL, validate on the master, and "rewrite" as necessary based on late-binding for mesos-health-checker.
> (2) Drop 'ip' completely, add other fields in protobuf (like user authentication) as necessary, let containerizer/executor "late-bind" as necessary, and expect mesos-health-checker to take an argument for the IP/hostname to use for the HTTP(S) request.
I missed the point that the ip wasn't supposed to be set by the framework - that changes the situation. What the framework developer sees is only path + accepted status codes?
How about HTTP services that listens to specific ips / hostnames? Isn't that pretty common?
I think we should be able to accommodate both scenarios. The URL can encode much more that we would want to add protobuf fields for.
I am up for adding a first simple one though.
How about having two HTTP checks? HTTPSimple / HTTP, or HTTPRelative / HTTPAbsolute ?
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21052/#review42207
-----------------------------------------------------------
On May 2, 2014, 11:38 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21052/
> -----------------------------------------------------------
>
> (Updated May 2, 2014, 11:38 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-741
> https://issues.apache.org/jira/browse/MESOS-741
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e48e50aae248bd9d5289dbaa36753be53b2e592a
> src/master/master.cpp c79fdafde6b08d74e6fea71afa79aded41b3a9ab
>
> Diff: https://reviews.apache.org/r/21052/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 21052: Introduced a HealthCheck protobuf.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On May 5, 2014, 9:48 p.m., Niklas Nielsen wrote:
> > include/mesos/mesos.proto, line 137
> > <https://reviews.apache.org/r/21052/diff/2/?file=574065#file574065line137>
> >
> > Shouldn't this be a hostname?
>
> Benjamin Hindman wrote:
> I wasn't convinced that a hostname would be sufficient. For example, in the event that we have internal containers that we're trying to query that have private IP addresses (i.e., via Docker) it seemed like it was possible we'd want to run mesos-health-checker both outside of such a container (in which case the NAT'ing might be sufficient) or inside of a container in which case it seemed like having an IP might be nice. I could have both with some semantics about what to do if both are present.
>
> Niklas Nielsen wrote:
> I was thinking along the lines of an full URL instead of ip + port + path. That could accommodate hostname _and_ ip's too, would capture http/https, port, login and so on. Are their downsides I am missing?
>
> Connor Doyle wrote:
> +1 Niklas. A full URL would be the most flexible here, and schedulers could readily derive this value at launchTasks() time.
>
> Benjamin Hindman wrote:
> First, just so everyone is on the same page, the 'ip' field is optional and is meant as an escape hatch for internal usage with the mesos-health-checker command line program that Niklas is working on. The intended behavior here is to make things simple for a scheduler writer by just saying: to check the health of this command do a GET on this path at this port, with these timeouts and expected response statuses (but you figure out the resulting IP/hostname of the command since you're the one running it). I'm almost inclined to remove the 'ip' field and pass it explicitly to mesos-health-checker.
>
> Taking a step back, IMHO requiring that a scheduler constructs a complete URL is less flexible as the containerizer/executor can't "late bind" necessary components of the URL (like the IP/hostname) depending on the execution environment.
>
> I do like that a URL can accommodate a lot of the other fields (like user authentication, https, etc), but I'm not convinced that we won't end up breaking the URL apart anyway in order to (a) do validation or (b) rewrite the URL in order to late-bind things that might have changed based on the environment the mesos-health-checker command line program is run.
>
> So, current proposals:
>
> (1) Use a full URL, validate on the master, and "rewrite" as necessary based on late-binding for mesos-health-checker.
> (2) Drop 'ip' completely, add other fields in protobuf (like user authentication) as necessary, let containerizer/executor "late-bind" as necessary, and expect mesos-health-checker to take an argument for the IP/hostname to use for the HTTP(S) request.
>
> Niklas Nielsen wrote:
> I missed the point that the ip wasn't supposed to be set by the framework - that changes the situation. What the framework developer sees is only path + accepted status codes?
> How about HTTP services that listens to specific ips / hostnames? Isn't that pretty common?
>
> I think we should be able to accommodate both scenarios. The URL can encode much more that we would want to add protobuf fields for.
> I am up for adding a first simple one though.
>
> How about having two HTTP checks? HTTPSimple / HTTP, or HTTPRelative / HTTPAbsolute ?
>
>
Yes, having multiple strategies sounds good so I've added a TODO for a 'URL' strategy that can complement the 'HTTP' strategy. I've also dropped the 'ip' field for now. If it turns out we want this we can re-add it in the future.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21052/#review42207
-----------------------------------------------------------
On May 8, 2014, 7:42 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21052/
> -----------------------------------------------------------
>
> (Updated May 8, 2014, 7:42 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-741
> https://issues.apache.org/jira/browse/MESOS-741
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e48e50aae248bd9d5289dbaa36753be53b2e592a
> src/master/master.cpp bfe48bc2fd9528db2888ea472932a92dd8b106eb
>
> Diff: https://reviews.apache.org/r/21052/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 21052: Introduced a HealthCheck protobuf.
Posted by Connor Doyle <co...@mesosphere.io>.
> On May 5, 2014, 9:48 p.m., Niklas Nielsen wrote:
> > include/mesos/mesos.proto, line 137
> > <https://reviews.apache.org/r/21052/diff/2/?file=574065#file574065line137>
> >
> > Shouldn't this be a hostname?
>
> Benjamin Hindman wrote:
> I wasn't convinced that a hostname would be sufficient. For example, in the event that we have internal containers that we're trying to query that have private IP addresses (i.e., via Docker) it seemed like it was possible we'd want to run mesos-health-checker both outside of such a container (in which case the NAT'ing might be sufficient) or inside of a container in which case it seemed like having an IP might be nice. I could have both with some semantics about what to do if both are present.
>
> Niklas Nielsen wrote:
> I was thinking along the lines of an full URL instead of ip + port + path. That could accommodate hostname _and_ ip's too, would capture http/https, port, login and so on. Are their downsides I am missing?
+1 Niklas. A full URL would be the most flexible here, and schedulers could readily derive this value at launchTasks() time.
- Connor
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21052/#review42207
-----------------------------------------------------------
On May 3, 2014, 6:38 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21052/
> -----------------------------------------------------------
>
> (Updated May 3, 2014, 6:38 a.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-741
> https://issues.apache.org/jira/browse/MESOS-741
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e48e50aae248bd9d5289dbaa36753be53b2e592a
> src/master/master.cpp c79fdafde6b08d74e6fea71afa79aded41b3a9ab
>
> Diff: https://reviews.apache.org/r/21052/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 21052: Introduced a HealthCheck protobuf.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On May 5, 2014, 2:48 p.m., Niklas Nielsen wrote:
> > include/mesos/mesos.proto, line 137
> > <https://reviews.apache.org/r/21052/diff/2/?file=574065#file574065line137>
> >
> > Shouldn't this be a hostname?
>
> Benjamin Hindman wrote:
> I wasn't convinced that a hostname would be sufficient. For example, in the event that we have internal containers that we're trying to query that have private IP addresses (i.e., via Docker) it seemed like it was possible we'd want to run mesos-health-checker both outside of such a container (in which case the NAT'ing might be sufficient) or inside of a container in which case it seemed like having an IP might be nice. I could have both with some semantics about what to do if both are present.
I was thinking along the lines of an full URL instead of ip + port + path. That could accommodate hostname _and_ ip's too, would capture http/https, port, login and so on. Are their downsides I am missing?
> On May 5, 2014, 2:48 p.m., Niklas Nielsen wrote:
> > include/mesos/mesos.proto, lines 212-220
> > <https://reviews.apache.org/r/21052/diff/2/?file=574065#file574065line212>
> >
> > Should this be a part of the diff? I am a bit confused - so might just be me.
>
> Benjamin Hindman wrote:
> I was just cleaning this up as I went. Not changes here, just a little housekeeping.
Sorry about that - go drop that misplaced issue :)
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21052/#review42207
-----------------------------------------------------------
On May 2, 2014, 11:38 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21052/
> -----------------------------------------------------------
>
> (Updated May 2, 2014, 11:38 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-741
> https://issues.apache.org/jira/browse/MESOS-741
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e48e50aae248bd9d5289dbaa36753be53b2e592a
> src/master/master.cpp c79fdafde6b08d74e6fea71afa79aded41b3a9ab
>
> Diff: https://reviews.apache.org/r/21052/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 21052: Introduced a HealthCheck protobuf.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21052/#review42207
-----------------------------------------------------------
Looking great - a few questions before a ship-it :)
include/mesos/mesos.proto
<https://reviews.apache.org/r/21052/#comment75956>
Shouldn't this be a hostname?
include/mesos/mesos.proto
<https://reviews.apache.org/r/21052/#comment75957>
Should this be a part of the diff? I am a bit confused - so might just be me.
- Niklas Nielsen
On May 2, 2014, 11:38 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21052/
> -----------------------------------------------------------
>
> (Updated May 2, 2014, 11:38 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-741
> https://issues.apache.org/jira/browse/MESOS-741
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e48e50aae248bd9d5289dbaa36753be53b2e592a
> src/master/master.cpp c79fdafde6b08d74e6fea71afa79aded41b3a9ab
>
> Diff: https://reviews.apache.org/r/21052/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 21052: Introduced a HealthCheck protobuf.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21052/#review42226
-----------------------------------------------------------
include/mesos/mesos.proto
<https://reviews.apache.org/r/21052/#comment76020>
Oops totally missed the move. I thought you were introducing it. Ah Review Board.
- Vinod Kone
On May 3, 2014, 6:38 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21052/
> -----------------------------------------------------------
>
> (Updated May 3, 2014, 6:38 a.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-741
> https://issues.apache.org/jira/browse/MESOS-741
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e48e50aae248bd9d5289dbaa36753be53b2e592a
> src/master/master.cpp c79fdafde6b08d74e6fea71afa79aded41b3a9ab
>
> Diff: https://reviews.apache.org/r/21052/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 21052: Introduced a HealthCheck protobuf.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21052/#review42532
-----------------------------------------------------------
Ship it!
Ship It!
- Niklas Nielsen
On May 8, 2014, 12:42 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21052/
> -----------------------------------------------------------
>
> (Updated May 8, 2014, 12:42 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-741
> https://issues.apache.org/jira/browse/MESOS-741
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e48e50aae248bd9d5289dbaa36753be53b2e592a
> src/master/master.cpp bfe48bc2fd9528db2888ea472932a92dd8b106eb
>
> Diff: https://reviews.apache.org/r/21052/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 21052: Introduced a HealthCheck protobuf.
Posted by Tobi Knaup <to...@knaup.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21052/#review42552
-----------------------------------------------------------
include/mesos/mesos.proto
<https://reviews.apache.org/r/21052/#comment76358>
port, path, ssl could be nicely captured in a URL
include/mesos/mesos.proto
<https://reviews.apache.org/r/21052/#comment76359>
It's pretty common to accept 200-399 per default. Some folks will be surprised if we silently accept errors as healthy. I also can't think of a use case where you would want to redefine 4xx and 5xx as healthy. Maybe just remove statuses altogether?
- Tobi Knaup
On May 8, 2014, 7:42 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21052/
> -----------------------------------------------------------
>
> (Updated May 8, 2014, 7:42 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-741
> https://issues.apache.org/jira/browse/MESOS-741
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e48e50aae248bd9d5289dbaa36753be53b2e592a
> src/master/master.cpp bfe48bc2fd9528db2888ea472932a92dd8b106eb
>
> Diff: https://reviews.apache.org/r/21052/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 21052: Introduced a HealthCheck protobuf.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21052/
-----------------------------------------------------------
(Updated May 8, 2014, 7:42 p.m.)
Review request for mesos, Niklas Nielsen and Vinod Kone.
Changes
-------
Review comments.
Bugs: MESOS-741
https://issues.apache.org/jira/browse/MESOS-741
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
include/mesos/mesos.proto e48e50aae248bd9d5289dbaa36753be53b2e592a
src/master/master.cpp bfe48bc2fd9528db2888ea472932a92dd8b106eb
Diff: https://reviews.apache.org/r/21052/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 21052: Introduced a HealthCheck protobuf.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21052/
-----------------------------------------------------------
(Updated May 3, 2014, 6:38 a.m.)
Review request for mesos, Niklas Nielsen and Vinod Kone.
Bugs: MESOS-741
https://issues.apache.org/jira/browse/MESOS-741
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
include/mesos/mesos.proto e48e50aae248bd9d5289dbaa36753be53b2e592a
src/master/master.cpp c79fdafde6b08d74e6fea71afa79aded41b3a9ab
Diff: https://reviews.apache.org/r/21052/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman