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/01/18 18:25:48 UTC
Review Request 55683: Rationalize process wait error checking.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55683/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, Joris Van Remoortere, Michael Park, and Jiang Yan Xu.
Bugs: MESOS-6946
https://issues.apache.org/jira/browse/MESOS-6946
Repository: mesos
Description
-------
Introduce the WSUCCESS() API to test that a forked process exited
successfully. Use it in all the places that were performing this test
bespokely, and update error logs to user WSTRINGIFY() if appropriate.
Diffs
-----
src/common/status_utils.hpp 26b94d49973dd61a6138e6a64d5ec0bdec6dea7e
src/docker/executor.cpp 9b5c469e2d0f33e228ec746711e6bc6ed352cbc7
src/hdfs/hdfs.cpp bebba8bb3c16f40293d21b5e00c353135a0f8f26
src/launcher/default_executor.cpp 57e4799e750f8f5352a9fec58af40efe432ea865
src/launcher/executor.cpp e035a4ee4438a3342f68a5548a5fd8d57a315cfa
src/launcher/fetcher.cpp 5f9a38d3ae8fb9b4b21203f96d53c5bef0909eba
src/linux/ns.hpp da43266ba67f76c9ca856584a540794062097691
src/slave/containerizer/docker.cpp 7a8a7271b54af0b4dcdae7a2aa8a90a8a7d05fd6
src/slave/containerizer/mesos/io/switchboard.cpp 2c4eed8f2dc7f91b9fd08f91f1b08484fca9a9af
src/slave/containerizer/mesos/launch.cpp e482ab8bdfc358f695b87cda72ca59fb64cd8c4d
src/tests/script.cpp ef6b22aaeb056ed489f2c519e1a02644f2653329
Diff: https://reviews.apache.org/r/55683/diff/
Testing
-------
make check && sudo make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 55683: Rationalize process wait error checking.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55683/#review162188
-----------------------------------------------------------
Patch looks great!
Reviews applied: [55683]
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 Jan. 18, 2017, 6:25 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55683/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2017, 6:25 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Joris Van Remoortere, Michael Park, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6946
> https://issues.apache.org/jira/browse/MESOS-6946
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduce the WSUCCESS() API to test that a forked process exited
> successfully. Use it in all the places that were performing this test
> bespokely, and update error logs to user WSTRINGIFY() if appropriate.
>
>
> Diffs
> -----
>
> src/common/status_utils.hpp 26b94d49973dd61a6138e6a64d5ec0bdec6dea7e
> src/docker/executor.cpp 9b5c469e2d0f33e228ec746711e6bc6ed352cbc7
> src/hdfs/hdfs.cpp bebba8bb3c16f40293d21b5e00c353135a0f8f26
> src/launcher/default_executor.cpp 57e4799e750f8f5352a9fec58af40efe432ea865
> src/launcher/executor.cpp e035a4ee4438a3342f68a5548a5fd8d57a315cfa
> src/launcher/fetcher.cpp 5f9a38d3ae8fb9b4b21203f96d53c5bef0909eba
> src/linux/ns.hpp da43266ba67f76c9ca856584a540794062097691
> src/slave/containerizer/docker.cpp 7a8a7271b54af0b4dcdae7a2aa8a90a8a7d05fd6
> src/slave/containerizer/mesos/io/switchboard.cpp 2c4eed8f2dc7f91b9fd08f91f1b08484fca9a9af
> src/slave/containerizer/mesos/launch.cpp e482ab8bdfc358f695b87cda72ca59fb64cd8c4d
> src/tests/script.cpp ef6b22aaeb056ed489f2c519e1a02644f2653329
>
> Diff: https://reviews.apache.org/r/55683/diff/
>
>
> Testing
> -------
>
> make check && sudo make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 55683: Rationalize process wait error checking.
Posted by James Peach <jp...@apache.org>.
> On Jan. 19, 2017, 12:43 a.m., Jiang Yan Xu wrote:
> > src/launcher/fetcher.cpp, line 114
> > <https://reviews.apache.org/r/55683/diff/1/?file=1607831#file1607831line114>
> >
> > It's worth noting that
> >
> > ```
> > !WSUCCESS()
> > ```
> >
> > translates to
> >
> > ```
> > !WIFEXITED(status) || WEXITSTATUS(status) != 0
> > ```
> >
> > so it's not exactly the same as
> >
> >
> > ```
> > WIFEXITED(status) && WEXITSTATUS(status) != 0
> > ```
> >
> > But the result seems to be more correct than the previous: previously if the subprocess is signaled, the fetcher would return `true`.
> >
> > This is then a bug fix which we could note in the description.
Right :)
> On Jan. 19, 2017, 12:43 a.m., Jiang Yan Xu wrote:
> > src/common/status_utils.hpp, line 26
> > <https://reviews.apache.org/r/55683/diff/1/?file=1607826#file1607826line26>
> >
> > Use an adjective? `WSUCCESSFUL` for a boolean concept?
Agreed to rename it to `WSUCCEEDED`.
> On Jan. 19, 2017, 12:43 a.m., Jiang Yan Xu wrote:
> > src/common/status_utils.hpp, line 28
> > <https://reviews.apache.org/r/55683/diff/1/?file=1607826#file1607826line28>
> >
> > I guess the same `#ifdef __WINDOWS__` guards apply?
Windows folks think this is probably OK.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55683/#review162196
-----------------------------------------------------------
On Jan. 18, 2017, 6:25 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55683/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2017, 6:25 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Joris Van Remoortere, Michael Park, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6946
> https://issues.apache.org/jira/browse/MESOS-6946
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduce the WSUCCESS() API to test that a forked process exited
> successfully. Use it in all the places that were performing this test
> bespokely, and update error logs to user WSTRINGIFY() if appropriate.
>
>
> Diffs
> -----
>
> src/common/status_utils.hpp 26b94d49973dd61a6138e6a64d5ec0bdec6dea7e
> src/docker/executor.cpp 9b5c469e2d0f33e228ec746711e6bc6ed352cbc7
> src/hdfs/hdfs.cpp bebba8bb3c16f40293d21b5e00c353135a0f8f26
> src/launcher/default_executor.cpp 57e4799e750f8f5352a9fec58af40efe432ea865
> src/launcher/executor.cpp e035a4ee4438a3342f68a5548a5fd8d57a315cfa
> src/launcher/fetcher.cpp 5f9a38d3ae8fb9b4b21203f96d53c5bef0909eba
> src/linux/ns.hpp da43266ba67f76c9ca856584a540794062097691
> src/slave/containerizer/docker.cpp 7a8a7271b54af0b4dcdae7a2aa8a90a8a7d05fd6
> src/slave/containerizer/mesos/io/switchboard.cpp 2c4eed8f2dc7f91b9fd08f91f1b08484fca9a9af
> src/slave/containerizer/mesos/launch.cpp e482ab8bdfc358f695b87cda72ca59fb64cd8c4d
> src/tests/script.cpp ef6b22aaeb056ed489f2c519e1a02644f2653329
>
> Diff: https://reviews.apache.org/r/55683/diff/
>
>
> Testing
> -------
>
> make check && sudo make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 55683: Rationalize process wait error checking.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55683/#review162196
-----------------------------------------------------------
Fix it, then Ship it!
src/common/status_utils.hpp (line 26)
<https://reviews.apache.org/r/55683/#comment233457>
Use an adjective? `WSUCCESSFUL` for a boolean concept?
src/common/status_utils.hpp (line 28)
<https://reviews.apache.org/r/55683/#comment233475>
I guess the same `#ifdef __WINDOWS__` guards apply?
src/launcher/fetcher.cpp (line 114)
<https://reviews.apache.org/r/55683/#comment233482>
It's worth noting that
```
!WSUCCESS()
```
translates to
```
!WIFEXITED(status) || WEXITSTATUS(status) != 0
```
so it's not exactly the same as
```
WIFEXITED(status) && WEXITSTATUS(status) != 0
```
But the result seems to be more correct than the previous: previously if the subprocess is signaled, the fetcher would return `true`.
This is then a bug fix which we could note in the description.
- Jiang Yan Xu
On Jan. 18, 2017, 10:25 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55683/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2017, 10:25 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Joris Van Remoortere, Michael Park, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6946
> https://issues.apache.org/jira/browse/MESOS-6946
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduce the WSUCCESS() API to test that a forked process exited
> successfully. Use it in all the places that were performing this test
> bespokely, and update error logs to user WSTRINGIFY() if appropriate.
>
>
> Diffs
> -----
>
> src/common/status_utils.hpp 26b94d49973dd61a6138e6a64d5ec0bdec6dea7e
> src/docker/executor.cpp 9b5c469e2d0f33e228ec746711e6bc6ed352cbc7
> src/hdfs/hdfs.cpp bebba8bb3c16f40293d21b5e00c353135a0f8f26
> src/launcher/default_executor.cpp 57e4799e750f8f5352a9fec58af40efe432ea865
> src/launcher/executor.cpp e035a4ee4438a3342f68a5548a5fd8d57a315cfa
> src/launcher/fetcher.cpp 5f9a38d3ae8fb9b4b21203f96d53c5bef0909eba
> src/linux/ns.hpp da43266ba67f76c9ca856584a540794062097691
> src/slave/containerizer/docker.cpp 7a8a7271b54af0b4dcdae7a2aa8a90a8a7d05fd6
> src/slave/containerizer/mesos/io/switchboard.cpp 2c4eed8f2dc7f91b9fd08f91f1b08484fca9a9af
> src/slave/containerizer/mesos/launch.cpp e482ab8bdfc358f695b87cda72ca59fb64cd8c4d
> src/tests/script.cpp ef6b22aaeb056ed489f2c519e1a02644f2653329
>
> Diff: https://reviews.apache.org/r/55683/diff/
>
>
> Testing
> -------
>
> make check && sudo make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 55683: Rationalize process wait error checking.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55683/#review162272
-----------------------------------------------------------
Patch looks great!
Reviews applied: [55683]
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 Jan. 19, 2017, 1:11 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55683/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2017, 1:11 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Joris Van Remoortere, Michael Park, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6946
> https://issues.apache.org/jira/browse/MESOS-6946
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduce the WSUCCEEDED() API to test that a forked process exited
> successfully. Use it in all the places that were performing this test
> bespokely, and update error logs to user WSTRINGIFY() if appropriate.
>
> This also fixes called that were checking the exit status but not the
> signaled status.
>
>
> Diffs
> -----
>
> src/common/status_utils.hpp 26b94d49973dd61a6138e6a64d5ec0bdec6dea7e
> src/docker/executor.cpp 9b5c469e2d0f33e228ec746711e6bc6ed352cbc7
> src/hdfs/hdfs.cpp bebba8bb3c16f40293d21b5e00c353135a0f8f26
> src/launcher/default_executor.cpp 57e4799e750f8f5352a9fec58af40efe432ea865
> src/launcher/executor.cpp e035a4ee4438a3342f68a5548a5fd8d57a315cfa
> src/launcher/fetcher.cpp 5f9a38d3ae8fb9b4b21203f96d53c5bef0909eba
> src/linux/ns.hpp da43266ba67f76c9ca856584a540794062097691
> src/slave/containerizer/docker.cpp 7a8a7271b54af0b4dcdae7a2aa8a90a8a7d05fd6
> src/slave/containerizer/mesos/io/switchboard.cpp 2c4eed8f2dc7f91b9fd08f91f1b08484fca9a9af
> src/slave/containerizer/mesos/launch.cpp e482ab8bdfc358f695b87cda72ca59fb64cd8c4d
> src/tests/script.cpp ef6b22aaeb056ed489f2c519e1a02644f2653329
>
> Diff: https://reviews.apache.org/r/55683/diff/
>
>
> Testing
> -------
>
> make check && sudo make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 55683: Rationalize process wait error checking.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55683/
-----------------------------------------------------------
(Updated Jan. 19, 2017, 1:11 a.m.)
Review request for mesos, Benjamin Bannier, Joris Van Remoortere, Michael Park, and Jiang Yan Xu.
Changes
-------
Rebase and address review feedback.
Bugs: MESOS-6946
https://issues.apache.org/jira/browse/MESOS-6946
Repository: mesos
Description (updated)
-------
Introduce the WSUCCEEDED() API to test that a forked process exited
successfully. Use it in all the places that were performing this test
bespokely, and update error logs to user WSTRINGIFY() if appropriate.
This also fixes called that were checking the exit status but not the
signaled status.
Diffs (updated)
-----
src/common/status_utils.hpp 26b94d49973dd61a6138e6a64d5ec0bdec6dea7e
src/docker/executor.cpp 9b5c469e2d0f33e228ec746711e6bc6ed352cbc7
src/hdfs/hdfs.cpp bebba8bb3c16f40293d21b5e00c353135a0f8f26
src/launcher/default_executor.cpp 57e4799e750f8f5352a9fec58af40efe432ea865
src/launcher/executor.cpp e035a4ee4438a3342f68a5548a5fd8d57a315cfa
src/launcher/fetcher.cpp 5f9a38d3ae8fb9b4b21203f96d53c5bef0909eba
src/linux/ns.hpp da43266ba67f76c9ca856584a540794062097691
src/slave/containerizer/docker.cpp 7a8a7271b54af0b4dcdae7a2aa8a90a8a7d05fd6
src/slave/containerizer/mesos/io/switchboard.cpp 2c4eed8f2dc7f91b9fd08f91f1b08484fca9a9af
src/slave/containerizer/mesos/launch.cpp e482ab8bdfc358f695b87cda72ca59fb64cd8c4d
src/tests/script.cpp ef6b22aaeb056ed489f2c519e1a02644f2653329
Diff: https://reviews.apache.org/r/55683/diff/
Testing
-------
make check && sudo make check (Fedora 25)
Thanks,
James Peach