Re: Review Request 34631: Added QoS Controller.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/ --- (Updated June 4, 2015, 2:23 p.m.) Review request for mesos, Bartek Plotka, Jie Yu, and Vinod Kone. Changes --- Forgot to add header to makefile Repository: mesos Description --- Added QoS Controller class and NOOP controller implementation Diffs (updated) - include/mesos/slave/qos_controller.hpp PRE-CREATION src/Makefile.am cc3d2e60fe2bb2b78ca93dd05dbf004a049c4aa5 src/slave/qos_controller.hpp PRE-CREATION src/slave/qos_controller.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34631/diff/ Testing --- make check Thanks, Niklas Nielsen
Re: Review Request 34631: Added QoS Controller.
On June 3, 2015, 4:02 p.m., Jie Yu wrote: include/mesos/slave/qos_controller.hpp, lines 25-26 https://reviews.apache.org/r/34631/diff/3/?file=977207#file977207line25 Can you add a wrapper header mesos/slave/oversubscription.hpp similar to what we do for things like fetcher? Added dependent review request - Niklas --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/#review86499 --- On June 4, 2015, 10:42 a.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/ --- (Updated June 4, 2015, 10:42 a.m.) Review request for mesos, Bartek Plotka, Jie Yu, and Vinod Kone. Repository: mesos Description --- Added QoS Controller class and NOOP controller implementation Diffs - include/mesos/slave/qos_controller.hpp PRE-CREATION src/Makefile.am cc3d2e60fe2bb2b78ca93dd05dbf004a049c4aa5 src/slave/qos_controller.hpp PRE-CREATION src/slave/qos_controller.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34631/diff/ Testing --- make check Thanks, Niklas Nielsen
Re: Review Request 34631: Added QoS Controller.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/ --- (Updated June 4, 2015, 10:42 a.m.) Review request for mesos, Bartek Plotka, Jie Yu, and Vinod Kone. Repository: mesos Description --- Added QoS Controller class and NOOP controller implementation Diffs (updated) - include/mesos/slave/qos_controller.hpp PRE-CREATION src/Makefile.am cc3d2e60fe2bb2b78ca93dd05dbf004a049c4aa5 src/slave/qos_controller.hpp PRE-CREATION src/slave/qos_controller.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34631/diff/ Testing --- make check Thanks, Niklas Nielsen
Re: Review Request 34631: Added QoS Controller.
On June 3, 2015, 5:54 p.m., Bartek Plotka wrote: include/mesos/slave/qos_controller.hpp, line 52 https://reviews.apache.org/r/34631/diff/3/?file=977207#file977207line52 Small thing: s/type/name/ ..to be consistent with allocator factory. (: What is the reason of naming it type? Niklas Nielsen wrote: It is called 'type' in the estimator as well: https://github.com/apache/mesos/blob/master/src/slave/resource_estimator.cpp#L28 Still want me to change? I know, so resource estimator has also that potential issue. In fact there is a consistency issue not only in naming.. I can see in mesos 2 approaches: 1. Allocator and authenticator (which were earlier then RE) -Defined default value if no flag. (https://github.com/apache/mesos/blob/master/src/master/flags.cpp#L349) -Allocator factory gets only *const string name* (not Option). We don't need to check if it's null.. (https://github.com/apache/mesos/blob/master/src/master/allocator/allocator.cpp#L37) 2. Your approach (and Jie Yu in https://reviews.apache.org/r/35028/diff/#) -There is no default flag, but it's it catched in Option so it can be None() -You have to check if it's None but we save space - we don't have to create default const in constant.hpp To sum up - i'm fine with any of that options, however i'd love to have everything done in the same manner (((: - Bartek --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/#review86447 --- On June 3, 2015, 4:26 a.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/ --- (Updated June 3, 2015, 4:26 a.m.) Review request for mesos, Bartek Plotka, Jie Yu, and Vinod Kone. Repository: mesos Description --- Added QoS Controller class and NOOP controller implementation Diffs - include/mesos/slave/qos_controller.hpp PRE-CREATION src/Makefile.am f045b89e8590c4775b21980bedff14cc27d31bfb src/slave/qos_controller.hpp PRE-CREATION src/slave/qos_controller.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34631/diff/ Testing --- make check Thanks, Niklas Nielsen
Re: Review Request 34631: Added QoS Controller.
On June 3, 2015, 10:54 a.m., Bartek Plotka wrote: include/mesos/slave/qos_controller.hpp, line 52 https://reviews.apache.org/r/34631/diff/3/?file=977207#file977207line52 Small thing: s/type/name/ ..to be consistent with allocator factory. (: What is the reason of naming it type? It is called 'type' in the estimator as well: https://github.com/apache/mesos/blob/master/src/slave/resource_estimator.cpp#L28 Still want me to change? - Niklas --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/#review86447 --- On June 2, 2015, 9:26 p.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/ --- (Updated June 2, 2015, 9:26 p.m.) Review request for mesos, Bartek Plotka, Jie Yu, and Vinod Kone. Repository: mesos Description --- Added QoS Controller class and NOOP controller implementation Diffs - include/mesos/slave/qos_controller.hpp PRE-CREATION src/Makefile.am f045b89e8590c4775b21980bedff14cc27d31bfb src/slave/qos_controller.hpp PRE-CREATION src/slave/qos_controller.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34631/diff/ Testing --- make check Thanks, Niklas Nielsen
Re: Review Request 34631: Added QoS Controller.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/#review86499 --- Ship it! include/mesos/slave/qos_controller.hpp https://reviews.apache.org/r/34631/#comment138542 Can you add a wrapper header mesos/slave/oversubscription.hpp similar to what we do for things like fetcher? src/slave/qos_controller.cpp https://reviews.apache.org/r/34631/#comment138543 Not sure about the style here. I thought we don't add leading spaces here (similar to template functions). - Jie Yu On June 3, 2015, 4:26 a.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/ --- (Updated June 3, 2015, 4:26 a.m.) Review request for mesos, Bartek Plotka, Jie Yu, and Vinod Kone. Repository: mesos Description --- Added QoS Controller class and NOOP controller implementation Diffs - include/mesos/slave/qos_controller.hpp PRE-CREATION src/Makefile.am f045b89e8590c4775b21980bedff14cc27d31bfb src/slave/qos_controller.hpp PRE-CREATION src/slave/qos_controller.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34631/diff/ Testing --- make check Thanks, Niklas Nielsen
Re: Review Request 34631: Added QoS Controller.
On June 3, 2015, 10:54 a.m., Bartek Plotka wrote: include/mesos/slave/qos_controller.hpp, line 52 https://reviews.apache.org/r/34631/diff/3/?file=977207#file977207line52 Small thing: s/type/name/ ..to be consistent with allocator factory. (: What is the reason of naming it type? Niklas Nielsen wrote: It is called 'type' in the estimator as well: https://github.com/apache/mesos/blob/master/src/slave/resource_estimator.cpp#L28 Still want me to change? Bartek Plotka wrote: I know, so resource estimator has also that potential issue. In fact there is a consistency issue not only in naming.. I can see in mesos 2 approaches: 1. Allocator and authenticator (which were earlier then RE) -Defined default value if no flag. (https://github.com/apache/mesos/blob/master/src/master/flags.cpp#L349) -Allocator factory gets only *const string name* (not Option). We don't need to check if it's null.. (https://github.com/apache/mesos/blob/master/src/master/allocator/allocator.cpp#L37) 2. Your approach (and Jie Yu in https://reviews.apache.org/r/35028/diff/#) -There is no default flag, but it's it catched in Option so it can be None() -You have to check if it's None but we save space - we don't have to create default const in constant.hpp To sum up - i'm fine with any of that options, however i'd love to have everything done in the same manner (((: Will unify in a follow up patch: https://issues.apache.org/jira/browse/MESOS-2813 dropping issue for now :) - Niklas --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/#review86447 --- On June 2, 2015, 9:26 p.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/ --- (Updated June 2, 2015, 9:26 p.m.) Review request for mesos, Bartek Plotka, Jie Yu, and Vinod Kone. Repository: mesos Description --- Added QoS Controller class and NOOP controller implementation Diffs - include/mesos/slave/qos_controller.hpp PRE-CREATION src/Makefile.am f045b89e8590c4775b21980bedff14cc27d31bfb src/slave/qos_controller.hpp PRE-CREATION src/slave/qos_controller.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34631/diff/ Testing --- make check Thanks, Niklas Nielsen
Re: Review Request 34631: Added QoS Controller.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/#review86447 --- include/mesos/slave/qos_controller.hpp https://reviews.apache.org/r/34631/#comment138434 Small thing: s/type/name/ ..to be consistent with allocator factory. (: What is the reason of naming it type? src/slave/qos_controller.cpp https://reviews.apache.org/r/34631/#comment138435 The same potential issue here. (s/type/name/) - Bartek Plotka On June 3, 2015, 4:26 a.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/ --- (Updated June 3, 2015, 4:26 a.m.) Review request for mesos, Bartek Plotka, Jie Yu, and Vinod Kone. Repository: mesos Description --- Added QoS Controller class and NOOP controller implementation Diffs - include/mesos/slave/qos_controller.hpp PRE-CREATION src/Makefile.am f045b89e8590c4775b21980bedff14cc27d31bfb src/slave/qos_controller.hpp PRE-CREATION src/slave/qos_controller.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34631/diff/ Testing --- make check Thanks, Niklas Nielsen
Re: Review Request 34631: Added QoS Controller.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/ --- (Updated June 2, 2015, 9:26 p.m.) Review request for mesos, Bartek Plotka, Jie Yu, and Vinod Kone. Repository: mesos Description --- Added QoS Controller class and NOOP controller implementation Diffs (updated) - include/mesos/slave/qos_controller.hpp PRE-CREATION src/Makefile.am f045b89e8590c4775b21980bedff14cc27d31bfb src/slave/qos_controller.hpp PRE-CREATION src/slave/qos_controller.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34631/diff/ Testing --- make check Thanks, Niklas Nielsen
Re: Review Request 34631: Added QoS Controller.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/#review86004 --- Ship it! Ditto Vinod's comment on s/BE/revokable/ src/slave/qos_controller.cpp https://reviews.apache.org/r/34631/#comment137842 2 lines apart please. - Jie Yu On May 26, 2015, 3:30 p.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/ --- (Updated May 26, 2015, 3:30 p.m.) Review request for mesos, Bartek Plotka, Jie Yu, and Vinod Kone. Repository: mesos Description --- Added QoS Controller class and NOOP controller implementation Diffs - include/mesos/slave/qos_controller.hpp PRE-CREATION src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 src/slave/qos_controller.hpp PRE-CREATION src/slave/qos_controller.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34631/diff/ Testing --- make check Thanks, Niklas Nielsen
Re: Review Request 34631: Added QoS Controller.
On May 29, 2015, 5:39 p.m., Vinod Kone wrote: include/mesos/slave/qos_controller.hpp, line 62 https://reviews.apache.org/r/34631/diff/2/?file=971661#file971661line62 s/QoSCorrection/listQoSCorrection/ ? Probably calling it 'correction' and returns a single QoSCorrection? Can you remind me why we want to return a list? - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/#review85757 --- On May 26, 2015, 3:30 p.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/ --- (Updated May 26, 2015, 3:30 p.m.) Review request for mesos, Bartek Plotka, Jie Yu, and Vinod Kone. Repository: mesos Description --- Added QoS Controller class and NOOP controller implementation Diffs - include/mesos/slave/qos_controller.hpp PRE-CREATION src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 src/slave/qos_controller.hpp PRE-CREATION src/slave/qos_controller.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34631/diff/ Testing --- make check Thanks, Niklas Nielsen
Re: Review Request 34631: Added QoS Controller.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/#review85757 --- include/mesos/slave/qos_controller.hpp https://reviews.apache.org/r/34631/#comment137507 Instead of calling them best-effor or BE tasks, lets just call them revocable tasks? That way it'll be easy to correlate revocable tasks with revocable resources. include/mesos/slave/qos_controller.hpp https://reviews.apache.org/r/34631/#comment137508 s/BE/revocable/ include/mesos/slave/qos_controller.hpp https://reviews.apache.org/r/34631/#comment137510 s/QoSCorrection/listQoSCorrection/ ? src/slave/qos_controller.hpp https://reviews.apache.org/r/34631/#comment137509 s/our/out/ - Vinod Kone On May 26, 2015, 3:30 p.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/ --- (Updated May 26, 2015, 3:30 p.m.) Review request for mesos, Bartek Plotka, Jie Yu, and Vinod Kone. Repository: mesos Description --- Added QoS Controller class and NOOP controller implementation Diffs - include/mesos/slave/qos_controller.hpp PRE-CREATION src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 src/slave/qos_controller.hpp PRE-CREATION src/slave/qos_controller.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34631/diff/ Testing --- make check Thanks, Niklas Nielsen
Re: Review Request 34631: Added QoS Controller.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/#review85267 --- Ship it! Ship It! - Bartek Plotka On May 26, 2015, 3:30 p.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/ --- (Updated May 26, 2015, 3:30 p.m.) Review request for mesos, Bartek Plotka, Jie Yu, and Vinod Kone. Repository: mesos Description --- Added QoS Controller class and NOOP controller implementation Diffs - include/mesos/slave/qos_controller.hpp PRE-CREATION src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 src/slave/qos_controller.hpp PRE-CREATION src/slave/qos_controller.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34631/diff/ Testing --- make check Thanks, Niklas Nielsen
Re: Review Request 34631: Added QoS Controller.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/ --- (Updated May 26, 2015, 8:30 a.m.) Review request for mesos, Bartek Plotka, Jie Yu, and Vinod Kone. Changes --- Added missing Makefile entry for new public header. Repository: mesos Description --- Added QoS Controller class and NOOP controller implementation Diffs (updated) - include/mesos/slave/qos_controller.hpp PRE-CREATION src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 src/slave/qos_controller.hpp PRE-CREATION src/slave/qos_controller.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34631/diff/ Testing --- make check Thanks, Niklas Nielsen
Re: Review Request 34631: Added QoS Controller.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/#review85150 --- include/mesos/slave/qos_controller.hpp https://reviews.apache.org/r/34631/#comment136645 I think it's better idea to do it ASAP.. We produce more and more code, which we will have to modify after we expose that Resource Usage (same situation as with ResourceEstimator) - and we are aware of that. Maybe we should do that in the first place? - Bartek Plotka On May 23, 2015, 5:06 p.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34631/ --- (Updated May 23, 2015, 5:06 p.m.) Review request for mesos, Bartek Plotka, Jie Yu, and Vinod Kone. Repository: mesos Description --- Added QoS Controller class and NOOP controller implementation Diffs - include/mesos/slave/qos_controller.hpp PRE-CREATION src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 src/slave/qos_controller.hpp PRE-CREATION src/slave/qos_controller.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34631/diff/ Testing --- make check Thanks, Niklas Nielsen
