You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ian Downes <ia...@gmail.com> on 2014/04/02 23:42:14 UTC
Review Request 19951: Change cgroups::assign to assign all threads.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19951/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Repository: mesos-git
Description
-------
Use cgroups.proc (rather than tasks) so all threads in the pid's threadgroup are assigned.
Documentation/cgroups/cgroups.txt:
"You can use the cgroup.procs file instead of the tasks file to move all threads in a threadgroup at once. "
Added a test to verify all threads are assigned.
This obviates the recently introduced assignAllThreads and thus it has been removed.
Diffs
-----
src/linux/cgroups.hpp 5a5735721fb9f051eee661edb08d1cdaa163d0f3
src/linux/cgroups.cpp 8202c282f580d027a60ded2081962e96e4860f60
src/slave/slave.cpp a356f5f0a0a43581c6380df1bc9b15fd228b3015
src/tests/cgroups_tests.cpp 6ba9de622953e158feadaa9950618b0b13c9e832
src/tests/cluster.hpp 11684d99c6a4e623dd5ff9977d210de59f33f5cd
Diff: https://reviews.apache.org/r/19951/diff/
Testing
-------
make check # Linux
Thanks,
Ian Downes
Re: Review Request 19951: Change cgroups::assign to assign all threads.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19951/#review39404
-----------------------------------------------------------
src/tests/cgroups_tests.cpp
<https://reviews.apache.org/r/19951/#comment71804>
Okay, let's fix it then, and then clean this up. Happy to help: https://reviews.apache.org/r/19991.
- Benjamin Hindman
On April 2, 2014, 9:42 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19951/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 9:42 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Use cgroups.proc (rather than tasks) so all threads in the pid's threadgroup are assigned.
>
> Documentation/cgroups/cgroups.txt:
> "You can use the cgroup.procs file instead of the tasks file to move all threads in a threadgroup at once. "
>
> Added a test to verify all threads are assigned.
>
> This obviates the recently introduced assignAllThreads and thus it has been removed.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 5a5735721fb9f051eee661edb08d1cdaa163d0f3
> src/linux/cgroups.cpp 8202c282f580d027a60ded2081962e96e4860f60
> src/slave/slave.cpp a356f5f0a0a43581c6380df1bc9b15fd228b3015
> src/tests/cgroups_tests.cpp 6ba9de622953e158feadaa9950618b0b13c9e832
> src/tests/cluster.hpp 11684d99c6a4e623dd5ff9977d210de59f33f5cd
>
> Diff: https://reviews.apache.org/r/19951/diff/
>
>
> Testing
> -------
>
> make check # Linux
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 19951: Change cgroups::assign to assign all threads.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19951/#review39368
-----------------------------------------------------------
Bad patch!
Reviews applied: [19951]
Failed command: make -j3 check GTEST_FILTER='' >/dev/null
Error:
ev.c:1531:31: warning: 'ev_default_loop_ptr' initialized and declared 'extern' [enabled by default]
ev.c: In function 'evpipe_write':
ev.c:2160:17: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
ev.c:2172:17: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
ev.c: In function 'pipecb':
ev.c:2193:16: warning: ignoring return value of 'read', declared with attribute warn_unused_result [-Wunused-result]
ev.c:2207:16: warning: ignoring return value of 'read', declared with attribute warn_unused_result [-Wunused-result]
In file included from /usr/include/c++/4.6/ext/hash_set:61:0,
from src/glog/stl_logging.h:54,
from src/stl_logging_unittest.cc:34:
/usr/include/c++/4.6/backward/backward_warning.h:33:2: warning: #warning This file includes at least one deprecated or antiquated header which may be removed without further notice at a future date. Please use a non-deprecated interface with equivalent functionality instead. For a listing of replacement headers and interfaces, consult the file backward_warning.h. To disable this warning use -Wno-deprecated. [-Wcpp]
In file included from src/utilities.h:73:0,
from src/googletest.h:38,
from src/stl_logging_unittest.cc:48:
src/base/mutex.h:137:0: warning: "_XOPEN_SOURCE" redefined [enabled by default]
/usr/include/features.h:166:0: note: this is the location of the previous definition
warning: no files found matching 'Makefile' under directory 'docs'
warning: no files found matching 'indexsidebar.html' under directory 'docs'
zip_safe flag not set; analyzing archive contents...
WARNING: '.' not a valid package name; please use only.-separated package names in setup.py
package init file 'src/__init__.py' not found (or not a regular file)
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
zip_safe flag not set; analyzing archive contents...
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
In file included from ../3rdparty/libprocess/3rdparty/gmock-1.6.0/include/gmock/internal/gmock-internal-utils.h:47:0,
from ../3rdparty/libprocess/3rdparty/gmock-1.6.0/include/gmock/gmock-actions.h:46,
from ../3rdparty/libprocess/3rdparty/gmock-1.6.0/include/gmock/gmock.h:58,
from tests/cgroups_tests.cpp:36:
../3rdparty/libprocess/3rdparty/gmock-1.6.0/gtest/include/gtest/gtest.h: In function 'testing::AssertionResult testing::internal::CmpHelperEQ(const char*, const char*, const T1&, const T2&) [with T1 = long unsigned int, T2 = long int]':
../3rdparty/libprocess/3rdparty/gmock-1.6.0/gtest/include/gtest/gtest.h:1353:30: instantiated from 'static testing::AssertionResult testing::internal::EqHelper<lhs_is_null_literal>::Compare(const char*, const char*, const T1&, const T2&) [with T1 = long unsigned int, T2 = long int, bool lhs_is_null_literal = false]'
tests/cgroups_tests.cpp:870:3: instantiated from here
../3rdparty/libprocess/3rdparty/gmock-1.6.0/gtest/include/gtest/gtest.h:1316:3: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
cc1plus: all warnings being treated as errors
make[3]: *** [tests/mesos_tests-cgroups_tests.o] Error 1
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [check-am] Error 2
make[1]: *** [check] Error 2
make: *** [check-recursive] Error 1
- Mesos ReviewBot
On April 2, 2014, 9:42 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19951/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 9:42 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Use cgroups.proc (rather than tasks) so all threads in the pid's threadgroup are assigned.
>
> Documentation/cgroups/cgroups.txt:
> "You can use the cgroup.procs file instead of the tasks file to move all threads in a threadgroup at once. "
>
> Added a test to verify all threads are assigned.
>
> This obviates the recently introduced assignAllThreads and thus it has been removed.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 5a5735721fb9f051eee661edb08d1cdaa163d0f3
> src/linux/cgroups.cpp 8202c282f580d027a60ded2081962e96e4860f60
> src/slave/slave.cpp a356f5f0a0a43581c6380df1bc9b15fd228b3015
> src/tests/cgroups_tests.cpp 6ba9de622953e158feadaa9950618b0b13c9e832
> src/tests/cluster.hpp 11684d99c6a4e623dd5ff9977d210de59f33f5cd
>
> Diff: https://reviews.apache.org/r/19951/diff/
>
>
> Testing
> -------
>
> make check # Linux
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 19951: Change cgroups::assign to assign all threads.
Posted by Ian Downes <ia...@gmail.com>.
> On April 9, 2014, 10:12 p.m., Vinod Kone wrote:
> > src/linux/cgroups.cpp, line 1073
> > <https://reviews.apache.org/r/19951/diff/2/?file=548435#file548435line1073>
> >
> > woah. i thought this was not supported by kernels < 3.10 !?
That was my initial, incorrect understanding based partly because we'd been using tasks which definitely doesn't move all threads in a thread group. However:
Documentation/cgroups/cgroups.txt:
"You can use the cgroup.procs file instead of the tasks file to move all threads in a threadgroup at once. "
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19951/#review39948
-----------------------------------------------------------
On April 3, 2014, 10:59 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19951/
> -----------------------------------------------------------
>
> (Updated April 3, 2014, 10:59 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Use cgroups.proc (rather than tasks) so all threads in the pid's threadgroup are assigned.
>
> Documentation/cgroups/cgroups.txt:
> "You can use the cgroup.procs file instead of the tasks file to move all threads in a threadgroup at once. "
>
> Added a test to verify all threads are assigned.
>
> This obviates the recently introduced assignAllThreads and thus it has been removed.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 5a5735721fb9f051eee661edb08d1cdaa163d0f3
> src/linux/cgroups.cpp 8202c282f580d027a60ded2081962e96e4860f60
> src/slave/slave.cpp a356f5f0a0a43581c6380df1bc9b15fd228b3015
> src/tests/cgroups_tests.cpp 6ba9de622953e158feadaa9950618b0b13c9e832
> src/tests/cluster.hpp 11684d99c6a4e623dd5ff9977d210de59f33f5cd
>
> Diff: https://reviews.apache.org/r/19951/diff/
>
>
> Testing
> -------
>
> make check # Linux
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 19951: Change cgroups::assign to assign all threads.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19951/#review39948
-----------------------------------------------------------
Ship it!
Ian, can you commit this?
src/tests/cgroups_tests.cpp
<https://reviews.apache.org/r/19951/#comment72723>
How about
// Check the test cgroup now only contains all the child threads.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/19951/#comment72722>
woah. i thought this was not supported by kernels < 3.10 !?
- Vinod Kone
On April 3, 2014, 10:59 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19951/
> -----------------------------------------------------------
>
> (Updated April 3, 2014, 10:59 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Use cgroups.proc (rather than tasks) so all threads in the pid's threadgroup are assigned.
>
> Documentation/cgroups/cgroups.txt:
> "You can use the cgroup.procs file instead of the tasks file to move all threads in a threadgroup at once. "
>
> Added a test to verify all threads are assigned.
>
> This obviates the recently introduced assignAllThreads and thus it has been removed.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 5a5735721fb9f051eee661edb08d1cdaa163d0f3
> src/linux/cgroups.cpp 8202c282f580d027a60ded2081962e96e4860f60
> src/slave/slave.cpp a356f5f0a0a43581c6380df1bc9b15fd228b3015
> src/tests/cgroups_tests.cpp 6ba9de622953e158feadaa9950618b0b13c9e832
> src/tests/cluster.hpp 11684d99c6a4e623dd5ff9977d210de59f33f5cd
>
> Diff: https://reviews.apache.org/r/19951/diff/
>
>
> Testing
> -------
>
> make check # Linux
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 19951: Change cgroups::assign to assign all threads.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19951/#review39495
-----------------------------------------------------------
Patch looks great!
Reviews applied: [19951]
All tests passed.
- Mesos ReviewBot
On April 3, 2014, 10:59 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19951/
> -----------------------------------------------------------
>
> (Updated April 3, 2014, 10:59 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Use cgroups.proc (rather than tasks) so all threads in the pid's threadgroup are assigned.
>
> Documentation/cgroups/cgroups.txt:
> "You can use the cgroup.procs file instead of the tasks file to move all threads in a threadgroup at once. "
>
> Added a test to verify all threads are assigned.
>
> This obviates the recently introduced assignAllThreads and thus it has been removed.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 5a5735721fb9f051eee661edb08d1cdaa163d0f3
> src/linux/cgroups.cpp 8202c282f580d027a60ded2081962e96e4860f60
> src/slave/slave.cpp a356f5f0a0a43581c6380df1bc9b15fd228b3015
> src/tests/cgroups_tests.cpp 6ba9de622953e158feadaa9950618b0b13c9e832
> src/tests/cluster.hpp 11684d99c6a4e623dd5ff9977d210de59f33f5cd
>
> Diff: https://reviews.apache.org/r/19951/diff/
>
>
> Testing
> -------
>
> make check # Linux
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 19951: Change cgroups::assign to assign all threads.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19951/
-----------------------------------------------------------
(Updated April 3, 2014, 10:59 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Revised for updated os::read().
Repository: mesos-git
Description
-------
Use cgroups.proc (rather than tasks) so all threads in the pid's threadgroup are assigned.
Documentation/cgroups/cgroups.txt:
"You can use the cgroup.procs file instead of the tasks file to move all threads in a threadgroup at once. "
Added a test to verify all threads are assigned.
This obviates the recently introduced assignAllThreads and thus it has been removed.
Diffs (updated)
-----
src/linux/cgroups.hpp 5a5735721fb9f051eee661edb08d1cdaa163d0f3
src/linux/cgroups.cpp 8202c282f580d027a60ded2081962e96e4860f60
src/slave/slave.cpp a356f5f0a0a43581c6380df1bc9b15fd228b3015
src/tests/cgroups_tests.cpp 6ba9de622953e158feadaa9950618b0b13c9e832
src/tests/cluster.hpp 11684d99c6a4e623dd5ff9977d210de59f33f5cd
Diff: https://reviews.apache.org/r/19951/diff/
Testing
-------
make check # Linux
Thanks,
Ian Downes