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/27 22:20:02 UTC
Review Request 62638: Removed support for platforms without O_CLOEXEC.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62638/
-----------------------------------------------------------
Review request for mesos, Andrew Schwartzmeyer and Jie Yu.
Bugs: MESOS-8027
https://issues.apache.org/jira/browse/MESOS-8027
Repository: mesos
Description
-------
The `O_CLOEXEC` fallback code was broken since it did not guarantee
to include `fcntl.h` before checking for the `O_CLOEXEC` symbol.
O_CLOEXEC is supported by all reasonably current service platforms,
so we should not need to fall back to compatibility code. So rather
than fixing the fallback code, we can just eliminate it.
Diffs
-----
3rdparty/stout/include/stout/os/open.hpp c9346c62e01688f0f55811f7acbe63321b084355
3rdparty/stout/include/stout/os/windows/fcntl.hpp ac90bf08ccf5b594e70310e9843475502b3603a5
3rdparty/stout/include/stout/windows.hpp 1d865f8fd23aba0198017f0bf4be8471cfb714ed
src/master/quota.cpp 58bab6a678bac9e41a7994ba0b7cc1ed069a8a18
src/tests/master_quota_tests.cpp f9feb67aba3b8eb56190c9400a5a96f61dd94181
Diff: https://reviews.apache.org/r/62638/diff/1/
Testing
-------
make check (Fedora 26)
Manually verified that sandbox files opened through the webui get the `O_CLOEXEC` flag applied.
Thanks,
James Peach
Re: Review Request 62638: Removed support for platforms without
O_CLOEXEC.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62638/#review186954
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On Sept. 27, 2017, 10:20 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62638/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2017, 10:20 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, and Jie Yu.
>
>
> Bugs: MESOS-8027
> https://issues.apache.org/jira/browse/MESOS-8027
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `O_CLOEXEC` fallback code was broken since it did not guarantee
> to include `fcntl.h` before checking for the `O_CLOEXEC` symbol.
> O_CLOEXEC is supported by all reasonably current service platforms,
> so we should not need to fall back to compatibility code. So rather
> than fixing the fallback code, we can just eliminate it.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/open.hpp c9346c62e01688f0f55811f7acbe63321b084355
> 3rdparty/stout/include/stout/os/windows/fcntl.hpp ac90bf08ccf5b594e70310e9843475502b3603a5
> 3rdparty/stout/include/stout/windows.hpp 1d865f8fd23aba0198017f0bf4be8471cfb714ed
>
>
> Diff: https://reviews.apache.org/r/62638/diff/3/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
> Manually verified that sandbox files opened through the webui get the `O_CLOEXEC` flag applied.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 62638: Removed support for platforms without
O_CLOEXEC.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62638/#review186506
-----------------------------------------------------------
Ship it!
Looks good to me, just waiting on the Windows ReviewBot to test it since I'm lazy. Ship it unless the bot fails.
- Andrew Schwartzmeyer
On Sept. 27, 2017, 3:20 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62638/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2017, 3:20 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer and Jie Yu.
>
>
> Bugs: MESOS-8027
> https://issues.apache.org/jira/browse/MESOS-8027
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `O_CLOEXEC` fallback code was broken since it did not guarantee
> to include `fcntl.h` before checking for the `O_CLOEXEC` symbol.
> O_CLOEXEC is supported by all reasonably current service platforms,
> so we should not need to fall back to compatibility code. So rather
> than fixing the fallback code, we can just eliminate it.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/open.hpp c9346c62e01688f0f55811f7acbe63321b084355
> 3rdparty/stout/include/stout/os/windows/fcntl.hpp ac90bf08ccf5b594e70310e9843475502b3603a5
> 3rdparty/stout/include/stout/windows.hpp 1d865f8fd23aba0198017f0bf4be8471cfb714ed
>
>
> Diff: https://reviews.apache.org/r/62638/diff/2/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
> Manually verified that sandbox files opened through the webui get the `O_CLOEXEC` flag applied.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 62638: Removed support for platforms without
O_CLOEXEC.
Posted by James Peach <jp...@apache.org>.
> On Oct. 4, 2017, 9:26 a.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/os/open.hpp
> > Lines 26 (patched)
> > <https://reviews.apache.org/r/62638/diff/3/?file=1839275#file1839275line26>
> >
> > I believe we should update the minimal Linux kernel requirements in the getting started documentation, https://github.com/apache/mesos/blob/32ea75b8426fb3972680a397dc3a05ff266d166d/docs/getting-started.md#L27.
> >
> > With this change we are introducing a hard requirement on kernels >=2.6.23 for even basic functionality, see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f23513e8d96cf5e6cf8d2ff0cb5dd6bbc33995e4, https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4a19542e5f694cd408a32c3d9dc593ba9366e2d7.
Updated the docs in [r/62939](https://reviews.apache.org/r/62939/).
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62638/#review187074
-----------------------------------------------------------
On Sept. 27, 2017, 10:20 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62638/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2017, 10:20 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, and Jie Yu.
>
>
> Bugs: MESOS-8027
> https://issues.apache.org/jira/browse/MESOS-8027
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `O_CLOEXEC` fallback code was broken since it did not guarantee
> to include `fcntl.h` before checking for the `O_CLOEXEC` symbol.
> O_CLOEXEC is supported by all reasonably current service platforms,
> so we should not need to fall back to compatibility code. So rather
> than fixing the fallback code, we can just eliminate it.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/open.hpp c9346c62e01688f0f55811f7acbe63321b084355
> 3rdparty/stout/include/stout/os/windows/fcntl.hpp ac90bf08ccf5b594e70310e9843475502b3603a5
> 3rdparty/stout/include/stout/windows.hpp fd7ffeabd2f33c45dcd67f1f54db7671a89dac64
>
>
> Diff: https://reviews.apache.org/r/62638/diff/4/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
> Manually verified that sandbox files opened through the webui get the `O_CLOEXEC` flag applied.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 62638: Removed support for platforms without
O_CLOEXEC.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62638/#review187074
-----------------------------------------------------------
Fix it, then Ship it!
3rdparty/stout/include/stout/os/open.hpp
Lines 26 (patched)
<https://reviews.apache.org/r/62638/#comment264021>
I believe we should update the minimal Linux kernel requirements in the getting started documentation, https://github.com/apache/mesos/blob/32ea75b8426fb3972680a397dc3a05ff266d166d/docs/getting-started.md#L27.
With this change we are introducing a hard requirement on kernels >=2.6.23 for even basic functionality, see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f23513e8d96cf5e6cf8d2ff0cb5dd6bbc33995e4, https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4a19542e5f694cd408a32c3d9dc593ba9366e2d7.
- Benjamin Bannier
On Sept. 28, 2017, 12:20 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62638/
> -----------------------------------------------------------
>
> (Updated Sept. 28, 2017, 12:20 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, and Jie Yu.
>
>
> Bugs: MESOS-8027
> https://issues.apache.org/jira/browse/MESOS-8027
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `O_CLOEXEC` fallback code was broken since it did not guarantee
> to include `fcntl.h` before checking for the `O_CLOEXEC` symbol.
> O_CLOEXEC is supported by all reasonably current service platforms,
> so we should not need to fall back to compatibility code. So rather
> than fixing the fallback code, we can just eliminate it.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/open.hpp c9346c62e01688f0f55811f7acbe63321b084355
> 3rdparty/stout/include/stout/os/windows/fcntl.hpp ac90bf08ccf5b594e70310e9843475502b3603a5
> 3rdparty/stout/include/stout/windows.hpp 1d865f8fd23aba0198017f0bf4be8471cfb714ed
>
>
> Diff: https://reviews.apache.org/r/62638/diff/3/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
> Manually verified that sandbox files opened through the webui get the `O_CLOEXEC` flag applied.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 62638: Removed support for platforms without
O_CLOEXEC.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62638/#review186609
-----------------------------------------------------------
PASS: Mesos patch 62638 was successfully built and tested.
Reviews applied: `['62638']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62638
- Mesos Reviewbot Windows
On Sept. 27, 2017, 10:20 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62638/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2017, 10:20 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, and Jie Yu.
>
>
> Bugs: MESOS-8027
> https://issues.apache.org/jira/browse/MESOS-8027
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `O_CLOEXEC` fallback code was broken since it did not guarantee
> to include `fcntl.h` before checking for the `O_CLOEXEC` symbol.
> O_CLOEXEC is supported by all reasonably current service platforms,
> so we should not need to fall back to compatibility code. So rather
> than fixing the fallback code, we can just eliminate it.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/open.hpp c9346c62e01688f0f55811f7acbe63321b084355
> 3rdparty/stout/include/stout/os/windows/fcntl.hpp ac90bf08ccf5b594e70310e9843475502b3603a5
> 3rdparty/stout/include/stout/windows.hpp 1d865f8fd23aba0198017f0bf4be8471cfb714ed
>
>
> Diff: https://reviews.apache.org/r/62638/diff/3/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
> Manually verified that sandbox files opened through the webui get the `O_CLOEXEC` flag applied.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 62638: Removed support for platforms without
O_CLOEXEC.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62638/#review186517
-----------------------------------------------------------
PASS: Mesos patch 62638 was successfully built and tested.
Reviews applied: `['62638']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62638
- Mesos Reviewbot Windows
On Sept. 27, 2017, 10:20 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62638/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2017, 10:20 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer and Jie Yu.
>
>
> Bugs: MESOS-8027
> https://issues.apache.org/jira/browse/MESOS-8027
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `O_CLOEXEC` fallback code was broken since it did not guarantee
> to include `fcntl.h` before checking for the `O_CLOEXEC` symbol.
> O_CLOEXEC is supported by all reasonably current service platforms,
> so we should not need to fall back to compatibility code. So rather
> than fixing the fallback code, we can just eliminate it.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/open.hpp c9346c62e01688f0f55811f7acbe63321b084355
> 3rdparty/stout/include/stout/os/windows/fcntl.hpp ac90bf08ccf5b594e70310e9843475502b3603a5
> 3rdparty/stout/include/stout/windows.hpp 1d865f8fd23aba0198017f0bf4be8471cfb714ed
>
>
> Diff: https://reviews.apache.org/r/62638/diff/2/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
> Manually verified that sandbox files opened through the webui get the `O_CLOEXEC` flag applied.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 62638: Removed support for platforms without
O_CLOEXEC.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62638/#review186703
-----------------------------------------------------------
Patch looks great!
Reviews applied: [62638]
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. 27, 2017, 3:20 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62638/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2017, 3:20 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, and Jie Yu.
>
>
> Bugs: MESOS-8027
> https://issues.apache.org/jira/browse/MESOS-8027
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `O_CLOEXEC` fallback code was broken since it did not guarantee
> to include `fcntl.h` before checking for the `O_CLOEXEC` symbol.
> O_CLOEXEC is supported by all reasonably current service platforms,
> so we should not need to fall back to compatibility code. So rather
> than fixing the fallback code, we can just eliminate it.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/open.hpp c9346c62e01688f0f55811f7acbe63321b084355
> 3rdparty/stout/include/stout/os/windows/fcntl.hpp ac90bf08ccf5b594e70310e9843475502b3603a5
> 3rdparty/stout/include/stout/windows.hpp 1d865f8fd23aba0198017f0bf4be8471cfb714ed
>
>
> Diff: https://reviews.apache.org/r/62638/diff/3/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
> Manually verified that sandbox files opened through the webui get the `O_CLOEXEC` flag applied.
>
>
> Thanks,
>
> James Peach
>
>