You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2017/09/22 23:22:44 UTC
Review Request 62518: Fixed default executor handling of nested
container status.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62518/
-----------------------------------------------------------
Review request for mesos, Andrei Budnik and Anand Mazumdar.
Repository: mesos
Description
-------
The default executor was not handling a missing nested container
exit status correctly. It was assuming the protobuf accessor was
returning an Option rather than explicitly checking whether the
`exit_status` field was present in the message.
Added the explicit check for the `exit_status` field, and always
propagated an appropriate message into the status update, even
when the `exit_status` is absent. Added some documentation of
the `exit_status` field to the protobuf definition files.
Diffs
-----
include/mesos/agent/agent.proto 7c8c8a7d8298e91e4e002327b3b27d4c74b5cbae
include/mesos/v1/agent/agent.proto 3e199124b23fa027232790d99370fe2f33660096
src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0
src/python/cli_new/lib/cli/plugins/base.py e01a7b2bc4d4cbabe706c8926913f43d2b4cf69c
src/python/cli_new/lib/cli/tests/base.py 33c67c1161d9c9fd57aa5f44b04746e3740e1dd9
src/python/cli_new/lib/cli/tests/constants.py 1398701b167eae397730afc5b1fab6a21e723266
src/python/pylint.config c76036441784543eb9025d0bd562a5628c4bad4e
support/mesos-style.py 9cd1623c48623e558dcd5b80b4bbf5a2162c57cb
Diff: https://reviews.apache.org/r/62518/diff/1/
Testing
-------
make check (Fedora 26)
Thanks,
James Peach
Re: Review Request 62518: Fixed default executor handling of nested
container status.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62518/#review186070
-----------------------------------------------------------
Patch looks great!
Reviews applied: [62518]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Sept. 22, 2017, 11:22 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62518/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2017, 11:22 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Anand Mazumdar.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The default executor was not handling a missing nested container
> exit status correctly. It was assuming the protobuf accessor was
> returning an Option rather than explicitly checking whether the
> `exit_status` field was present in the message.
>
> Added the explicit check for the `exit_status` field, and always
> propagated an appropriate message into the status update, even
> when the `exit_status` is absent. Added some documentation of
> the `exit_status` field to the protobuf definition files.
>
>
> Diffs
> -----
>
> include/mesos/agent/agent.proto 7c8c8a7d8298e91e4e002327b3b27d4c74b5cbae
> include/mesos/v1/agent/agent.proto 3e199124b23fa027232790d99370fe2f33660096
> src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0
> src/python/cli_new/lib/cli/plugins/base.py e01a7b2bc4d4cbabe706c8926913f43d2b4cf69c
> src/python/cli_new/lib/cli/tests/base.py 33c67c1161d9c9fd57aa5f44b04746e3740e1dd9
> src/python/cli_new/lib/cli/tests/constants.py 1398701b167eae397730afc5b1fab6a21e723266
> src/python/pylint.config c76036441784543eb9025d0bd562a5628c4bad4e
> support/mesos-style.py 9cd1623c48623e558dcd5b80b4bbf5a2162c57cb
>
>
> Diff: https://reviews.apache.org/r/62518/diff/1/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 62518: Fixed default executor handling of nested
container status.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62518/#review186388
-----------------------------------------------------------
Ship it!
Ship It!
- Qian Zhang
On Sept. 25, 2017, 11:21 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62518/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2017, 11:21 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Anand Mazumdar.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The default executor was not handling a missing nested container
> exit status correctly. It was assuming the protobuf accessor was
> returning an Option rather than explicitly checking whether the
> `exit_status` field was present in the message.
>
> Added the explicit check for the `exit_status` field, and always
> propagated an appropriate message into the status update, even
> when the `exit_status` is absent. Added some documentation of
> the `exit_status` field to the protobuf definition files.
>
>
> Diffs
> -----
>
> include/mesos/agent/agent.proto 7c8c8a7d8298e91e4e002327b3b27d4c74b5cbae
> include/mesos/v1/agent/agent.proto 3e199124b23fa027232790d99370fe2f33660096
> src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0
>
>
> Diff: https://reviews.apache.org/r/62518/diff/4/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 62518: Fixed default executor handling of nested
container status.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62518/#review186134
-----------------------------------------------------------
Patch looks great!
Reviews applied: [62518]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Sept. 25, 2017, 8:51 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62518/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2017, 8:51 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Anand Mazumdar.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The default executor was not handling a missing nested container
> exit status correctly. It was assuming the protobuf accessor was
> returning an Option rather than explicitly checking whether the
> `exit_status` field was present in the message.
>
> Added the explicit check for the `exit_status` field, and always
> propagated an appropriate message into the status update, even
> when the `exit_status` is absent. Added some documentation of
> the `exit_status` field to the protobuf definition files.
>
>
> Diffs
> -----
>
> include/mesos/agent/agent.proto 7c8c8a7d8298e91e4e002327b3b27d4c74b5cbae
> include/mesos/v1/agent/agent.proto 3e199124b23fa027232790d99370fe2f33660096
> src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0
>
>
> Diff: https://reviews.apache.org/r/62518/diff/4/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 62518: Fixed default executor handling of nested
container status.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62518/#review186139
-----------------------------------------------------------
PASS: Mesos patch 62518 was successfully built and tested.
Reviews applied: `['62518']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62518
- Mesos Reviewbot Windows
On Sept. 25, 2017, 3:21 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62518/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2017, 3:21 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Anand Mazumdar.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The default executor was not handling a missing nested container
> exit status correctly. It was assuming the protobuf accessor was
> returning an Option rather than explicitly checking whether the
> `exit_status` field was present in the message.
>
> Added the explicit check for the `exit_status` field, and always
> propagated an appropriate message into the status update, even
> when the `exit_status` is absent. Added some documentation of
> the `exit_status` field to the protobuf definition files.
>
>
> Diffs
> -----
>
> include/mesos/agent/agent.proto 7c8c8a7d8298e91e4e002327b3b27d4c74b5cbae
> include/mesos/v1/agent/agent.proto 3e199124b23fa027232790d99370fe2f33660096
> src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0
>
>
> Diff: https://reviews.apache.org/r/62518/diff/4/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 62518: Fixed default executor handling of nested
container status.
Posted by Andrei Budnik <ab...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62518/#review186553
-----------------------------------------------------------
Ship it!
Ship It!
- Andrei Budnik
On Sept. 25, 2017, 3:21 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62518/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2017, 3:21 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Anand Mazumdar.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The default executor was not handling a missing nested container
> exit status correctly. It was assuming the protobuf accessor was
> returning an Option rather than explicitly checking whether the
> `exit_status` field was present in the message.
>
> Added the explicit check for the `exit_status` field, and always
> propagated an appropriate message into the status update, even
> when the `exit_status` is absent. Added some documentation of
> the `exit_status` field to the protobuf definition files.
>
>
> Diffs
> -----
>
> include/mesos/agent/agent.proto 7c8c8a7d8298e91e4e002327b3b27d4c74b5cbae
> include/mesos/v1/agent/agent.proto 3e199124b23fa027232790d99370fe2f33660096
> src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0
>
>
> Diff: https://reviews.apache.org/r/62518/diff/4/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 62518: Fixed default executor handling of nested
container status.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62518/
-----------------------------------------------------------
(Updated Oct. 12, 2017, 3:57 p.m.)
Review request for mesos, Andrei Budnik and Anand Mazumdar.
Bugs: MESOS-8080
https://issues.apache.org/jira/browse/MESOS-8080
Repository: mesos
Description
-------
The default executor was not handling a missing nested container
exit status correctly. It was assuming the protobuf accessor was
returning an Option rather than explicitly checking whether the
`exit_status` field was present in the message.
Added the explicit check for the `exit_status` field, and always
propagated an appropriate message into the status update, even
when the `exit_status` is absent. Added some documentation of
the `exit_status` field to the protobuf definition files.
Diffs
-----
include/mesos/agent/agent.proto 7c8c8a7d8298e91e4e002327b3b27d4c74b5cbae
include/mesos/v1/agent/agent.proto 3e199124b23fa027232790d99370fe2f33660096
src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0
Diff: https://reviews.apache.org/r/62518/diff/4/
Testing
-------
make check (Fedora 26)
Thanks,
James Peach
Re: Review Request 62518: Fixed default executor handling of nested
container status.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62518/
-----------------------------------------------------------
(Updated Sept. 25, 2017, 3:21 p.m.)
Review request for mesos, Andrei Budnik and Anand Mazumdar.
Repository: mesos
Description
-------
The default executor was not handling a missing nested container
exit status correctly. It was assuming the protobuf accessor was
returning an Option rather than explicitly checking whether the
`exit_status` field was present in the message.
Added the explicit check for the `exit_status` field, and always
propagated an appropriate message into the status update, even
when the `exit_status` is absent. Added some documentation of
the `exit_status` field to the protobuf definition files.
Diffs (updated)
-----
include/mesos/agent/agent.proto 7c8c8a7d8298e91e4e002327b3b27d4c74b5cbae
include/mesos/v1/agent/agent.proto 3e199124b23fa027232790d99370fe2f33660096
src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0
Diff: https://reviews.apache.org/r/62518/diff/4/
Changes: https://reviews.apache.org/r/62518/diff/3-4/
Testing
-------
make check (Fedora 26)
Thanks,
James Peach
Re: Review Request 62518: Fixed default executor handling of nested
container status.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62518/#review186043
-----------------------------------------------------------
PASS: Mesos patch 62518 was successfully built and tested.
Reviews applied: `['62518']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62518
- Mesos Reviewbot Windows
On Sept. 22, 2017, 11:22 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62518/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2017, 11:22 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Anand Mazumdar.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The default executor was not handling a missing nested container
> exit status correctly. It was assuming the protobuf accessor was
> returning an Option rather than explicitly checking whether the
> `exit_status` field was present in the message.
>
> Added the explicit check for the `exit_status` field, and always
> propagated an appropriate message into the status update, even
> when the `exit_status` is absent. Added some documentation of
> the `exit_status` field to the protobuf definition files.
>
>
> Diffs
> -----
>
> include/mesos/agent/agent.proto 7c8c8a7d8298e91e4e002327b3b27d4c74b5cbae
> include/mesos/v1/agent/agent.proto 3e199124b23fa027232790d99370fe2f33660096
> src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0
> src/python/cli_new/lib/cli/plugins/base.py e01a7b2bc4d4cbabe706c8926913f43d2b4cf69c
> src/python/cli_new/lib/cli/tests/base.py 33c67c1161d9c9fd57aa5f44b04746e3740e1dd9
> src/python/cli_new/lib/cli/tests/constants.py 1398701b167eae397730afc5b1fab6a21e723266
> src/python/pylint.config c76036441784543eb9025d0bd562a5628c4bad4e
> support/mesos-style.py 9cd1623c48623e558dcd5b80b4bbf5a2162c57cb
>
>
> Diff: https://reviews.apache.org/r/62518/diff/1/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 62518: Fixed default executor handling of nested
container status.
Posted by James Peach <jp...@apache.org>.
> On Sept. 25, 2017, 12:42 a.m., Qian Zhang wrote:
> > Why do we need to change those .py files? I do not see you mentioned it in the commit message.
AFAICT `post-reviews` gets confused if you `git fetch` without doing a `git pull` and you end up including spurious changes. Removed them.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62518/#review186081
-----------------------------------------------------------
On Sept. 22, 2017, 11:22 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62518/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2017, 11:22 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Anand Mazumdar.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The default executor was not handling a missing nested container
> exit status correctly. It was assuming the protobuf accessor was
> returning an Option rather than explicitly checking whether the
> `exit_status` field was present in the message.
>
> Added the explicit check for the `exit_status` field, and always
> propagated an appropriate message into the status update, even
> when the `exit_status` is absent. Added some documentation of
> the `exit_status` field to the protobuf definition files.
>
>
> Diffs
> -----
>
> include/mesos/agent/agent.proto 7c8c8a7d8298e91e4e002327b3b27d4c74b5cbae
> include/mesos/v1/agent/agent.proto 3e199124b23fa027232790d99370fe2f33660096
> src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0
>
>
> Diff: https://reviews.apache.org/r/62518/diff/3/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 62518: Fixed default executor handling of nested
container status.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62518/#review186081
-----------------------------------------------------------
Why do we need to change those .py files? I do not see you mentioned it in the commit message.
include/mesos/agent/agent.proto
Lines 337 (patched)
<https://reviews.apache.org/r/62518/#comment262502>
Kill the "be".
include/mesos/v1/agent/agent.proto
Lines 337 (patched)
<https://reviews.apache.org/r/62518/#comment262503>
Dito.
src/launcher/default_executor.cpp
Line 822 (original), 820-824 (patched)
<https://reviews.apache.org/r/62518/#comment262504>
Can we change these code to the below? And then we do not need to set `taskState` to `TASK_FAILED` when declaring it in the line 800.
```
} else {
taskState = TASK_FAILED;
message = "Command terminated with unknown status";
}
```
- Qian Zhang
On Sept. 23, 2017, 7:22 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62518/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2017, 7:22 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Anand Mazumdar.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The default executor was not handling a missing nested container
> exit status correctly. It was assuming the protobuf accessor was
> returning an Option rather than explicitly checking whether the
> `exit_status` field was present in the message.
>
> Added the explicit check for the `exit_status` field, and always
> propagated an appropriate message into the status update, even
> when the `exit_status` is absent. Added some documentation of
> the `exit_status` field to the protobuf definition files.
>
>
> Diffs
> -----
>
> include/mesos/agent/agent.proto 7c8c8a7d8298e91e4e002327b3b27d4c74b5cbae
> include/mesos/v1/agent/agent.proto 3e199124b23fa027232790d99370fe2f33660096
> src/launcher/default_executor.cpp 136c000917b2c4fdaf68fd460764c3a15fdf3bf0
> src/python/cli_new/lib/cli/plugins/base.py e01a7b2bc4d4cbabe706c8926913f43d2b4cf69c
> src/python/cli_new/lib/cli/tests/base.py 33c67c1161d9c9fd57aa5f44b04746e3740e1dd9
> src/python/cli_new/lib/cli/tests/constants.py 1398701b167eae397730afc5b1fab6a21e723266
> src/python/pylint.config c76036441784543eb9025d0bd562a5628c4bad4e
> support/mesos-style.py 9cd1623c48623e558dcd5b80b4bbf5a2162c57cb
>
>
> Diff: https://reviews.apache.org/r/62518/diff/1/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>