You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2017/01/12 13:18:13 UTC
Review Request 55453: Updated comments in `HealthCheck` protobuf for
clarity.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55453/
-----------------------------------------------------------
Review request for mesos, Gast�n Kleiman, haosdent huang, and Vinod Kone.
Repository: mesos
Description
-------
See summary.
Diffs
-----
include/mesos/mesos.proto ab68ff85c4af5d254779b30a7f27eda9fcb790eb
include/mesos/v1/mesos.proto caefa239be6ead10b9a5fc91ba120ea9c8775313
Diff: https://reviews.apache.org/r/55453/diff/
Testing
-------
None: not a functional change.
Thanks,
Alexander Rukletsov
Re: Review Request 55453: Updated comments in `HealthCheck` protobuf
for clarity.
Posted by Gastón Kleiman <ga...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55453/#review161504
-----------------------------------------------------------
Ship it!
Ship It!
- Gast�n Kleiman
On Jan. 12, 2017, 1:18 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55453/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2017, 1:18 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman, haosdent huang, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto ab68ff85c4af5d254779b30a7f27eda9fcb790eb
> include/mesos/v1/mesos.proto caefa239be6ead10b9a5fc91ba120ea9c8775313
>
> Diff: https://reviews.apache.org/r/55453/diff/
>
>
> Testing
> -------
>
> None: not a functional change.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 55453: Updated comments in `HealthCheck` protobuf
for clarity.
Posted by Robert Lacroix <ma...@robertlacroix.com>.
> On Jan. 31, 2017, 2:01 a.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 424-426
> > <https://reviews.apache.org/r/55453/diff/4/?file=1610587#file1610587line424>
> >
> > The most direct interpretation for the delay is actually the time since the task was launched right? AFAIK Mesos provided executors immediately send TASK_RUNNING but this is not generally required right?
>
> Alexander Rukletsov wrote:
> What do you understand under launch. Is it when the task has been submitted to the master? Or when a scheduler got `TASK_STAGING`? Or all necessary artifacts have been fetched and _executor_ launches the task? Built-in executors indeed send task running right after they launched the task (and at the same time start the delay timer for checks or health checks). Custom executors are out of our control, they may send updates later on.
>
> Since we cannot start the delay timer at `TASK_STAGING` or when the task is submitted to the master, we can either agree on `TASK_RUNNING` or "task has been submitted to the executor". I think the latter event is vague and hence suggested to tie the delay to `TASK_RUNNING`. For built-in executors both events occur close to each other in time.
I would say it should start after `TASK_STARTING`. For years we have been using the following semantic:
`TASK_STARTING`: The executor launched the task
`TASK_RUNNING`: The task is ready to receive production traffic (i.e. passing health checks if there are any defined. If there aren't health checks defined it's being sent right after `TASK_STARTING`).
If delay_seconds only starts after `TASK_RUNNING` this semantic doesn't make sense anymore. If that's the case, I'm not sure why we even have a distinction between `TASK_RUNNING` and `TASK_STARTING`?
- Robert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55453/#review163612
-----------------------------------------------------------
On Jan. 20, 2017, 2:48 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55453/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2017, 2:48 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman, haosdent huang, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8f14444d6957a97eff1e0a94817d38e7a7de6d69
> include/mesos/v1/mesos.proto 74e7851b147ab821dceeab6e838d34b092f101c3
>
> Diff: https://reviews.apache.org/r/55453/diff/
>
>
> Testing
> -------
>
> None: not a functional change.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 55453: Updated comments in `HealthCheck` protobuf
for clarity.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Jan. 31, 2017, 2:01 a.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 424-426
> > <https://reviews.apache.org/r/55453/diff/4/?file=1610587#file1610587line424>
> >
> > The most direct interpretation for the delay is actually the time since the task was launched right? AFAIK Mesos provided executors immediately send TASK_RUNNING but this is not generally required right?
What do you understand under launch. Is it when the task has been submitted to the master? Or when a scheduler got `TASK_STAGING`? Or all necessary artifacts have been fetched and _executor_ launches the task? Built-in executors indeed send task running right after they launched the task (and at the same time start the delay timer for checks or health checks). Custom executors are out of our control, they may send updates later on.
Since we cannot start the delay timer at `TASK_STAGING` or when the task is submitted to the master, we can either agree on `TASK_RUNNING` or "task has been submitted to the executor". I think the latter event is vague and hence suggested to tie the delay to `TASK_RUNNING`. For built-in executors both events occur close to each other in time.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55453/#review163612
-----------------------------------------------------------
On Jan. 20, 2017, 2:48 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55453/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2017, 2:48 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman, haosdent huang, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8f14444d6957a97eff1e0a94817d38e7a7de6d69
> include/mesos/v1/mesos.proto 74e7851b147ab821dceeab6e838d34b092f101c3
>
> Diff: https://reviews.apache.org/r/55453/diff/
>
>
> Testing
> -------
>
> None: not a functional change.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 55453: Updated comments in `HealthCheck` protobuf
for clarity.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Jan. 31, 2017, 2:01 a.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 424-426
> > <https://reviews.apache.org/r/55453/diff/4/?file=1610587#file1610587line424>
> >
> > The most direct interpretation for the delay is actually the time since the task was launched right? AFAIK Mesos provided executors immediately send TASK_RUNNING but this is not generally required right?
>
> Alexander Rukletsov wrote:
> What do you understand under launch. Is it when the task has been submitted to the master? Or when a scheduler got `TASK_STAGING`? Or all necessary artifacts have been fetched and _executor_ launches the task? Built-in executors indeed send task running right after they launched the task (and at the same time start the delay timer for checks or health checks). Custom executors are out of our control, they may send updates later on.
>
> Since we cannot start the delay timer at `TASK_STAGING` or when the task is submitted to the master, we can either agree on `TASK_RUNNING` or "task has been submitted to the executor". I think the latter event is vague and hence suggested to tie the delay to `TASK_RUNNING`. For built-in executors both events occur close to each other in time.
>
> Robert Lacroix wrote:
> I would say it should start after `TASK_STARTING`. For years we have been using the following semantic:
>
> `TASK_STARTING`: The executor launched the task
> `TASK_RUNNING`: The task is ready to receive production traffic (i.e. passing health checks if there are any defined. If there aren't health checks defined it's being sent right after `TASK_STARTING`).
>
> If delay_seconds only starts after `TASK_RUNNING` this semantic doesn't make sense anymore. If that's the case, I'm not sure why we even have a distinction between `TASK_RUNNING` and `TASK_STARTING`?
I see, good point. Build-in executors do not send `TASK_STARTING` so we can agree that the delay timer starts right after `TASK_STARTING`.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55453/#review163612
-----------------------------------------------------------
On Jan. 20, 2017, 2:48 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55453/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2017, 2:48 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman, haosdent huang, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8f14444d6957a97eff1e0a94817d38e7a7de6d69
> include/mesos/v1/mesos.proto 74e7851b147ab821dceeab6e838d34b092f101c3
>
> Diff: https://reviews.apache.org/r/55453/diff/
>
>
> Testing
> -------
>
> None: not a functional change.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 55453: Updated comments in `HealthCheck` protobuf
for clarity.
Posted by Jiang Yan Xu <xu...@apple.com>.
> On Jan. 30, 2017, 6:01 p.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 424-426
> > <https://reviews.apache.org/r/55453/diff/4/?file=1610587#file1610587line424>
> >
> > The most direct interpretation for the delay is actually the time since the task was launched right? AFAIK Mesos provided executors immediately send TASK_RUNNING but this is not generally required right?
>
> Alexander Rukletsov wrote:
> What do you understand under launch. Is it when the task has been submitted to the master? Or when a scheduler got `TASK_STAGING`? Or all necessary artifacts have been fetched and _executor_ launches the task? Built-in executors indeed send task running right after they launched the task (and at the same time start the delay timer for checks or health checks). Custom executors are out of our control, they may send updates later on.
>
> Since we cannot start the delay timer at `TASK_STAGING` or when the task is submitted to the master, we can either agree on `TASK_RUNNING` or "task has been submitted to the executor". I think the latter event is vague and hence suggested to tie the delay to `TASK_RUNNING`. For built-in executors both events occur close to each other in time.
>
> Robert Lacroix wrote:
> I would say it should start after `TASK_STARTING`. For years we have been using the following semantic:
>
> `TASK_STARTING`: The executor launched the task
> `TASK_RUNNING`: The task is ready to receive production traffic (i.e. passing health checks if there are any defined. If there aren't health checks defined it's being sent right after `TASK_STARTING`).
>
> If delay_seconds only starts after `TASK_RUNNING` this semantic doesn't make sense anymore. If that's the case, I'm not sure why we even have a distinction between `TASK_RUNNING` and `TASK_STARTING`?
>
> Alexander Rukletsov wrote:
> I see, good point. Build-in executors do not send `TASK_STARTING` so we can agree that the delay timer starts right after `TASK_STARTING`.
Sorry I overlooked this conversation. By "launch" I meant the moment when the executor "launches" it (by fork-execing, launching nested container or simply calling a function). Since `TASK_STARTING` is not a required state I wouldn't use it to defined "launch" in general but one can certainly associate `TASK_STARTING` with this action.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55453/#review163612
-----------------------------------------------------------
On Jan. 20, 2017, 6:48 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55453/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2017, 6:48 a.m.)
>
>
> Review request for mesos, Gast�n Kleiman, haosdent huang, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8f14444d6957a97eff1e0a94817d38e7a7de6d69
> include/mesos/v1/mesos.proto 74e7851b147ab821dceeab6e838d34b092f101c3
>
> Diff: https://reviews.apache.org/r/55453/diff/
>
>
> Testing
> -------
>
> None: not a functional change.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 55453: Updated comments in `HealthCheck` protobuf
for clarity.
Posted by Jiang Yan Xu <xu...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55453/#review163612
-----------------------------------------------------------
include/mesos/mesos.proto (lines 424 - 426)
<https://reviews.apache.org/r/55453/#comment235079>
The most direct interpretation for the delay is actually the time since the task was launched right? AFAIK Mesos provided executors immediately send TASK_RUNNING but this is not generally required right?
- Jiang Yan Xu
On Jan. 20, 2017, 6:48 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55453/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2017, 6:48 a.m.)
>
>
> Review request for mesos, Gast�n Kleiman, haosdent huang, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8f14444d6957a97eff1e0a94817d38e7a7de6d69
> include/mesos/v1/mesos.proto 74e7851b147ab821dceeab6e838d34b092f101c3
>
> Diff: https://reviews.apache.org/r/55453/diff/
>
>
> Testing
> -------
>
> None: not a functional change.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 55453: Updated comments in `HealthCheck` protobuf
for clarity.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55453/#review162556
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On Jan. 20, 2017, 2:48 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55453/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2017, 2:48 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman, haosdent huang, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8f14444d6957a97eff1e0a94817d38e7a7de6d69
> include/mesos/v1/mesos.proto 74e7851b147ab821dceeab6e838d34b092f101c3
>
> Diff: https://reviews.apache.org/r/55453/diff/
>
>
> Testing
> -------
>
> None: not a functional change.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 55453: Updated comments in `HealthCheck` protobuf
for clarity.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55453/
-----------------------------------------------------------
(Updated Jan. 20, 2017, 2:48 p.m.)
Review request for mesos, Gast�n Kleiman, haosdent huang, and Vinod Kone.
Changes
-------
Rebased.
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
include/mesos/mesos.proto 8f14444d6957a97eff1e0a94817d38e7a7de6d69
include/mesos/v1/mesos.proto 74e7851b147ab821dceeab6e838d34b092f101c3
Diff: https://reviews.apache.org/r/55453/diff/
Testing
-------
None: not a functional change.
Thanks,
Alexander Rukletsov
Re: Review Request 55453: Updated comments in `HealthCheck` protobuf
for clarity.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55453/
-----------------------------------------------------------
(Updated Jan. 19, 2017, 5:39 p.m.)
Review request for mesos, Gast�n Kleiman, haosdent huang, and Vinod Kone.
Changes
-------
Vinod's comments.
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
include/mesos/mesos.proto 8f14444d6957a97eff1e0a94817d38e7a7de6d69
include/mesos/v1/mesos.proto 74e7851b147ab821dceeab6e838d34b092f101c3
Diff: https://reviews.apache.org/r/55453/diff/
Testing
-------
None: not a functional change.
Thanks,
Alexander Rukletsov
Re: Review Request 55453: Updated comments in `HealthCheck` protobuf
for clarity.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55453/#review162053
-----------------------------------------------------------
include/mesos/mesos.proto (lines 424 - 426)
<https://reviews.apache.org/r/55453/#comment233301>
How about:
// Amount of time to wait to start health checking
// the task *after* it transitions to TASK_RUNNING.
include/mesos/mesos.proto (lines 428 - 430)
<https://reviews.apache.org/r/55453/#comment233302>
// Amount of time to wait after the previous health check finished or timed out
// to start the next health check.
- Vinod Kone
On Jan. 18, 2017, 9:30 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55453/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2017, 9:30 a.m.)
>
>
> Review request for mesos, Gast�n Kleiman, haosdent huang, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8f14444d6957a97eff1e0a94817d38e7a7de6d69
> include/mesos/v1/mesos.proto 74e7851b147ab821dceeab6e838d34b092f101c3
>
> Diff: https://reviews.apache.org/r/55453/diff/
>
>
> Testing
> -------
>
> None: not a functional change.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 55453: Updated comments in `HealthCheck` protobuf
for clarity.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55453/
-----------------------------------------------------------
(Updated Jan. 18, 2017, 9:30 a.m.)
Review request for mesos, Gast�n Kleiman, haosdent huang, and Vinod Kone.
Changes
-------
Rebased.
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
include/mesos/mesos.proto 8f14444d6957a97eff1e0a94817d38e7a7de6d69
include/mesos/v1/mesos.proto 74e7851b147ab821dceeab6e838d34b092f101c3
Diff: https://reviews.apache.org/r/55453/diff/
Testing
-------
None: not a functional change.
Thanks,
Alexander Rukletsov