You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Megha Sharma <ms...@apple.com> on 2017/12/01 16:20:35 UTC
Review Request 64250: Added new reasons task status update.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64250/
-----------------------------------------------------------
Review request for mesos, Ilya Pronin, James Peach, and Jiang Yan Xu.
Repository: mesos
Description
-------
Added new reasons `REASON_SLAVE_REREGISTERED` and
`REASON_AGENT_REREGISTERED` for v0 and v1 task status update.
The new reason will be used when master starts to send status
update during the re-registeration of an agent which was
previosuly removed by the master because of being unreachable.
Diffs
-----
docs/task-state-reasons.md 07f7927e28fe5624a6c9b74de731483aaa729ca3
include/mesos/mesos.proto 11089b7d6f48f001ae64e8b0c67a2732684aa2fa
include/mesos/v1/mesos.proto c496da927903f472c78d455b7fdf886d8fb692c4
Diff: https://reviews.apache.org/r/64250/diff/1/
Testing
-------
Thanks,
Megha Sharma
Re: Review Request 64250: Added new reasons task status update.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64250/#review192531
-----------------------------------------------------------
docs/task-state-reasons.md
Lines 370-372 (patched)
<https://reviews.apache.org/r/64250/#comment270738>
Should it be in the "Reasons for Non-Terminal Status Updates" section and is there a reason to refer to TASK_LOST?
docs/task-state-reasons.md
Lines 479 (patched)
<https://reviews.apache.org/r/64250/#comment270740>
"The task was part of an accepted offer": this is irrevelent to either TASK_UNREACHABLE or the reason REASON_SLAVE_REREGISTERED.
docs/task-state-reasons.md
Lines 480-482 (patched)
<https://reviews.apache.org/r/64250/#comment270741>
We don't need to put too much details on TASK_UNREACHABLE since it's described elsewhere.
docs/task-state-reasons.md
Lines 483-485 (patched)
<https://reviews.apache.org/r/64250/#comment270744>
I think we can just focus on:
- This is sent when an agent previously marked as unreachable re-registers.
- Status updates with this reason are not the original ones, see the comments for `REASON_RECONCILIATION`.
include/mesos/mesos.proto
Lines 2406 (patched)
<https://reviews.apache.org/r/64250/#comment270736>
I think has convention used here is to order the reasons alphabetically.
include/mesos/v1/mesos.proto
Lines 2387 (patched)
<https://reviews.apache.org/r/64250/#comment270737>
Ditto.
- Jiang Yan Xu
On Dec. 1, 2017, 8:20 a.m., Megha Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64250/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2017, 8:20 a.m.)
>
>
> Review request for mesos, Ilya Pronin, James Peach, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new reasons `REASON_SLAVE_REREGISTERED` and
> `REASON_AGENT_REREGISTERED` for v0 and v1 task status update.
> The new reason will be used when master starts to send status
> update during the re-registeration of an agent which was
> previosuly removed by the master because of being unreachable.
>
>
> Diffs
> -----
>
> docs/task-state-reasons.md 07f7927e28fe5624a6c9b74de731483aaa729ca3
> include/mesos/mesos.proto 11089b7d6f48f001ae64e8b0c67a2732684aa2fa
> include/mesos/v1/mesos.proto c496da927903f472c78d455b7fdf886d8fb692c4
>
>
> Diff: https://reviews.apache.org/r/64250/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Megha Sharma
>
>
Re: Review Request 64250: Added new reasons task status update.
Posted by Ilya Pronin <ip...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64250/#review192600
-----------------------------------------------------------
Ship it!
Ship It!
- Ilya Pronin
On Dec. 1, 2017, 1:06 p.m., Megha Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64250/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2017, 1:06 p.m.)
>
>
> Review request for mesos, Ilya Pronin, James Peach, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new reasons `REASON_SLAVE_REREGISTERED` and
> `REASON_AGENT_REREGISTERED` for v0 and v1 task status update.
> The new reason will be used when master starts to send status
> update during the re-registration of an agent which was
> previously removed by the master because of being unreachable
> or is unknown to the master due to the garbage collection of
> the unreachable and gone agents in the registry.
>
>
> Diffs
> -----
>
> docs/task-state-reasons.md 07f7927e28fe5624a6c9b74de731483aaa729ca3
> include/mesos/mesos.proto 11089b7d6f48f001ae64e8b0c67a2732684aa2fa
> include/mesos/v1/mesos.proto c496da927903f472c78d455b7fdf886d8fb692c4
>
>
> Diff: https://reviews.apache.org/r/64250/diff/5/
>
>
> Testing
> -------
>
> code changes verified with make check and the documents changes with markdown viewer
>
>
> Thanks,
>
> Megha Sharma
>
>
Re: Review Request 64250: Added new reasons task status update.
Posted by Megha Sharma <ms...@apple.com>.
> On Dec. 1, 2017, 9:55 p.m., Ilya Pronin wrote:
> > docs/task-state-reasons.md
> > Lines 474-477 (patched)
> > <https://reviews.apache.org/r/64250/diff/2/?file=1906002#file1906002line474>
> >
> > I don't quite follow this note. A modified copy of which update? Should we just say that these updates reflect the states of the tasks reported by the agent upon its re-registration?
>
> Megha Sharma wrote:
> Here, we are saying that master takes the actual state reported by the agent and enriches it with reason and message before sending to the framework. But I am open to changing it if you or Yan feel it doesn't convery the idea.
>
> Jiang Yan Xu wrote:
> I was suggesting it because this status is generated by the master the same way they are generated upon reconciliation.
>
> For `REASON_RECONCILIATION` in the same doc there is this note:
>
> ```
> Note: Status updates with this reason are not the original ones, but rather a modified copy that is re-sent from the master. In particular, the original data and message fields are erased and the original reason field is overwritten by REASON_RECONCILIATION .
> ```
>
> It is the same for `REASON_SLAVE_REREGISTERED`.
>
>
> Megha I just noticed that you didn't mention the erasure of `data` and `message` fields, but perhaps this is not worth repeating. I was originally suggesting refering to `REASON_RECONCILIATION` for details: something like: `Status updates with this reason are a modified copies re-sent by the master. See comments for REASON_RECONCILIATION.`
>
> Would this be clearer?
>
> "reflect the states of the tasks reported by the agent upon its re-registration" this sentence is good too.
>
> Ilya Pronin wrote:
> Oh, now I got where this came from :) Yeah, it would be clearer if there was a mention that new states come from the agent re-registration message and a reference to `REASON_RECONCILIATION` description. Or it should be enough to jsut say that these are produced by the master and reflect the states reported by the agent during re-registration.
PTAL, updated the description based on the discussion.
- Megha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64250/#review192567
-----------------------------------------------------------
On Dec. 1, 2017, 9:06 p.m., Megha Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64250/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2017, 9:06 p.m.)
>
>
> Review request for mesos, Ilya Pronin, James Peach, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new reasons `REASON_SLAVE_REREGISTERED` and
> `REASON_AGENT_REREGISTERED` for v0 and v1 task status update.
> The new reason will be used when master starts to send status
> update during the re-registration of an agent which was
> previously removed by the master because of being unreachable
> or is unknown to the master due to the garbage collection of
> the unreachable and gone agents in the registry.
>
>
> Diffs
> -----
>
> docs/task-state-reasons.md 07f7927e28fe5624a6c9b74de731483aaa729ca3
> include/mesos/mesos.proto 11089b7d6f48f001ae64e8b0c67a2732684aa2fa
> include/mesos/v1/mesos.proto c496da927903f472c78d455b7fdf886d8fb692c4
>
>
> Diff: https://reviews.apache.org/r/64250/diff/5/
>
>
> Testing
> -------
>
> code changes verified with make check and the documents changes with markdown viewer
>
>
> Thanks,
>
> Megha Sharma
>
>
Re: Review Request 64250: Added new reasons task status update.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Dec. 1, 2017, 1:55 p.m., Ilya Pronin wrote:
> > docs/task-state-reasons.md
> > Lines 474-477 (patched)
> > <https://reviews.apache.org/r/64250/diff/2/?file=1906002#file1906002line474>
> >
> > I don't quite follow this note. A modified copy of which update? Should we just say that these updates reflect the states of the tasks reported by the agent upon its re-registration?
>
> Megha Sharma wrote:
> Here, we are saying that master takes the actual state reported by the agent and enriches it with reason and message before sending to the framework. But I am open to changing it if you or Yan feel it doesn't convery the idea.
I was suggesting it because this status is generated by the master the same way they are generated upon reconciliation.
For `REASON_RECONCILIATION` in the same doc there is this note:
```
Note: Status updates with this reason are not the original ones, but rather a modified copy that is re-sent from the master. In particular, the original data and message fields are erased and the original reason field is overwritten by REASON_RECONCILIATION .
```
It is the same for `REASON_SLAVE_REREGISTERED`.
Megha I just noticed that you didn't mention the erasure of `data` and `message` fields, but perhaps this is not worth repeating. I was originally suggesting refering to `REASON_RECONCILIATION` for details: something like: `Status updates with this reason are a modified copies re-sent by the master. See comments for REASON_RECONCILIATION.`
Would this be clearer?
"reflect the states of the tasks reported by the agent upon its re-registration" this sentence is good too.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64250/#review192567
-----------------------------------------------------------
On Dec. 1, 2017, 1:06 p.m., Megha Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64250/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2017, 1:06 p.m.)
>
>
> Review request for mesos, Ilya Pronin, James Peach, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new reasons `REASON_SLAVE_REREGISTERED` and
> `REASON_AGENT_REREGISTERED` for v0 and v1 task status update.
> The new reason will be used when master starts to send status
> update during the re-registration of an agent which was
> previously removed by the master because of being unreachable
> or is unknown to the master due to the garbage collection of
> the unreachable and gone agents in the registry.
>
>
> Diffs
> -----
>
> docs/task-state-reasons.md 07f7927e28fe5624a6c9b74de731483aaa729ca3
> include/mesos/mesos.proto 11089b7d6f48f001ae64e8b0c67a2732684aa2fa
> include/mesos/v1/mesos.proto c496da927903f472c78d455b7fdf886d8fb692c4
>
>
> Diff: https://reviews.apache.org/r/64250/diff/4/
>
>
> Testing
> -------
>
> code changes verified with make check and the documents changes with markdown viewer
>
>
> Thanks,
>
> Megha Sharma
>
>
Re: Review Request 64250: Added new reasons task status update.
Posted by Ilya Pronin <ip...@twopensource.com>.
> On Dec. 1, 2017, 1:55 p.m., Ilya Pronin wrote:
> > docs/task-state-reasons.md
> > Lines 474-477 (patched)
> > <https://reviews.apache.org/r/64250/diff/2/?file=1906002#file1906002line474>
> >
> > I don't quite follow this note. A modified copy of which update? Should we just say that these updates reflect the states of the tasks reported by the agent upon its re-registration?
>
> Megha Sharma wrote:
> Here, we are saying that master takes the actual state reported by the agent and enriches it with reason and message before sending to the framework. But I am open to changing it if you or Yan feel it doesn't convery the idea.
>
> Jiang Yan Xu wrote:
> I was suggesting it because this status is generated by the master the same way they are generated upon reconciliation.
>
> For `REASON_RECONCILIATION` in the same doc there is this note:
>
> ```
> Note: Status updates with this reason are not the original ones, but rather a modified copy that is re-sent from the master. In particular, the original data and message fields are erased and the original reason field is overwritten by REASON_RECONCILIATION .
> ```
>
> It is the same for `REASON_SLAVE_REREGISTERED`.
>
>
> Megha I just noticed that you didn't mention the erasure of `data` and `message` fields, but perhaps this is not worth repeating. I was originally suggesting refering to `REASON_RECONCILIATION` for details: something like: `Status updates with this reason are a modified copies re-sent by the master. See comments for REASON_RECONCILIATION.`
>
> Would this be clearer?
>
> "reflect the states of the tasks reported by the agent upon its re-registration" this sentence is good too.
Oh, now I got where this came from :) Yeah, it would be clearer if there was a mention that new states come from the agent re-registration message and a reference to `REASON_RECONCILIATION` description. Or it should be enough to jsut say that these are produced by the master and reflect the states reported by the agent during re-registration.
- Ilya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64250/#review192567
-----------------------------------------------------------
On Dec. 1, 2017, 1:06 p.m., Megha Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64250/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2017, 1:06 p.m.)
>
>
> Review request for mesos, Ilya Pronin, James Peach, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new reasons `REASON_SLAVE_REREGISTERED` and
> `REASON_AGENT_REREGISTERED` for v0 and v1 task status update.
> The new reason will be used when master starts to send status
> update during the re-registration of an agent which was
> previously removed by the master because of being unreachable
> or is unknown to the master due to the garbage collection of
> the unreachable and gone agents in the registry.
>
>
> Diffs
> -----
>
> docs/task-state-reasons.md 07f7927e28fe5624a6c9b74de731483aaa729ca3
> include/mesos/mesos.proto 11089b7d6f48f001ae64e8b0c67a2732684aa2fa
> include/mesos/v1/mesos.proto c496da927903f472c78d455b7fdf886d8fb692c4
>
>
> Diff: https://reviews.apache.org/r/64250/diff/4/
>
>
> Testing
> -------
>
> code changes verified with make check and the documents changes with markdown viewer
>
>
> Thanks,
>
> Megha Sharma
>
>
Re: Review Request 64250: Added new reasons task status update.
Posted by Ilya Pronin <ip...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64250/#review192567
-----------------------------------------------------------
Looks good. I just have a small suggestion about the doc.
docs/task-state-reasons.md
Lines 474-477 (patched)
<https://reviews.apache.org/r/64250/#comment270811>
I don't quite follow this note. A modified copy of which update? Should we just say that these updates reflect the states of the tasks reported by the agent upon its re-registration?
- Ilya Pronin
On Dec. 1, 2017, 1:06 p.m., Megha Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64250/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2017, 1:06 p.m.)
>
>
> Review request for mesos, Ilya Pronin, James Peach, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new reasons `REASON_SLAVE_REREGISTERED` and
> `REASON_AGENT_REREGISTERED` for v0 and v1 task status update.
> The new reason will be used when master starts to send status
> update during the re-registeration of an agent which was
> previosuly removed by the master because of being unreachable.
>
>
> Diffs
> -----
>
> docs/task-state-reasons.md 07f7927e28fe5624a6c9b74de731483aaa729ca3
> include/mesos/mesos.proto 11089b7d6f48f001ae64e8b0c67a2732684aa2fa
> include/mesos/v1/mesos.proto c496da927903f472c78d455b7fdf886d8fb692c4
>
>
> Diff: https://reviews.apache.org/r/64250/diff/2/
>
>
> Testing
> -------
>
> with make check
>
>
> Thanks,
>
> Megha Sharma
>
>
Re: Review Request 64250: Added new reasons task status update.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64250/#review192687
-----------------------------------------------------------
docs/task-state-reasons.md
Line 469 (original), 469-470 (patched)
<https://reviews.apache.org/r/64250/#comment270935>
Please add extra newline for consistency with the rest of the doc.
docs/task-state-reasons.md
Line 470 (original), 481-482 (patched)
<https://reviews.apache.org/r/64250/#comment270936>
Please put these onto the same line for consistency.
- Alexander Rukletsov
On Dec. 1, 2017, 9:06 p.m., Megha Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64250/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2017, 9:06 p.m.)
>
>
> Review request for mesos, Ilya Pronin, James Peach, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new reasons `REASON_SLAVE_REREGISTERED` and
> `REASON_AGENT_REREGISTERED` for v0 and v1 task status update.
> The new reason will be used when master starts to send status
> update during the re-registration of an agent which was
> previously removed by the master because of being unreachable
> or is unknown to the master due to the garbage collection of
> the unreachable and gone agents in the registry.
>
>
> Diffs
> -----
>
> docs/task-state-reasons.md 07f7927e28fe5624a6c9b74de731483aaa729ca3
> include/mesos/mesos.proto 11089b7d6f48f001ae64e8b0c67a2732684aa2fa
> include/mesos/v1/mesos.proto c496da927903f472c78d455b7fdf886d8fb692c4
>
>
> Diff: https://reviews.apache.org/r/64250/diff/5/
>
>
> Testing
> -------
>
> code changes verified with make check and the documents changes with markdown viewer
>
>
> Thanks,
>
> Megha Sharma
>
>
Re: Review Request 64250: Added new reasons task status update.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64250/#review192563
-----------------------------------------------------------
Fix it, then Ship it!
docs/task-state-reasons.md
Lines 476 (patched)
<https://reviews.apache.org/r/64250/#comment270801>
What is the `/written` part about? `overwritten` covers all cases?
docs/task-state-reasons.md
Lines 477 (patched)
<https://reviews.apache.org/r/64250/#comment270805>
Given the discussion in /r/64098/, here perhaps add another `Note: Due to garbage collection of the unreachable and gone agents in the registry, Mesos also sends such status updates for agents unknown to the master`.
docs/task-state-reasons.md
Lines 478 (patched)
<https://reviews.apache.org/r/64250/#comment270804>
Should there be a `</td></tr>`?
Also when it involves doc changes, could you in the testing done section mention that you have used a markdown viewer to review the doc change?
- Jiang Yan Xu
On Dec. 1, 2017, 1:06 p.m., Megha Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64250/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2017, 1:06 p.m.)
>
>
> Review request for mesos, Ilya Pronin, James Peach, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new reasons `REASON_SLAVE_REREGISTERED` and
> `REASON_AGENT_REREGISTERED` for v0 and v1 task status update.
> The new reason will be used when master starts to send status
> update during the re-registeration of an agent which was
> previosuly removed by the master because of being unreachable.
>
>
> Diffs
> -----
>
> docs/task-state-reasons.md 07f7927e28fe5624a6c9b74de731483aaa729ca3
> include/mesos/mesos.proto 11089b7d6f48f001ae64e8b0c67a2732684aa2fa
> include/mesos/v1/mesos.proto c496da927903f472c78d455b7fdf886d8fb692c4
>
>
> Diff: https://reviews.apache.org/r/64250/diff/2/
>
>
> Testing
> -------
>
> with make check
>
>
> Thanks,
>
> Megha Sharma
>
>
Re: Review Request 64250: Added new reasons task status update.
Posted by Megha Sharma <ms...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64250/
-----------------------------------------------------------
(Updated Dec. 1, 2017, 9:06 p.m.)
Review request for mesos, Ilya Pronin, James Peach, and Jiang Yan Xu.
Changes
-------
done testing
Repository: mesos
Description
-------
Added new reasons `REASON_SLAVE_REREGISTERED` and
`REASON_AGENT_REREGISTERED` for v0 and v1 task status update.
The new reason will be used when master starts to send status
update during the re-registeration of an agent which was
previosuly removed by the master because of being unreachable.
Diffs
-----
docs/task-state-reasons.md 07f7927e28fe5624a6c9b74de731483aaa729ca3
include/mesos/mesos.proto 11089b7d6f48f001ae64e8b0c67a2732684aa2fa
include/mesos/v1/mesos.proto c496da927903f472c78d455b7fdf886d8fb692c4
Diff: https://reviews.apache.org/r/64250/diff/2/
Testing (updated)
-------
with make check
Thanks,
Megha Sharma