You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2016/12/02 06:42:48 UTC
Review Request 54291: Added os::ptsname to stout.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54291/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Kevin Klues.
Repository: mesos
Description
-------
Added os::ptsname to stout.
Diffs
-----
3rdparty/stout/include/stout/posix/os.hpp c37e64db662ba3cee83d2f55de0f9d71ad72c038
Diff: https://reviews.apache.org/r/54291/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 54291: Added os::ptsname to stout.
Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54291/#review157953
-----------------------------------------------------------
Ship it!
Ship It!
- Kevin Klues
On Dec. 2, 2016, 6:29 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54291/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2016, 6:29 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Kevin Klues.
>
>
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added os::ptsname to stout.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp c37e64db662ba3cee83d2f55de0f9d71ad72c038
>
> Diff: https://reviews.apache.org/r/54291/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 54291: Added os::ptsname to stout.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54291/
-----------------------------------------------------------
(Updated Dec. 5, 2016, 5:34 a.m.)
Review request for mesos, Benjamin Hindman and Kevin Klues.
Changes
-------
Addressed comments. Rebased.
Bugs: MESOS-6470
https://issues.apache.org/jira/browse/MESOS-6470
Repository: mesos
Description
-------
Added os::ptsname to stout.
Diffs (updated)
-----
3rdparty/stout/include/stout/posix/os.hpp c37e64db662ba3cee83d2f55de0f9d71ad72c038
Diff: https://reviews.apache.org/r/54291/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 54291: Added os::ptsname to stout.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54291/#review157948
-----------------------------------------------------------
Ship it!
Adding the TODO for `ptsname_r` on Linux SGTM as well as using `const char*` if possible. Otherwise LGTM.
- Benjamin Hindman
On Dec. 2, 2016, 6:29 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54291/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2016, 6:29 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Kevin Klues.
>
>
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added os::ptsname to stout.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp c37e64db662ba3cee83d2f55de0f9d71ad72c038
>
> Diff: https://reviews.apache.org/r/54291/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 54291: Added os::ptsname to stout.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 2, 2016, 7:29 p.m., Kevin Klues wrote:
> > 3rdparty/stout/include/stout/posix/os.hpp, line 480
> > <https://reviews.apache.org/r/54291/diff/1/?file=1574565#file1574565line480>
> >
> > If I recall, there was a reason we usually only declare static pointers (and leak memory) instead of statically declaring actual objects, but I don't remember of the top of my head. Joris explained it to me once.
yeah, to avoid global non-POD data. I'll add a TODO to use ptsname_r for linux.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54291/#review157800
-----------------------------------------------------------
On Dec. 2, 2016, 6:29 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54291/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2016, 6:29 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Kevin Klues.
>
>
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added os::ptsname to stout.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp c37e64db662ba3cee83d2f55de0f9d71ad72c038
>
> Diff: https://reviews.apache.org/r/54291/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 54291: Added os::ptsname to stout.
Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54291/#review157800
-----------------------------------------------------------
3rdparty/stout/include/stout/posix/os.hpp (line 480)
<https://reviews.apache.org/r/54291/#comment228401>
If I recall, there was a reason we usually only declare static pointers (and leak memory) instead of statically declaring actual objects, but I don't remember of the top of my head. Joris explained it to me once.
- Kevin Klues
On Dec. 2, 2016, 6:29 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54291/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2016, 6:29 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Kevin Klues.
>
>
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added os::ptsname to stout.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp c37e64db662ba3cee83d2f55de0f9d71ad72c038
>
> Diff: https://reviews.apache.org/r/54291/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 54291: Added os::ptsname to stout.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54291/
-----------------------------------------------------------
(Updated Dec. 2, 2016, 6:29 p.m.)
Review request for mesos, Benjamin Hindman and Kevin Klues.
Bugs: MESOS-6470
https://issues.apache.org/jira/browse/MESOS-6470
Repository: mesos
Description
-------
Added os::ptsname to stout.
Diffs
-----
3rdparty/stout/include/stout/posix/os.hpp c37e64db662ba3cee83d2f55de0f9d71ad72c038
Diff: https://reviews.apache.org/r/54291/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 54291: Added os::ptsname to stout.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 2, 2016, 9:16 a.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/posix/os.hpp, line 480
> > <https://reviews.apache.org/r/54291/diff/1/?file=1574565#file1574565line480>
> >
> > We should be able to use a simple function-local static here and avoid the global leak of `mutex`, e.g.,
> >
> > static std::mutex mutex;
We use pointer here to avoid global non-POD data.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54291/#review157711
-----------------------------------------------------------
On Dec. 2, 2016, 6:29 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54291/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2016, 6:29 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Kevin Klues.
>
>
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added os::ptsname to stout.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp c37e64db662ba3cee83d2f55de0f9d71ad72c038
>
> Diff: https://reviews.apache.org/r/54291/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 54291: Added os::ptsname to stout.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Dec. 2, 2016, 10:16 a.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/posix/os.hpp, line 480
> > <https://reviews.apache.org/r/54291/diff/1/?file=1574565#file1574565line480>
> >
> > We should be able to use a simple function-local static here and avoid the global leak of `mutex`, e.g.,
> >
> > static std::mutex mutex;
>
> Jie Yu wrote:
> We use pointer here to avoid global non-POD data.
This sounds like a very useful additon to the coding standard. Currently the code base seems to be split regarding the right pattern, e.g., overly simplistic grep patterns give
3rdparty/libprocess/src/clock.cpp:static recursive_mutex* timers_mutex = new recursive_mutex();
3rdparty/libprocess/src/libevent.cpp:static std::mutex* functions_mutex = new std::mutex();
3rdparty/libprocess/src/process.cpp: static std::mutex* prefixes_mutex = new std::mutex();
3rdparty/libprocess/src/process.cpp:static std::mutex* socket_mutex = new std::mutex();
3rdparty/libprocess/src/process.cpp:static std::recursive_mutex* filterer_mutex = new std::recursive_mutex();
3rdparty/stout/include/stout/posix/os.hpp: static std::mutex* mutex = new std::mutex;
and
src/authentication/cram_md5/auxprop.hpp: static std::mutex mutex;
src/hook/manager.cpp:static std::mutex mutex;
src/linux/fs.cpp: static std::mutex mutex;
src/module/manager.hpp: static std::mutex mutex;
src/tests/containerizer.cpp: static std::mutex mutex;
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54291/#review157711
-----------------------------------------------------------
On Dec. 5, 2016, 6:34 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54291/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2016, 6:34 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Kevin Klues.
>
>
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added os::ptsname to stout.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp c37e64db662ba3cee83d2f55de0f9d71ad72c038
>
> Diff: https://reviews.apache.org/r/54291/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 54291: Added os::ptsname to stout.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54291/#review157711
-----------------------------------------------------------
3rdparty/stout/include/stout/posix/os.hpp (line 480)
<https://reviews.apache.org/r/54291/#comment228328>
We should be able to use a simple function-local static here and avoid the global leak of `mutex`, e.g.,
static std::mutex mutex;
3rdparty/stout/include/stout/posix/os.hpp (line 483)
<https://reviews.apache.org/r/54291/#comment228329>
We can use a `const char*` here as we wouldn't want to modify the returned value.
- Benjamin Bannier
On Dec. 2, 2016, 7:42 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54291/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2016, 7:42 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Kevin Klues.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added os::ptsname to stout.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp c37e64db662ba3cee83d2f55de0f9d71ad72c038
>
> Diff: https://reviews.apache.org/r/54291/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>