Review Request 26487: Perform read right after subprocess for docker ps
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26487/ --- Review request for mesos and Benjamin Hindman. Repository: mesos-git Description --- Perform read right after subprocess for docker ps Diffs - src/docker/docker.hpp 7972edaf3038e70b6a5ed369f9b8de0c644fc4b0 src/docker/docker.cpp 20be6cd4649e12ed6f7d5d1991b5040342c56f35 Diff: https://reviews.apache.org/r/26487/diff/ Testing --- make check Thanks, Timothy Chen
Review Request 26486: Fix containerizer not receiving destroy/update calls when launching
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26486/ --- Review request for mesos and Benjamin Hindman. Bugs: MESOS-1884 https://issues.apache.org/jira/browse/MESOS-1884 Repository: mesos-git Description --- Fix containerizer not receiving destroy/update calls when launching Diffs - src/Makefile.am d503c8df73cda15a9d59254e8265e4a5d0e003a4 src/slave/containerizer/composing.cpp 9022700b628d9746a6a8a17c9fbf1b1988da6fca src/tests/composing_containerizer_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26486/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 26486: Fix containerizer not receiving destroy/update calls when launching
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26486/#review55954 --- Patch looks great! Reviews applied: [26486] All tests passed. - Mesos ReviewBot On Oct. 9, 2014, 5:58 a.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26486/ --- (Updated Oct. 9, 2014, 5:58 a.m.) Review request for mesos and Benjamin Hindman. Bugs: MESOS-1884 https://issues.apache.org/jira/browse/MESOS-1884 Repository: mesos-git Description --- Fix containerizer not receiving destroy/update calls when launching Diffs - src/Makefile.am d503c8df73cda15a9d59254e8265e4a5d0e003a4 src/slave/containerizer/composing.cpp 9022700b628d9746a6a8a17c9fbf1b1988da6fca src/tests/composing_containerizer_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26486/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 26436: Avoid docker inspect on each usage call
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26436/#review55955 --- src/slave/containerizer/docker.cpp https://reviews.apache.org/r/26436/#comment96314 Can we make the container here `const`? We don't actually do any modification on it in this function. - Michael Park On Oct. 8, 2014, 6:43 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26436/ --- (Updated Oct. 8, 2014, 6:43 p.m.) Review request for mesos and Benjamin Hindman. Repository: mesos-git Description --- Review: https://reviews.apache.org/r/26436 Diffs - src/slave/containerizer/docker.cpp 9a2948951f57f3ab16291df51cd9f33e5e96add4 Diff: https://reviews.apache.org/r/26436/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 25184: Delete framework data in TaskStatus to avoid OOM
On Oct. 7, 2014, 12:14 a.m., Timothy Chen wrote: Chengwei are you still able to work on this patch ? Will like to see this get merged in 0.21 @Timothy, sorry to late, I have a one week holiday last week, will update this patch within this week. - Chengwei --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25184/#review55509 --- On Sept. 27, 2014, 12:01 a.m., Chengwei Yang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25184/ --- (Updated Sept. 27, 2014, 12:01 a.m.) Review request for mesos, Adam B and Timothy St. Clair. Bugs: MESOS-1746 https://issues.apache.org/jira/browse/MESOS-1746 Repository: mesos-git Description --- There was a bug found that Spark use TaskStatus.data to transfer computed result and mesos-master RES memory keeps increasing fast and finally will be killed by OOM killer. Diffs - src/master/master.cpp 2508b38e86b8399886bffcbaca8ec11c731363d8 Diff: https://reviews.apache.org/r/25184/diff/ Testing --- tested with spark Thanks, Chengwei Yang
Re: Review Request 19180: Fix mesos command parsing help
On May 16, 2014, 7:32 a.m., Niklas Nielsen wrote: Hey Chengwei - sorry for the tardy turnaround time on this review request. To me, it still seems like we are treating the symptoms of the real issue: PATH is appended multiple times and the subsequent globbing adds the available commands to same number of times. The reason I am saying this is because the fix is difficult to understand (it is not immediate that this is the problem it solves) and seems very specialized for the mesos help --help and mesos help help case. Two things we could do: 1) Don't add the new path unconditionally to the PATH variable i.e. check if it is already there. 2) In usage(), don't add duplicates to the commands from the globbed list of candidates. This can be done pretty easy and local by using a set instead of a list. Try to take a look at: https://github.com/nqn/mesos/commit/01f77a1ab6d96f48765cc3539da1a1ebd4ba1d56 Thoughts? Adam B wrote: +1 Love the 'set'. Calling mesos help foo will still recurse into main and dirname will still be prepended to PATH multiple times, but the commands will not be printed multiple times. mesos help help will give a weird error (Not expecting --help before command) instead of calling usage, but I think that's a pretty contrived case. Chengwei Yang wrote: Hi Niklas, Sorry for late reply, so since the 2) improvement landed into usage(), so anyway we can't get duplicated commands in usage now though the 1) thing is still left to take. Do you like the first version of this patch? Which just do the small fix, add directory to PATH in the first through. Niklas Nielsen wrote: Can you point me review there 2) landed? If that's is in, why bother with 1)? I am still _not_ in favor of a notion of firstThrough with a static variable - if anything, it should be firstPass and I already enumerated other ways of doing it. If you want to push for that fix still, I suggest we find another shepherd for this RR. Chengwei Yang wrote: Sorry, I didn't aware that patch is still in your branch, not the official mesos repo. Do you submit that patch recently? Even we have your patch merged, the issue (directory added into $PATH more than once) is still here. I'll align this patch with your vision soon. Niklas Nielsen wrote: Hi Chengwei - do you still want this to go in? I'll revisite this patch this week, just have one week holiday last week. - Chengwei --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19180/#review43181 --- On May 16, 2014, 6:48 a.m., Chengwei Yang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19180/ --- (Updated May 16, 2014, 6:48 a.m.) Review request for mesos and Niklas Nielsen. Bugs: MESOS-1093 https://issues.apache.org/jira/browse/MESOS-1093 Repository: mesos-git Description --- Fix mesos command parsing help Without this patch, mesos help --help will output below Not expecting '--help' before command Usage: mesos command [OPTIONS] Available commands: help resolve cat scp log tail execute ps local resolve cat scp log tail execute ps local Apparently all available commands printed twice, the mesos help help will print available commands 3 times. The root cause is the directory contains available mesos commands are added more than one times when recursive to main() again. Idea comes from Adam B. Review: https://reviews.apache.org/r/19180 Diffs - src/cli/mesos.cpp 171a707cd2ba2348898e7fbe8fe9f0634edd6d86 Diff: https://reviews.apache.org/r/19180/diff/ Testing --- done? Thanks, Chengwei Yang
Re: Review Request 26487: Perform read right after subprocess for docker ps
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26487/#review55960 --- Patch looks great! Reviews applied: [26487] All tests passed. - Mesos ReviewBot On Oct. 9, 2014, 6:03 a.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26487/ --- (Updated Oct. 9, 2014, 6:03 a.m.) Review request for mesos and Benjamin Hindman. Repository: mesos-git Description --- Perform read right after subprocess for docker ps Diffs - src/docker/docker.hpp 7972edaf3038e70b6a5ed369f9b8de0c644fc4b0 src/docker/docker.cpp 20be6cd4649e12ed6f7d5d1991b5040342c56f35 Diff: https://reviews.apache.org/r/26487/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 26472: stout: Always use stout ABORT() rather than system abort()
On Oct. 8, 2014, 8:49 p.m., Dominic Hamon wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/stringify.hpp, line 36 https://reviews.apache.org/r/26472/diff/1/?file=716305#file716305line36 it might be worth considering using stringstream here to build the same error message we had before. we will have a stack trace, but having the thing that was attempting to stringify could be useful too, i think. +1 on having the thing that was attempting to stringify could be useful - Adam --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26472/#review55946 --- On Oct. 8, 2014, 6:20 p.m., Cody Maloney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26472/ --- (Updated Oct. 8, 2014, 6:20 p.m.) Review request for mesos, Adam B and Dominic Hamon. Bugs: MESOS-1870 https://issues.apache.org/jira/browse/MESOS-1870 Repository: mesos-git Description --- This makes it so any time there is an abort, we get a line number and at least a basic message as to why there was an abort. If you want a clean(er) exit, use stout/exit. Also adds an overload which takes a standard string and unwraps it to a const char * automatically, since a lot of the time we are building strings to pass them to abort. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/abort.hpp 6b5b5d1 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9d244b2 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp 7138bc2 3rdparty/libprocess/3rdparty/stout/include/stout/os/fork.hpp 8aa21ed 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp ccf80a7 3rdparty/libprocess/3rdparty/stout/include/stout/result.hpp ce8dd9b 3rdparty/libprocess/3rdparty/stout/include/stout/stringify.hpp ed0a1ef 3rdparty/libprocess/3rdparty/stout/include/stout/thread.hpp b1af74f 3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp 87c5fc8 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 9207c55 Diff: https://reviews.apache.org/r/26472/diff/ Testing --- make distcheck Thanks, Cody Maloney
Re: Review Request 26473: libprocess: Always use stout ABORT() rather than system abort()
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26473/#review55963 --- Ship it! Minor nit, question 3rdparty/libprocess/include/process/http.hpp https://reviews.apache.org/r/26473/#comment96319 Please align s together 3rdparty/libprocess/src/httpd.cpp https://reviews.apache.org/r/26473/#comment96320 Why are you removing this here? Doesn't seem related to the abort change. Or did you just notice that these are completely unused? - Adam B On Oct. 8, 2014, 6:20 p.m., Cody Maloney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26473/ --- (Updated Oct. 8, 2014, 6:20 p.m.) Review request for mesos, Adam B and Dominic Hamon. Bugs: MESOS-1870 https://issues.apache.org/jira/browse/MESOS-1870 Repository: mesos-git Description --- This makes it so any time there is an abort, we get a line number and at least a basic message as to why there was an abort. If you want a clean(er) exit, use stout/exit. Diffs - 3rdparty/libprocess/include/process/event.hpp bf689d7 3rdparty/libprocess/include/process/http.hpp d540775 3rdparty/libprocess/include/process/socket.hpp dbcb4f4 3rdparty/libprocess/src/httpd.cpp eab3aa5 3rdparty/libprocess/src/synchronized.hpp 70f6cd0 Diff: https://reviews.apache.org/r/26473/diff/ Testing --- make distcheck Thanks, Cody Maloney
Re: Review Request 26472: stout: Always use stout ABORT() rather than system abort()
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26472/#review55961
---
Ship it!
Please fix the shadowed variable and maybe the minor alignment nits.
3rdparty/libprocess/3rdparty/stout/include/stout/os/fork.hpp
https://reviews.apache.org/r/26472/#comment96318
Align with std::string
3rdparty/libprocess/3rdparty/stout/include/stout/result.hpp
https://reviews.apache.org/r/26472/#comment96315
Shadowed 'message' variable? How about a different name for the local
string?
3rdparty/libprocess/3rdparty/stout/include/stout/thread.hpp
https://reviews.apache.org/r/26472/#comment96317
Would prefer to see the second line aligned with std::string, after the
'ABORT('
Same for other changes in this file
- Adam B
On Oct. 8, 2014, 6:20 p.m., Cody Maloney wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26472/
---
(Updated Oct. 8, 2014, 6:20 p.m.)
Review request for mesos, Adam B and Dominic Hamon.
Bugs: MESOS-1870
https://issues.apache.org/jira/browse/MESOS-1870
Repository: mesos-git
Description
---
This makes it so any time there is an abort, we get a line number and at
least a basic message as to why there was an abort. If you want a clean(er)
exit, use stout/exit.
Also adds an overload which takes a standard string and unwraps it to a const
char * automatically, since a lot of the time we are building strings to pass
them to abort.
Diffs
-
3rdparty/libprocess/3rdparty/stout/include/stout/abort.hpp 6b5b5d1
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9d244b2
3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp 7138bc2
3rdparty/libprocess/3rdparty/stout/include/stout/os/fork.hpp 8aa21ed
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp ccf80a7
3rdparty/libprocess/3rdparty/stout/include/stout/result.hpp ce8dd9b
3rdparty/libprocess/3rdparty/stout/include/stout/stringify.hpp ed0a1ef
3rdparty/libprocess/3rdparty/stout/include/stout/thread.hpp b1af74f
3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp 87c5fc8
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 9207c55
Diff: https://reviews.apache.org/r/26472/diff/
Testing
---
make distcheck
Thanks,
Cody Maloney
Re: Review Request 26382: [WIP] Annotate TASK_LOST with source. Consider TASK_INVALID for some cases.
On Oct. 7, 2014, 6:50 p.m., Vinod Kone wrote:
OK. Here is a proposal for what it could look like.
General idea: We should add as few top level task states as possible
because it is more work for frameworks. TASK_LOST should be used for cases
where we expect a relaunch of the task would succeed (unfortunately this
principle breaks with reconciliation).
Add 2 new task states to TaskState
enum TaskState {
...
...
...,
TASK_UNAUTHORIZED, # Fold this into TASK_INVALID?
TASK_INVALID # Maybe use TASK_ERROR instead since it already exists but
unused?
}
We add 2 new fields, source and reason/code both enums, to TaskStatus
NOTE: We should take this opportunity to move task validations from
scheduler driver to master, to simplify. Maybe do this as first patch.
enum Source {
MASTER,
SLAVE,
EXECUTOR,
SCHEDULER, # Don't need this when we move validation to master.
}
Based on the different status updates, these are the reasons i came up
with. Let me know if you can't figure out which reason should be used where
:)
enum Reason {
# Set by master
INVALID_OFFERS,
SLAVE_REMOVED,
SLAVE_DISCONNECTED,
SLAVE_UKNOWN,
TASK_UNKNOWN,
# Set by scheduler driver for now. But we could kill this and expect
scheduler to not send launch tasks when it is disconnected?
MASTER_DISCONNECTED,
# Set by slave
GC_ERROR,
SLAVE_RESTARTED
EXECUTOR_TERMINATED,
}
Currently the Reason make sense for LOST updates generated by
master/slave. Executors might use this code for udpates they generate, but
it is upto the framework on how to interpret it. We could also consider
adding more reasons for TASK_INVALID/TASK_ERROR which is also generated by
master (e.g., TASK_UNAUTHORIZED could be a reason for TASK_INVALID).
Bill Farner wrote:
This looks good; i have one addendum: frameworks must not be allowed to
set status update fields in ways that conflict with the master/slave. i.e.
an executor should not be allowed to specify the `Source` (or if it does,
mesos should overwrite it).
Vinod Kone wrote:
yup. that definitely was on my mind :)
Looks good to me. We can also add `TASK_FAILED` reasons and `TASK_KILLED`
explanations to the `Reason` enum. Generally, my proposal is to use `Reason`
for all second-tier states.
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26382/#review55668
---
On Oct. 9, 2014, 1:18 a.m., Dominic Hamon wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26382/
---
(Updated Oct. 9, 2014, 1:18 a.m.)
Review request for mesos and Vinod Kone.
Bugs: MESOS-1, MESOS-1830 and MESOS-343
https://issues.apache.org/jira/browse/MESOS-1
https://issues.apache.org/jira/browse/MESOS-1830
https://issues.apache.org/jira/browse/MESOS-343
Repository: mesos-git
Description
---
Annotating every TASK_LOST with comments to open discussion.
If we add a 'source' field and consider adding TASK_INVALID i think it adds
much more information. I don't think the metrics would have to change as the
source matches the source file, I think. Unless I missed a subtlety. Ie, some
of the master TASK_LOST could be set to slave source, but i think it's
debatable.
Diffs
-
src/master/master.cpp f05275b00635cee82007ed851bba1cd30a7aa74f
src/sched/sched.cpp a37ed3d2e11035650b9bf0440fb87f9129d8
src/scheduler/scheduler.cpp fb88a3e029e97ba33eae5d50503be5ed9c9533e6
src/slave/slave.cpp e56dcbd80114730949a0d4b553470802a4d38281
Diff: https://reviews.apache.org/r/26382/diff/
Testing
---
Thanks,
Dominic Hamon
Re: Review Request 23912: Fix MESOS-947: Slave should properly handle a killTask() that arrives between runTask() and _runTask()
On Oct. 8, 2014, 10:42 a.m., Vinod Kone wrote: src/tests/slave_tests.cpp, lines 1071-1075 https://reviews.apache.org/r/23912/diff/6-7/?file=714593#file714593line1071 This has not been moved !? I have been searching through the previous review comments, but have not been able to determine which one you are referring to. However, I strongly suspect that you want this code to be moved downwards, after driver.launchTasks(), just before driver.killTask. Correct? On Oct. 8, 2014, 10:42 a.m., Vinod Kone wrote: src/slave/slave.cpp, line 1355 https://reviews.apache.org/r/23912/diff/6-7/?file=714590#file714590line1355 I missed this before, but you also should erase the executorid if this is the last task on this executor. additionally, you want to remove the framework is this is the last executor. see _runTask() for details. Yes, thanks! I was wondering about this, but not enough, not aware how eagerly the slave wants to remove its framework info :-) The next patch will support that. The fix will actually be located in _runTask(). - Bernd --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23912/#review55834 --- On Oct. 8, 2014, 3:57 a.m., Bernd Mathiske wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23912/ --- (Updated Oct. 8, 2014, 3:57 a.m.) Review request for mesos. Bugs: MESOS-947 https://issues.apache.org/jira/browse/MESOS-947 Repository: mesos-git Description --- Fixes MESOS-947 Slave should properly handle a killTask() that arrives between runTask() and _runTask(). Slave::killTask() did not check for task in question combination to be pending (i.e. Slave::runTask had happened, but Slave::_runTask had not yet) and then erroneously assumed that Slave::runTask() had not been executed. The task was then marked LOST instead of KILLED. But Slave::runTask had already scheduled Slave::_runTask to follow. Now the entry for being pending is removed, and the task is marked KILLED, and _runTask gets informed about this. It checks whether the task in question is currently pending and if it is not, then it infers that the task has been killed and does not erroneously try to complete launching it. Diffs - src/slave/slave.hpp 28697102047b972ecb3b6b627ee089b430549fc0 src/slave/slave.cpp e56dcbd80114730949a0d4b553470802a4d38281 src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 src/tests/mesos.cpp 3dcb2acd5ad4ab5e3a7b4fe524ee077558112773 src/tests/slave_tests.cpp 69be28f6e82b99e23424bd2be8294f715d8040d4 Diff: https://reviews.apache.org/r/23912/diff/ Testing --- Wrote a unit test that reliably created the situation described in the ticket. Observed that TASK_LOST and the listed log output occurred. This pointed directly to the lines in killTask() where the problem is rooted. Ran the test after fixing, it succeeded. Checked the log. It looks like a clean kill now :-) Thanks, Bernd Mathiske
Re: Review Request 26476: Remove dynamic allocation from Option.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26476/#review55977 --- 3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp https://reviews.apache.org/r/26476/#comment96328 std::unique_ptr would also be an option as it can be moved on Option copy. Would that be a less intrusive change? - Dominic Hamon On Oct. 8, 2014, 6:35 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26476/ --- (Updated Oct. 8, 2014, 6:35 p.m.) Review request for mesos, Benjamin Hindman and Niklas Nielsen. Repository: mesos-git Description --- Remove dynamic allocations from Option class. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 47fe92c Diff: https://reviews.apache.org/r/26476/diff/ Testing --- make check support/mesos-style.py valgrind (reduced allocation count) Thanks, Joris Van Remoortere
Re: Review Request 26382: [WIP] Annotate TASK_LOST with source. Consider TASK_INVALID for some cases.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26382/ --- (Updated Oct. 9, 2014, 7:07 a.m.) Review request for mesos and Vinod Kone. Changes --- remove MESOS-1 Bugs: MESOS-1830 and MESOS-343 https://issues.apache.org/jira/browse/MESOS-1830 https://issues.apache.org/jira/browse/MESOS-343 Repository: mesos-git Description --- Annotating every TASK_LOST with comments to open discussion. If we add a 'source' field and consider adding TASK_INVALID i think it adds much more information. I don't think the metrics would have to change as the source matches the source file, I think. Unless I missed a subtlety. Ie, some of the master TASK_LOST could be set to slave source, but i think it's debatable. Diffs - src/master/master.cpp f05275b00635cee82007ed851bba1cd30a7aa74f src/sched/sched.cpp a37ed3d2e11035650b9bf0440fb87f9129d8 src/scheduler/scheduler.cpp fb88a3e029e97ba33eae5d50503be5ed9c9533e6 src/slave/slave.cpp e56dcbd80114730949a0d4b553470802a4d38281 Diff: https://reviews.apache.org/r/26382/diff/ Testing --- Thanks, Dominic Hamon
Re: Review Request 23912: Fix MESOS-947: Slave should properly handle a killTask() that arrives between runTask() and _runTask()
On Aug. 5, 2014, 2:29 p.m., Vinod Kone wrote: src/tests/slave_tests.cpp, lines 958-962 https://reviews.apache.org/r/23912/diff/3/?file=643815#file643815line958 Kill these expectations since an executor won't be launched in this test. Vinod Kone wrote: doesnt look like this was fixed. Bernd Mathiske wrote: I will take off the 'registered' expectation, but I prefer keeping the second one to make sure the test is tight. That includes that launchTask should not happen by accident. Turns out the executor should not even register. I have that expectation in there, too, now, and I believe it is for the best. The AtMost(1) above was wrong. Should be 0, and is now. - Bernd --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23912/#review49552 --- On Oct. 8, 2014, 3:57 a.m., Bernd Mathiske wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23912/ --- (Updated Oct. 8, 2014, 3:57 a.m.) Review request for mesos. Bugs: MESOS-947 https://issues.apache.org/jira/browse/MESOS-947 Repository: mesos-git Description --- Fixes MESOS-947 Slave should properly handle a killTask() that arrives between runTask() and _runTask(). Slave::killTask() did not check for task in question combination to be pending (i.e. Slave::runTask had happened, but Slave::_runTask had not yet) and then erroneously assumed that Slave::runTask() had not been executed. The task was then marked LOST instead of KILLED. But Slave::runTask had already scheduled Slave::_runTask to follow. Now the entry for being pending is removed, and the task is marked KILLED, and _runTask gets informed about this. It checks whether the task in question is currently pending and if it is not, then it infers that the task has been killed and does not erroneously try to complete launching it. Diffs - src/slave/slave.hpp 28697102047b972ecb3b6b627ee089b430549fc0 src/slave/slave.cpp e56dcbd80114730949a0d4b553470802a4d38281 src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 src/tests/mesos.cpp 3dcb2acd5ad4ab5e3a7b4fe524ee077558112773 src/tests/slave_tests.cpp 69be28f6e82b99e23424bd2be8294f715d8040d4 Diff: https://reviews.apache.org/r/23912/diff/ Testing --- Wrote a unit test that reliably created the situation described in the ticket. Observed that TASK_LOST and the listed log output occurred. This pointed directly to the lines in killTask() where the problem is rooted. Ran the test after fixing, it succeeded. Checked the log. It looks like a clean kill now :-) Thanks, Bernd Mathiske
Re: Review Request 23912: Fix MESOS-947: Slave should properly handle a killTask() that arrives between runTask() and _runTask()
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23912/ --- (Updated Oct. 9, 2014, 7:10 a.m.) Review request for mesos. Changes --- Now not only the killed taks is removed, but also its executor's id and the whole framework. Added a mock method and a check in the test to verify that removeFramework does get called. Bugs: MESOS-947 https://issues.apache.org/jira/browse/MESOS-947 Repository: mesos-git Description --- Fixes MESOS-947 Slave should properly handle a killTask() that arrives between runTask() and _runTask(). Slave::killTask() did not check for task in question combination to be pending (i.e. Slave::runTask had happened, but Slave::_runTask had not yet) and then erroneously assumed that Slave::runTask() had not been executed. The task was then marked LOST instead of KILLED. But Slave::runTask had already scheduled Slave::_runTask to follow. Now the entry for being pending is removed, and the task is marked KILLED, and _runTask gets informed about this. It checks whether the task in question is currently pending and if it is not, then it infers that the task has been killed and does not erroneously try to complete launching it. Diffs (updated) - src/slave/slave.hpp 76d505c698774204b2536b66ea8a83a9a2a5e2c1 src/slave/slave.cpp cb3759993f863590cb1545c73072feb0331aa6c9 src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 src/tests/mesos.cpp 3dcb2acd5ad4ab5e3a7b4fe524ee077558112773 src/tests/slave_tests.cpp 69be28f6e82b99e23424bd2be8294f715d8040d4 Diff: https://reviews.apache.org/r/23912/diff/ Testing --- Wrote a unit test that reliably created the situation described in the ticket. Observed that TASK_LOST and the listed log output occurred. This pointed directly to the lines in killTask() where the problem is rooted. Ran the test after fixing, it succeeded. Checked the log. It looks like a clean kill now :-) Thanks, Bernd Mathiske
Re: Review Request 23912: Fix MESOS-947: Slave should properly handle a killTask() that arrives between runTask() and _runTask()
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23912/#review55993 --- Patch looks great! Reviews applied: [23912] All tests passed. - Mesos ReviewBot On Oct. 9, 2014, 2:10 p.m., Bernd Mathiske wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23912/ --- (Updated Oct. 9, 2014, 2:10 p.m.) Review request for mesos. Bugs: MESOS-947 https://issues.apache.org/jira/browse/MESOS-947 Repository: mesos-git Description --- Fixes MESOS-947 Slave should properly handle a killTask() that arrives between runTask() and _runTask(). Slave::killTask() did not check for task in question combination to be pending (i.e. Slave::runTask had happened, but Slave::_runTask had not yet) and then erroneously assumed that Slave::runTask() had not been executed. The task was then marked LOST instead of KILLED. But Slave::runTask had already scheduled Slave::_runTask to follow. Now the entry for being pending is removed, and the task is marked KILLED, and _runTask gets informed about this. It checks whether the task in question is currently pending and if it is not, then it infers that the task has been killed and does not erroneously try to complete launching it. Diffs - src/slave/slave.hpp 76d505c698774204b2536b66ea8a83a9a2a5e2c1 src/slave/slave.cpp cb3759993f863590cb1545c73072feb0331aa6c9 src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 src/tests/mesos.cpp 3dcb2acd5ad4ab5e3a7b4fe524ee077558112773 src/tests/slave_tests.cpp 69be28f6e82b99e23424bd2be8294f715d8040d4 Diff: https://reviews.apache.org/r/23912/diff/ Testing --- Wrote a unit test that reliably created the situation described in the ticket. Observed that TASK_LOST and the listed log output occurred. This pointed directly to the lines in killTask() where the problem is rooted. Ran the test after fixing, it succeeded. Checked the log. It looks like a clean kill now :-) Thanks, Bernd Mathiske
[GitHub] mesos pull request: Added option to enable privileged mode for Doc...
Github user minid33 commented on the pull request: https://github.com/apache/mesos/pull/26#issuecomment-58530671 @kmatzen Looks like this is the way your code enters an Apache project, I'd love for this change to go in, I need it a lot. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
Re: Review Request 26436: Avoid docker inspect on each usage call
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26436/ --- (Updated Oct. 9, 2014, 3:56 p.m.) Review request for mesos and Benjamin Hindman. Repository: mesos-git Description --- Review: https://reviews.apache.org/r/26436 Diffs (updated) - src/slave/containerizer/docker.cpp 9a2948951f57f3ab16291df51cd9f33e5e96add4 Diff: https://reviews.apache.org/r/26436/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 26436: Avoid docker inspect on each usage call
On Oct. 9, 2014, 7:07 a.m., Michael Park wrote: LGTM, leaving `Ship It`s for committers. Thanks for reviewing! You did catch very good spots. - Timothy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26436/#review55956 --- On Oct. 9, 2014, 3:56 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26436/ --- (Updated Oct. 9, 2014, 3:56 p.m.) Review request for mesos and Benjamin Hindman. Repository: mesos-git Description --- Review: https://reviews.apache.org/r/26436 Diffs - src/slave/containerizer/docker.cpp 9a2948951f57f3ab16291df51cd9f33e5e96add4 Diff: https://reviews.apache.org/r/26436/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 26229: Expose poll interval from the reaper.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26229/ --- (Updated Oct. 9, 2014, 4:34 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, and Till Toenshoff. Changes --- Rebase. Repository: mesos-git Description --- Lower and upper bounds for the poll interval are refactored as static functions visible to outer world. Diffs (updated) - 3rdparty/libprocess/include/process/reap.hpp 9de5336 3rdparty/libprocess/src/reap.cpp ac14a86 Diff: https://reviews.apache.org/r/26229/diff/ Testing --- make check (Mac OS 10.9.4, Ubuntu 14.04) Thanks, Alexander Rukletsov
Re: Review Request 26472: stout: Always use stout ABORT() rather than system abort()
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26472/ --- (Updated Oct. 9, 2014, 4:35 p.m.) Review request for mesos, Adam B and Dominic Hamon. Changes --- Update for Adam and Dominic's comments. Unshadow 'message' member variable Align ABORT() messages Bugs: MESOS-1870 https://issues.apache.org/jira/browse/MESOS-1870 Repository: mesos-git Description --- This makes it so any time there is an abort, we get a line number and at least a basic message as to why there was an abort. If you want a clean(er) exit, use stout/exit. Also adds an overload which takes a standard string and unwraps it to a const char * automatically, since a lot of the time we are building strings to pass them to abort. Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/abort.hpp 6b5b5d1faa488cb3048f6d59bae17123b1b05a33 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9d244b2275f7ef84e8c486f2090b67a57ca8c670 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp 7138bc24912f35bfd04d2341b2218ea349ab40b8 3rdparty/libprocess/3rdparty/stout/include/stout/os/fork.hpp 8aa21ed10293c3dd7f55b7d30f6014bd05fd7455 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp ccf80a771c59092cb72d4dac0d868eec7df2f088 3rdparty/libprocess/3rdparty/stout/include/stout/result.hpp ce8dd9b6468fb36daceba60d8b43bf2fbfceab6c 3rdparty/libprocess/3rdparty/stout/include/stout/stringify.hpp ed0a1ef2b617bd87261bf4836706cedb80fdf043 3rdparty/libprocess/3rdparty/stout/include/stout/thread.hpp b1af74f517da75525d0dedacaf4a1c6f8cbf55f4 3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp 87c5fc8391d5213f0d76fa5980176726a0366f56 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 9207c551170b747d25207efe7d3c03675b184953 Diff: https://reviews.apache.org/r/26472/diff/ Testing --- make distcheck Thanks, Cody Maloney
Re: Review Request 26473: libprocess: Always use stout ABORT() rather than system abort()
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26473/ --- (Updated Oct. 9, 2014, 4:38 p.m.) Review request for mesos, Adam B and Dominic Hamon. Changes --- Align ABORT() messages Bugs: MESOS-1870 https://issues.apache.org/jira/browse/MESOS-1870 Repository: mesos-git Description --- This makes it so any time there is an abort, we get a line number and at least a basic message as to why there was an abort. If you want a clean(er) exit, use stout/exit. Diffs (updated) - 3rdparty/libprocess/include/process/event.hpp bf689d7270df2c8f1f5c9165d2bbcfdca06e15a8 3rdparty/libprocess/include/process/http.hpp d5407755a51a6edf779b2d219b4d81a90c3af2f8 3rdparty/libprocess/include/process/socket.hpp dbcb4f4c2eb12663158057a844b4511d6dde0508 3rdparty/libprocess/src/httpd.cpp eab3aa5f1c74cfc211b0efcc40f984222c85785c 3rdparty/libprocess/src/synchronized.hpp 70f6cd06825ac7bde5e45f2a900d2b2659e02b6e Diff: https://reviews.apache.org/r/26473/diff/ Testing --- make distcheck Thanks, Cody Maloney
Re: Review Request 26472: stout: Always use stout ABORT() rather than system abort()
On Oct. 8, 2014, 8:49 p.m., Dominic Hamon wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/stringify.hpp, line 36 https://reviews.apache.org/r/26472/diff/1/?file=716305#file716305line36 it might be worth considering using stringstream here to build the same error message we had before. we will have a stack trace, but having the thing that was attempting to stringify could be useful too, i think. Adam B wrote: +1 on having the thing that was attempting to stringify could be useful Cody Maloney wrote: The thing is we would be doing the same operation which just failed in order to try to print out T again. So in the failure case (Which is unlikely to fail unless you are OOM), we have the operation fail, then try the failed thing again in order to provide debug information. I'm planning to make ABORT() give backtraces on debug builds, which should provide a lot more information than this ever possibly did, without retrying something we know is likely to fail because it just did. stringify would fail again, but we used to use ostream operators. We should then be able to use stringstream to get some representation of t. - Dominic --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26472/#review55946 --- On Oct. 9, 2014, 9:35 a.m., Cody Maloney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26472/ --- (Updated Oct. 9, 2014, 9:35 a.m.) Review request for mesos, Adam B and Dominic Hamon. Bugs: MESOS-1870 https://issues.apache.org/jira/browse/MESOS-1870 Repository: mesos-git Description --- This makes it so any time there is an abort, we get a line number and at least a basic message as to why there was an abort. If you want a clean(er) exit, use stout/exit. Also adds an overload which takes a standard string and unwraps it to a const char * automatically, since a lot of the time we are building strings to pass them to abort. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/abort.hpp 6b5b5d1faa488cb3048f6d59bae17123b1b05a33 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9d244b2275f7ef84e8c486f2090b67a57ca8c670 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp 7138bc24912f35bfd04d2341b2218ea349ab40b8 3rdparty/libprocess/3rdparty/stout/include/stout/os/fork.hpp 8aa21ed10293c3dd7f55b7d30f6014bd05fd7455 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp ccf80a771c59092cb72d4dac0d868eec7df2f088 3rdparty/libprocess/3rdparty/stout/include/stout/result.hpp ce8dd9b6468fb36daceba60d8b43bf2fbfceab6c 3rdparty/libprocess/3rdparty/stout/include/stout/stringify.hpp ed0a1ef2b617bd87261bf4836706cedb80fdf043 3rdparty/libprocess/3rdparty/stout/include/stout/thread.hpp b1af74f517da75525d0dedacaf4a1c6f8cbf55f4 3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp 87c5fc8391d5213f0d76fa5980176726a0366f56 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 9207c551170b747d25207efe7d3c03675b184953 Diff: https://reviews.apache.org/r/26472/diff/ Testing --- make distcheck Thanks, Cody Maloney
Re: Review Request 26436: Avoid docker inspect on each usage call
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26436/#review56000 --- Patch looks great! Reviews applied: [26436] All tests passed. - Mesos ReviewBot On Oct. 9, 2014, 3:56 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26436/ --- (Updated Oct. 9, 2014, 3:56 p.m.) Review request for mesos and Benjamin Hindman. Repository: mesos-git Description --- Review: https://reviews.apache.org/r/26436 Diffs - src/slave/containerizer/docker.cpp 9a2948951f57f3ab16291df51cd9f33e5e96add4 Diff: https://reviews.apache.org/r/26436/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 26472: stout: Always use stout ABORT() rather than system abort()
On Oct. 9, 2014, 3:49 a.m., Dominic Hamon wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/stringify.hpp, line 36 https://reviews.apache.org/r/26472/diff/1/?file=716305#file716305line36 it might be worth considering using stringstream here to build the same error message we had before. we will have a stack trace, but having the thing that was attempting to stringify could be useful too, i think. Adam B wrote: +1 on having the thing that was attempting to stringify could be useful Cody Maloney wrote: The thing is we would be doing the same operation which just failed in order to try to print out T again. So in the failure case (Which is unlikely to fail unless you are OOM), we have the operation fail, then try the failed thing again in order to provide debug information. I'm planning to make ABORT() give backtraces on debug builds, which should provide a lot more information than this ever possibly did, without retrying something we know is likely to fail because it just did. Dominic Hamon wrote: stringify would fail again, but we used to use ostream operators. We should then be able to use stringstream to get some representation of t. I can do typeid(t).name() which would give me the mangled name and use libcxxabi (Available on all of our platforms), to demangle it to the human-friendly C++ type name, But that is a lot of utility library to add for little benefit. - Cody --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26472/#review55946 --- On Oct. 9, 2014, 4:35 p.m., Cody Maloney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26472/ --- (Updated Oct. 9, 2014, 4:35 p.m.) Review request for mesos, Adam B and Dominic Hamon. Bugs: MESOS-1870 https://issues.apache.org/jira/browse/MESOS-1870 Repository: mesos-git Description --- This makes it so any time there is an abort, we get a line number and at least a basic message as to why there was an abort. If you want a clean(er) exit, use stout/exit. Also adds an overload which takes a standard string and unwraps it to a const char * automatically, since a lot of the time we are building strings to pass them to abort. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/abort.hpp 6b5b5d1faa488cb3048f6d59bae17123b1b05a33 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9d244b2275f7ef84e8c486f2090b67a57ca8c670 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp 7138bc24912f35bfd04d2341b2218ea349ab40b8 3rdparty/libprocess/3rdparty/stout/include/stout/os/fork.hpp 8aa21ed10293c3dd7f55b7d30f6014bd05fd7455 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp ccf80a771c59092cb72d4dac0d868eec7df2f088 3rdparty/libprocess/3rdparty/stout/include/stout/result.hpp ce8dd9b6468fb36daceba60d8b43bf2fbfceab6c 3rdparty/libprocess/3rdparty/stout/include/stout/stringify.hpp ed0a1ef2b617bd87261bf4836706cedb80fdf043 3rdparty/libprocess/3rdparty/stout/include/stout/thread.hpp b1af74f517da75525d0dedacaf4a1c6f8cbf55f4 3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp 87c5fc8391d5213f0d76fa5980176726a0366f56 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 9207c551170b747d25207efe7d3c03675b184953 Diff: https://reviews.apache.org/r/26472/diff/ Testing --- make distcheck Thanks, Cody Maloney
Re: Review Request 26472: stout: Always use stout ABORT() rather than system abort()
On Oct. 8, 2014, 8:49 p.m., Dominic Hamon wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/stringify.hpp, line 36 https://reviews.apache.org/r/26472/diff/1/?file=716305#file716305line36 it might be worth considering using stringstream here to build the same error message we had before. we will have a stack trace, but having the thing that was attempting to stringify could be useful too, i think. Adam B wrote: +1 on having the thing that was attempting to stringify could be useful Cody Maloney wrote: The thing is we would be doing the same operation which just failed in order to try to print out T again. So in the failure case (Which is unlikely to fail unless you are OOM), we have the operation fail, then try the failed thing again in order to provide debug information. I'm planning to make ABORT() give backtraces on debug builds, which should provide a lot more information than this ever possibly did, without retrying something we know is likely to fail because it just did. Dominic Hamon wrote: stringify would fail again, but we used to use ostream operators. We should then be able to use stringstream to get some representation of t. Cody Maloney wrote: I can do typeid(t).name() which would give me the mangled name and use libcxxabi (Available on all of our platforms), to demangle it to the human-friendly C++ type name, But that is a lot of utility library to add for little benefit. std::ostringstream os; os t; ABORT(Failed to stringify: + os.str()); is the equivalent of what was there. - Dominic --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26472/#review55946 --- On Oct. 9, 2014, 9:35 a.m., Cody Maloney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26472/ --- (Updated Oct. 9, 2014, 9:35 a.m.) Review request for mesos, Adam B and Dominic Hamon. Bugs: MESOS-1870 https://issues.apache.org/jira/browse/MESOS-1870 Repository: mesos-git Description --- This makes it so any time there is an abort, we get a line number and at least a basic message as to why there was an abort. If you want a clean(er) exit, use stout/exit. Also adds an overload which takes a standard string and unwraps it to a const char * automatically, since a lot of the time we are building strings to pass them to abort. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/abort.hpp 6b5b5d1faa488cb3048f6d59bae17123b1b05a33 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9d244b2275f7ef84e8c486f2090b67a57ca8c670 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp 7138bc24912f35bfd04d2341b2218ea349ab40b8 3rdparty/libprocess/3rdparty/stout/include/stout/os/fork.hpp 8aa21ed10293c3dd7f55b7d30f6014bd05fd7455 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp ccf80a771c59092cb72d4dac0d868eec7df2f088 3rdparty/libprocess/3rdparty/stout/include/stout/result.hpp ce8dd9b6468fb36daceba60d8b43bf2fbfceab6c 3rdparty/libprocess/3rdparty/stout/include/stout/stringify.hpp ed0a1ef2b617bd87261bf4836706cedb80fdf043 3rdparty/libprocess/3rdparty/stout/include/stout/thread.hpp b1af74f517da75525d0dedacaf4a1c6f8cbf55f4 3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp 87c5fc8391d5213f0d76fa5980176726a0366f56 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 9207c551170b747d25207efe7d3c03675b184953 Diff: https://reviews.apache.org/r/26472/diff/ Testing --- make distcheck Thanks, Cody Maloney
Review Request 26508: Added --module flag for Mesos master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26508/ --- Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Niklas Nielsen. Repository: mesos-git Description --- Added --module flag for Mesos master. Diffs - src/common/parse.hpp e6153d8a1f25bc9ddbe1e391306beeacfc8d5ff6 src/common/type_utils.hpp 480c0883fe6ed7f6a9daf77d83ebb077da2e66ee src/master/flags.hpp b8b59a245523c7e7d4e04f86801ed7b023a129ea src/master/main.cpp 018f8036b742f0b083445e03dca5c06c13d80e68 src/module/manager.hpp 8275fd0c2f0c9ba92a9ad47f7d5d2df9295fa184 Diff: https://reviews.apache.org/r/26508/diff/ Testing --- make check Thanks, Kapil Arya
Review Request 26509: Added --module flag for Mesos slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26509/ --- Review request for mesos and Niklas Nielsen. Repository: mesos-git Description --- Added --module flag for Mesos slave. Diffs - src/slave/flags.hpp 16f0cc2ab5ba16a39499608174278b3082e0585d src/slave/main.cpp 2c4d365a04acbcb382e978d811a318130484b3d5 Diff: https://reviews.apache.org/r/26509/diff/ Testing --- Thanks, Kapil Arya
Re: Review Request 26476: Remove dynamic allocation from Option.
On Oct. 9, 2014, 2:05 p.m., Dominic Hamon wrote:
3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp, line 131
https://reviews.apache.org/r/26476/diff/1/?file=716336#file716336line131
std::unique_ptr would also be an option as it can be moved on Option
copy. Would that be a less intrusive change?
That would effectively make Option a pointer to a pointer... Better would be to
do something like:
```
class Option : std::unique_ptr {
...
}
```
But overall making the optional thing live in place is preferable. In the mesos
codebase there aren't any really large objects (Objects with a lot of members,
or large in-place array members).
Anything that grows / tends to get really large (We move about some big
strings), all have external storage from the base class, and this object stays
small.
In terms of perf, linear copies of an object in place are fast, and practically
copying around the object a whole lot is waayyy faster than if we have a single
cache miss looking up the pointer. If you want a real world example of this,
std::vector is almost always faster than std::list, unless you have very large
objects
(http://baptiste-wicht.com/posts/2012/12/cpp-benchmark-vector-list-deque.html)
- Cody
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26476/#review55977
---
On Oct. 9, 2014, 1:35 a.m., Joris Van Remoortere wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26476/
---
(Updated Oct. 9, 2014, 1:35 a.m.)
Review request for mesos, Benjamin Hindman and Niklas Nielsen.
Repository: mesos-git
Description
---
Remove dynamic allocations from Option class.
Diffs
-
3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 47fe92c
Diff: https://reviews.apache.org/r/26476/diff/
Testing
---
make check
support/mesos-style.py
valgrind (reduced allocation count)
Thanks,
Joris Van Remoortere
Re: Review Request 26508: Added --module flag for Mesos master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26508/#review56013 --- src/master/flags.hpp https://reviews.apache.org/r/26508/#comment96366 s/The value could be a/A/ ? src/master/flags.hpp https://reviews.apache.org/r/26508/#comment96367 .. that will be loaded and accessible to augment mesos subsystems? Something that describes what happens to the input :-) src/master/flags.hpp https://reviews.apache.org/r/26508/#comment96372 Or else what? Modules gets overwritten right? Or maybe just mentioned that you 'must not'? src/module/manager.hpp https://reviews.apache.org/r/26508/#comment96373 Is this just a fly-by style fix? Why include module.hpp here? - Niklas Nielsen On Oct. 9, 2014, 10:19 a.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26508/ --- (Updated Oct. 9, 2014, 10:19 a.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Niklas Nielsen. Repository: mesos-git Description --- Added --module flag for Mesos master. Diffs - src/common/parse.hpp e6153d8a1f25bc9ddbe1e391306beeacfc8d5ff6 src/common/type_utils.hpp 480c0883fe6ed7f6a9daf77d83ebb077da2e66ee src/master/flags.hpp b8b59a245523c7e7d4e04f86801ed7b023a129ea src/master/main.cpp 018f8036b742f0b083445e03dca5c06c13d80e68 src/module/manager.hpp 8275fd0c2f0c9ba92a9ad47f7d5d2df9295fa184 Diff: https://reviews.apache.org/r/26508/diff/ Testing --- make check Thanks, Kapil Arya
Re: Review Request 26509: Added --module flag for Mesos slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26509/#review56024 --- src/slave/flags.hpp https://reviews.apache.org/r/26509/#comment96383 Let's get the master flags help text solidified and update this accordingly. - Niklas Nielsen On Oct. 9, 2014, 10:20 a.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26509/ --- (Updated Oct. 9, 2014, 10:20 a.m.) Review request for mesos and Niklas Nielsen. Repository: mesos-git Description --- Added --module flag for Mesos slave. Diffs - src/slave/flags.hpp 16f0cc2ab5ba16a39499608174278b3082e0585d src/slave/main.cpp 2c4d365a04acbcb382e978d811a318130484b3d5 Diff: https://reviews.apache.org/r/26509/diff/ Testing --- Thanks, Kapil Arya
Review Request 26513: Disallow duplicate module names.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26513/ --- Review request for mesos and Niklas Nielsen. Repository: mesos-git Description --- Module manager should return error on encountering a duplicate module name (i.e. if a module with the same name has already been loaded). Diffs - src/module/manager.cpp 72041c06b28191dea244a39ba99666e53198decc src/tests/module_tests.cpp 6f9ee33f2f2d41dcc5bde9ef6326186f56e7c7fc Diff: https://reviews.apache.org/r/26513/diff/ Testing --- Added a test with duplicate modules; ran make check. Thanks, Kapil Arya
Re: Review Request 26508: Added --module flag for Mesos master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26508/ --- (Updated Oct. 9, 2014, 3:05 p.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Niklas Nielsen. Changes --- Updated help message. Repository: mesos-git Description --- Added --module flag for Mesos master. Diffs (updated) - src/common/parse.hpp e6153d8a1f25bc9ddbe1e391306beeacfc8d5ff6 src/common/type_utils.hpp 480c0883fe6ed7f6a9daf77d83ebb077da2e66ee src/master/flags.hpp b8b59a245523c7e7d4e04f86801ed7b023a129ea src/master/main.cpp 018f8036b742f0b083445e03dca5c06c13d80e68 src/module/manager.hpp 8275fd0c2f0c9ba92a9ad47f7d5d2df9295fa184 Diff: https://reviews.apache.org/r/26508/diff/ Testing --- make check Thanks, Kapil Arya
Re: Review Request 26509: Added --module flag for Mesos slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26509/ --- (Updated Oct. 9, 2014, 3:07 p.m.) Review request for mesos and Niklas Nielsen. Changes --- Update help message. Repository: mesos-git Description --- Added --module flag for Mesos slave. Diffs (updated) - src/slave/flags.hpp 16f0cc2ab5ba16a39499608174278b3082e0585d src/slave/main.cpp 2c4d365a04acbcb382e978d811a318130484b3d5 Diff: https://reviews.apache.org/r/26509/diff/ Testing --- Thanks, Kapil Arya
Re: Review Request 26229: Expose poll interval from the reaper.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26229/#review56038 --- Patch looks great! Reviews applied: [26229] All tests passed. - Mesos ReviewBot On Oct. 9, 2014, 4:34 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26229/ --- (Updated Oct. 9, 2014, 4:34 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, and Till Toenshoff. Repository: mesos-git Description --- Lower and upper bounds for the poll interval are refactored as static functions visible to outer world. Diffs - 3rdparty/libprocess/include/process/reap.hpp 9de5336 3rdparty/libprocess/src/reap.cpp ac14a86 Diff: https://reviews.apache.org/r/26229/diff/ Testing --- make check (Mac OS 10.9.4, Ubuntu 14.04) Thanks, Alexander Rukletsov
Re: Review Request 25250: Mark running tasks killed during framework shutdown.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25250/#review56041 --- src/tests/master_tests.cpp https://reviews.apache.org/r/25250/#comment96393 Perhaps you should do EXPECT so you can cleanly shutdown in the end. - Timothy Chen On Oct. 9, 2014, 5:05 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25250/ --- (Updated Oct. 9, 2014, 5:05 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, and Till Toenshoff. Bugs: MESOS-1736 https://issues.apache.org/jira/browse/MESOS-1736 Repository: mesos-git Description --- When a framework is shut down e.g. by calling driver.stop() from the scheduler, running tasks are marked KILLED before migrating them to completed. Diffs - src/master/master.cpp cb46cec src/tests/master_tests.cpp d9dc40c Diff: https://reviews.apache.org/r/25250/diff/ Testing --- make check (OS X) Thanks, Alexander Rukletsov
Re: Review Request 26513: Disallow duplicate module names.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26513/ --- (Updated Oct. 9, 2014, 3:59 p.m.) Review request for mesos and Niklas Nielsen. Changes --- Fixed typos and updated test comment. Repository: mesos-git Description --- Module manager should return error on encountering a duplicate module name (i.e. if a module with the same name has already been loaded). Diffs (updated) - src/module/manager.cpp 72041c06b28191dea244a39ba99666e53198decc src/tests/module_tests.cpp 6f9ee33f2f2d41dcc5bde9ef6326186f56e7c7fc Diff: https://reviews.apache.org/r/26513/diff/ Testing --- Added a test with duplicate modules; ran make check. Thanks, Kapil Arya
Re: Review Request 26513: Disallow duplicate module names.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26513/#review56043 --- Ship it! Looks good! Will get this committed for you shortly - Niklas Nielsen On Oct. 9, 2014, 12:59 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26513/ --- (Updated Oct. 9, 2014, 12:59 p.m.) Review request for mesos and Niklas Nielsen. Repository: mesos-git Description --- Module manager should return error on encountering a duplicate module name (i.e. if a module with the same name has already been loaded). Diffs - src/module/manager.cpp 72041c06b28191dea244a39ba99666e53198decc src/tests/module_tests.cpp 6f9ee33f2f2d41dcc5bde9ef6326186f56e7c7fc Diff: https://reviews.apache.org/r/26513/diff/ Testing --- Added a test with duplicate modules; ran make check. Thanks, Kapil Arya
Re: Review Request 25551: Add standard versioning to shared libmesos.so
On Sept. 26, 2014, 5:22 p.m., Vinod Kone wrote: src/Makefile.am, line 567 https://reviews.apache.org/r/25551/diff/2/?file=706149#file706149line567 why not just libmesos_la_LDFLAGS = -version-info $(PACKAGE-VERSION) More importantly how do the resulting dylibs (symlink and regular) look like compared to the previous style? Is it backwards compatible or needs re-linking? Timothy St. Clair wrote: Right now it would work, but it will likely shift. It would need relinking. Given that the semantics may shift, perhaps the cleanest thing to do it mark a comment in both configure.ac as well as Makefile.am - Timothy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25551/#review54699 --- On Sept. 26, 2014, 3:31 p.m., Timothy St. Clair wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25551/ --- (Updated Sept. 26, 2014, 3:31 p.m.) Review request for mesos, Jie Yu and Vinod Kone. Bugs: MESOS-1675 https://issues.apache.org/jira/browse/MESOS-1675 Repository: mesos-git Description --- Add standard -version-info to shared libmesos, it will need to be updated on major modifications. Diffs - configure.ac 86d448c src/Makefile.am 27c42df Diff: https://reviews.apache.org/r/25551/diff/ Testing --- make check Thanks, Timothy St. Clair
Re: Review Request 26473: libprocess: Always use stout ABORT() rather than system abort()
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26473/#review56053 --- Patch looks great! Reviews applied: [26472, 26473] All tests passed. - Mesos ReviewBot On Oct. 9, 2014, 4:38 p.m., Cody Maloney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26473/ --- (Updated Oct. 9, 2014, 4:38 p.m.) Review request for mesos, Adam B and Dominic Hamon. Bugs: MESOS-1870 https://issues.apache.org/jira/browse/MESOS-1870 Repository: mesos-git Description --- This makes it so any time there is an abort, we get a line number and at least a basic message as to why there was an abort. If you want a clean(er) exit, use stout/exit. Diffs - 3rdparty/libprocess/include/process/event.hpp bf689d7270df2c8f1f5c9165d2bbcfdca06e15a8 3rdparty/libprocess/include/process/http.hpp d5407755a51a6edf779b2d219b4d81a90c3af2f8 3rdparty/libprocess/include/process/socket.hpp dbcb4f4c2eb12663158057a844b4511d6dde0508 3rdparty/libprocess/src/httpd.cpp eab3aa5f1c74cfc211b0efcc40f984222c85785c 3rdparty/libprocess/src/synchronized.hpp 70f6cd06825ac7bde5e45f2a900d2b2659e02b6e Diff: https://reviews.apache.org/r/26473/diff/ Testing --- make distcheck Thanks, Cody Maloney
Re: Review Request 26487: Perform read right after subprocess for docker ps
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26487/#review56057 --- Ah, I suspected this would happen.. =/ https://issues.apache.org/jira/browse/MESOS-1336 Should consider making the Subprocess API less error prone in the long term. Just a drive by comment for the Subprocess API, I will let ben review this. - Ben Mahler On Oct. 9, 2014, 6:03 a.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26487/ --- (Updated Oct. 9, 2014, 6:03 a.m.) Review request for mesos and Benjamin Hindman. Repository: mesos-git Description --- Perform read right after subprocess for docker ps Diffs - src/docker/docker.hpp 7972edaf3038e70b6a5ed369f9b8de0c644fc4b0 src/docker/docker.cpp 20be6cd4649e12ed6f7d5d1991b5040342c56f35 Diff: https://reviews.apache.org/r/26487/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 26487: Perform read right after subprocess for docker ps
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26487/ --- (Updated Oct. 9, 2014, 9:38 p.m.) Review request for mesos and Benjamin Hindman. Bugs: MESOS-1885 https://issues.apache.org/jira/browse/MESOS-1885 Repository: mesos-git Description --- Perform read right after subprocess for docker ps Diffs - src/docker/docker.hpp 7972edaf3038e70b6a5ed369f9b8de0c644fc4b0 src/docker/docker.cpp 20be6cd4649e12ed6f7d5d1991b5040342c56f35 Diff: https://reviews.apache.org/r/26487/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 26517: Symlink sandbox directories in docker containerizer
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26517/#review56059 --- Bad patch! Reviews applied: [26517] Failed command: ./support/mesos-style.py Error: Checking 519 files using filter --filter=-,+build/class,+build/deprecated,+build/endif_comment,+readability/todo,+readability/namespace,+runtime/vlog,+whitespace/blank_line,+whitespace/comma,+whitespace/end_of_line,+whitespace/ending_newline,+whitespace/forcolon,+whitespace/indent,+whitespace/line_length,+whitespace/tab,+whitespace/todo src/tests/docker_containerizer_tests.cpp:110: Redundant blank line at the end of a code block should be deleted. [whitespace/blank_line] [3] Total errors found: 1 - Mesos ReviewBot On Oct. 9, 2014, 8:15 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26517/ --- (Updated Oct. 9, 2014, 8:15 p.m.) Review request for mesos and Benjamin Hindman. Repository: mesos-git Description --- Symlink sandbox directories in docker containerizer Diffs - src/slave/containerizer/docker.hpp fbbd45d77e5f2f74ca893552f85eb893b3dd948f src/slave/containerizer/docker.cpp 9a2948951f57f3ab16291df51cd9f33e5e96add4 src/tests/docker_containerizer_tests.cpp 67d60a885d65edbcbbf702bce83a54d1a5c0411f Diff: https://reviews.apache.org/r/26517/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 25622: Update the Mesos Style Guide with C++11 and naming notes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25622/#review56058 --- Ship it! Ok, looks good to me, modulo the comment. docs/mesos-c++-style-guide.md https://reviews.apache.org/r/25622/#comment96407 Why 'valueIt' here but 'i' below? How about 'i' in both cases? - Ben Mahler On Oct. 9, 2014, 4:31 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25622/ --- (Updated Oct. 9, 2014, 4:31 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Dominic Hamon, and Till Toenshoff. Bugs: MESOS-1793 https://issues.apache.org/jira/browse/MESOS-1793 Repository: mesos-git Description --- Explicitly prohibit the use of namespace aliases. Give examples of preferable way of using auto. Diffs - docs/mesos-c++-style-guide.md 59a39df Diff: https://reviews.apache.org/r/25622/diff/ Testing --- Documentation change, no `make check` needed. Thanks, Alexander Rukletsov
Re: Review Request 26472: stout: Always use stout ABORT() rather than system abort()
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26472/#review56060 --- Looks good, just two minor issues below. Deferring to adam and dominic for committing. 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp https://reviews.apache.org/r/26472/#comment96410 Hm.. could we avoid the assumption that stringify() keeps errno intact? ``` char* error = strerror(errno); ABORT(... + error); ``` 3rdparty/libprocess/3rdparty/stout/include/stout/result.hpp https://reviews.apache.org/r/26472/#comment96409 There's a bug here ;) 3rdparty/libprocess/3rdparty/stout/include/stout/thread.hpp https://reviews.apache.org/r/26472/#comment96408 How about wrapping `strerror` with `std::string()` in these cases so that you don't have to deal with the strange indentation here? ``` ABORT(Failed to destruct thread local, pthread_key_delete: + std::string(strerror(errno))); ``` - Ben Mahler On Oct. 9, 2014, 4:35 p.m., Cody Maloney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26472/ --- (Updated Oct. 9, 2014, 4:35 p.m.) Review request for mesos, Adam B and Dominic Hamon. Bugs: MESOS-1870 https://issues.apache.org/jira/browse/MESOS-1870 Repository: mesos-git Description --- This makes it so any time there is an abort, we get a line number and at least a basic message as to why there was an abort. If you want a clean(er) exit, use stout/exit. Also adds an overload which takes a standard string and unwraps it to a const char * automatically, since a lot of the time we are building strings to pass them to abort. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/abort.hpp 6b5b5d1faa488cb3048f6d59bae17123b1b05a33 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9d244b2275f7ef84e8c486f2090b67a57ca8c670 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp 7138bc24912f35bfd04d2341b2218ea349ab40b8 3rdparty/libprocess/3rdparty/stout/include/stout/os/fork.hpp 8aa21ed10293c3dd7f55b7d30f6014bd05fd7455 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp ccf80a771c59092cb72d4dac0d868eec7df2f088 3rdparty/libprocess/3rdparty/stout/include/stout/result.hpp ce8dd9b6468fb36daceba60d8b43bf2fbfceab6c 3rdparty/libprocess/3rdparty/stout/include/stout/stringify.hpp ed0a1ef2b617bd87261bf4836706cedb80fdf043 3rdparty/libprocess/3rdparty/stout/include/stout/thread.hpp b1af74f517da75525d0dedacaf4a1c6f8cbf55f4 3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp 87c5fc8391d5213f0d76fa5980176726a0366f56 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 9207c551170b747d25207efe7d3c03675b184953 Diff: https://reviews.apache.org/r/26472/diff/ Testing --- make distcheck Thanks, Cody Maloney
Re: Review Request 26525: Moved executor checkpointing code from the Executor constructor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26525/#review56061 --- Bad patch! Reviews applied: [26525] Failed command: git apply --index 26525.patch Error: error: patch failed: src/slave/slave.cpp:3935 error: src/slave/slave.cpp: patch does not apply - Mesos ReviewBot On Oct. 9, 2014, 9:39 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26525/ --- (Updated Oct. 9, 2014, 9:39 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Repository: mesos-git Description --- There are two places where 'new Executor' is called: 1) launchExecutor 2) recoverExecutor For 2), we don't need checkpointing. Therefore, putting checkpointing code in Executor constructor and use state != RECOVERING to disginguish is not explicit and confusing. Diffs - src/slave/slave.hpp 28697102047b972ecb3b6b627ee089b430549fc0 src/slave/slave.cpp e56dcbd80114730949a0d4b553470802a4d38281 Diff: https://reviews.apache.org/r/26525/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 25549: Basic filesystem isolator for Linux.
On Oct. 7, 2014, 2:44 p.m., Jie Yu wrote: src/slave/containerizer/isolators/filesystem/shared.cpp, lines 74-77 https://reviews.apache.org/r/25549/diff/5/?file=711209#file711209line74 Should we check other error conditions as well? For example, `!executorInfo.has_container()`? This is checked a few lines below and will return None(). On Oct. 7, 2014, 2:44 p.m., Jie Yu wrote: src/slave/containerizer/mesos/containerizer.cpp, lines 434-440 https://reviews.apache.org/r/25549/diff/5/?file=711211#file711211line434 This check is redundent, right? Redundant with what? This is returns false if launching a container with a ContainerInfo != MESOS. Do you mean the check in filesystem/shared.cpp is redundant? - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25549/#review55689 --- On Oct. 2, 2014, 11:21 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25549/ --- (Updated Oct. 2, 2014, 11:21 a.m.) Review request for mesos, Ben Mahler, Jie Yu, and Vinod Kone. Bugs: MESOS-1586 https://issues.apache.org/jira/browse/MESOS-1586 Repository: mesos-git Description --- Does not report usage or enforce quota but can create 'private' directories for each container which mask parts of the shared host filesystem. This review replaces https://reviews.apache.org/r/24178/ because of some file renaming. I addressed all comments from earlier reviews. Diffs - include/mesos/mesos.proto 735da535f136d1188d3c6cf47b2e11153dab6fc3 src/Makefile.am 27c42dfde45a449750132e416b4eaf776f8c5e3b src/common/parse.hpp e6153d8a1f25bc9ddbe1e391306beeacfc8d5ff6 src/common/type_utils.hpp 480c0883fe6ed7f6a9daf77d83ebb077da2e66ee src/slave/containerizer/isolators/filesystem/shared.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/shared.cpp PRE-CREATION src/slave/containerizer/linux_launcher.cpp f7bc894830a7ca3f55465dacc7b653cdc2d7758b src/slave/containerizer/mesos/containerizer.cpp 9d083294caa5c5a47ba3ceaa1b57346144cb795c src/slave/flags.hpp 16f0cc2ab5ba16a39499608174278b3082e0585d src/slave/slave.cpp e56dcbd80114730949a0d4b553470802a4d38281 src/tests/isolator_tests.cpp c38f87632cb6984543cb3767dbd656cde7459610 src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 Diff: https://reviews.apache.org/r/25549/diff/ Testing --- make check # added a test Thanks, Ian Downes
Re: Review Request 25865: Pid namespace isolator for the MesosContainerizer.
On Oct. 2, 2014, 12:12 p.m., Timothy St. Clair wrote: src/slave/containerizer/isolators/filesystem/shared.cpp, line 90 https://reviews.apache.org/r/25865/diff/3/?file=711226#file711226line90 Why not use api's and MS_REMOUNT? This is run in the forked child process. This code has been removed but remounting /proc is not sufficient to correctly reflect the different pid namespace: -o remount or MS_REMOUNT only updates the options and data for an existing mount. - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25865/#review55231 --- On Oct. 2, 2014, 11:23 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25865/ --- (Updated Oct. 2, 2014, 11:23 a.m.) Review request for mesos, Jie Yu and Vinod Kone. Repository: mesos-git Description --- Add namespaces/pid to --isolation slave flag. Places executor into a pid namespace so it and all descendants will be contained in the namespace. Requires the filesystem/shared isolator so /proc and /sys are remounted to reflect the different namespace. Diffs - src/Makefile.am 27c42dfde45a449750132e416b4eaf776f8c5e3b src/slave/containerizer/isolators/filesystem/shared.cpp PRE-CREATION src/slave/containerizer/isolators/namespaces/pid.hpp PRE-CREATION src/slave/containerizer/isolators/namespaces/pid.cpp PRE-CREATION src/slave/containerizer/linux_launcher.cpp f7bc894830a7ca3f55465dacc7b653cdc2d7758b src/slave/containerizer/mesos/containerizer.cpp 9d083294caa5c5a47ba3ceaa1b57346144cb795c src/tests/isolator_tests.cpp c38f87632cb6984543cb3767dbd656cde7459610 Diff: https://reviews.apache.org/r/25865/diff/ Testing --- Added test that command in pid namespaced container is in a different namespace and that the command is 'init' (verifies remount of /proc). Thanks, Ian Downes
Re: Review Request 25865: Pid namespace isolator for the MesosContainerizer.
On Oct. 7, 2014, 2:23 p.m., Ben Mahler wrote:
Vinod and Jie asked me to take a look at this review.
It looks like there are dependent changes that are not linked in?
(filesystem islotor, ContainerInfo::MESOS, os::namespaces os::getns).
My main comment is that it seems really unfortuante that the isolator
boundary is broken here between the filesystem isolator and the pid
namespace isolator. I'm sure there will be folks out there that want pid
namespace isolation without having to use the shared filesystem isolator.
This has been cleaned up some by removing the /sys mount (not needed by the
port_mapping isolator) and moving the /proc mount to the pid isolator.
On Oct. 7, 2014, 2:23 p.m., Ben Mahler wrote:
src/slave/containerizer/isolators/filesystem/shared.cpp, lines 85-89
https://reviews.apache.org/r/25865/diff/3/?file=711226#file711226line85
A few questions:
(1) Why are you saying lazily here? What aspect of this is done lazily?
(2) So.. with pid namespaces you have to manually remount /proc to hide
the changes? Can you make the comment more explicit about how important it
is to do this?
(3) Why is /sys remounted? An explanation in the comment would be great
for when us dummies are reading this code later and scratching our heads. :)
(4) Curious, is the remounting only specific to pid namespacing, as
opposed to other namespaces?
(1) commented
(2) Yes. Commented.
(3) /sys mount removed.
(4) Mostly yes. It turns out the remounting /sys is not required for a network
namespace but it definitely is required for a pid namespace so that /proc lists
the pids of the namespace, not the parent namespace.
On Oct. 7, 2014, 2:23 p.m., Ben Mahler wrote:
src/slave/containerizer/isolators/namespaces/pid.cpp, line 50
https://reviews.apache.org/r/25865/diff/3/?file=711228#file711228line50
If it's a pid namespace isolator, why not call this class
'PidNamespaceIsolator'?
See Vinod's comment. I agree it reads better but this is inline with existing
Cgroups{Cpu,Mem}Isolator. Happy to change though...
On Oct. 7, 2014, 2:23 p.m., Ben Mahler wrote:
src/slave/containerizer/isolators/namespaces/pid.cpp, lines 112-119
https://reviews.apache.org/r/25865/diff/3/?file=711228#file711228line112
Why is state.id an option in the first place?
Historical reasons? It was kept as an Option when we moved from UUID to
ContainerID.
On Oct. 7, 2014, 2:23 p.m., Ben Mahler wrote:
src/slave/containerizer/linux_launcher.cpp, line 362
https://reviews.apache.org/r/25865/diff/3/?file=711229#file711229line362
This line is unused...?
It's used in the os::getns call below. I capture it so I can erase from the map
in one place before any return code paths.
On Oct. 7, 2014, 2:23 p.m., Ben Mahler wrote:
src/tests/isolator_tests.cpp, lines 976-978
https://reviews.apache.org/r/25865/diff/3/?file=711231#file711231line976
What is the difference between os::getns() here and
NamespacesPidIsolatorProcess::getNamespace?
Where is the review that created os::getns, could we use a better name
for that method?
Why isn't is linux::namespaces and linux::getns, are namespaces
applicable to other systems?
Comment on diffence added to pid.hpp. os::getns() returns any namespace of a
pid, getNamespace() returns the pid namespace of *container*.
No, linux specific. os::getns() follows from the existing os::setns() which I
agree should be namespaced as linux::
On Oct. 7, 2014, 2:23 p.m., Ben Mahler wrote:
src/tests/isolator_tests.cpp, line 978
https://reviews.apache.org/r/25865/diff/3/?file=711231#file711231line978
The value itself doesn't matter?
No, it's None if it's in the caller's namespace (which would be a test
failure), otherwise it is in its own namespace.
- Ian
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25865/#review55685
---
On Oct. 2, 2014, 11:23 a.m., Ian Downes wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25865/
---
(Updated Oct. 2, 2014, 11:23 a.m.)
Review request for mesos, Jie Yu and Vinod Kone.
Repository: mesos-git
Description
---
Add namespaces/pid to --isolation slave flag. Places executor into a pid
namespace so it and all descendants will be contained in the namespace.
Requires the filesystem/shared isolator so /proc and /sys are remounted to
reflect the different namespace.
Diffs
-
src/Makefile.am 27c42dfde45a449750132e416b4eaf776f8c5e3b
src/slave/containerizer/isolators/filesystem/shared.cpp PRE-CREATION
Re: Review Request 26508: Added --module flag for Mesos master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26508/ --- (Updated Oct. 9, 2014, 7:19 p.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Niklas Nielsen. Repository: mesos-git Description --- Added --module flag for Mesos master. Diffs (updated) - src/common/parse.hpp e6153d8a1f25bc9ddbe1e391306beeacfc8d5ff6 src/common/type_utils.hpp 480c0883fe6ed7f6a9daf77d83ebb077da2e66ee src/master/flags.hpp b8b59a245523c7e7d4e04f86801ed7b023a129ea src/master/main.cpp 018f8036b742f0b083445e03dca5c06c13d80e68 src/module/manager.hpp 8275fd0c2f0c9ba92a9ad47f7d5d2df9295fa184 Diff: https://reviews.apache.org/r/26508/diff/ Testing --- make check Thanks, Kapil Arya
Re: Review Request 26509: Added --module flag for Mesos slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26509/#review56085 --- Bad patch! Reviews applied: [26529, 26513] Failed command: git apply --index 26513.patch Error: error: patch failed: src/module/manager.cpp:188 error: src/module/manager.cpp: patch does not apply - Mesos ReviewBot On Oct. 9, 2014, 11:19 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26509/ --- (Updated Oct. 9, 2014, 11:19 p.m.) Review request for mesos and Niklas Nielsen. Repository: mesos-git Description --- Added --module flag for Mesos slave. Diffs - src/slave/flags.hpp 16f0cc2ab5ba16a39499608174278b3082e0585d src/slave/main.cpp 2c4d365a04acbcb382e978d811a318130484b3d5 Diff: https://reviews.apache.org/r/26509/diff/ Testing --- Thanks, Kapil Arya
Re: Review Request 26472: stout: Always use stout ABORT() rather than system abort()
On Oct. 9, 2014, 9:59 p.m., Ben Mahler wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp, lines 442-443 https://reviews.apache.org/r/26472/diff/2/?file=716714#file716714line442 Hm.. could we avoid the assumption that stringify() keeps errno intact? ``` char* error = strerror(errno); ABORT(... + error); ``` What I would like to do (longer term) is make a couple helpers such as: ``` template typename T TryT TryOsReturn(T); template typename T TryT TryOsNonZero(T); ``` That take the os function return and then calls strerror for us when needed to set the error message. Would make it much, much harder to accidentally perturb errno. On Oct. 9, 2014, 9:59 p.m., Ben Mahler wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/thread.hpp, lines 42-44 https://reviews.apache.org/r/26472/diff/2/?file=716719#file716719line42 How about wrapping `strerror` with `std::string()` in these cases so that you don't have to deal with the strange indentation here? ``` ABORT(Failed to destruct thread local, pthread_key_delete: + std::string(strerror(errno))); ``` Updated - Cody --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26472/#review56060 --- On Oct. 10, 2014, 12:44 a.m., Cody Maloney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26472/ --- (Updated Oct. 10, 2014, 12:44 a.m.) Review request for mesos, Adam B and Dominic Hamon. Bugs: MESOS-1870 https://issues.apache.org/jira/browse/MESOS-1870 Repository: mesos-git Description --- This makes it so any time there is an abort, we get a line number and at least a basic message as to why there was an abort. If you want a clean(er) exit, use stout/exit. Also adds an overload which takes a standard string and unwraps it to a const char * automatically, since a lot of the time we are building strings to pass them to abort. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/abort.hpp 6b5b5d1faa488cb3048f6d59bae17123b1b05a33 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9d244b2275f7ef84e8c486f2090b67a57ca8c670 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp 7138bc24912f35bfd04d2341b2218ea349ab40b8 3rdparty/libprocess/3rdparty/stout/include/stout/os/fork.hpp 8aa21ed10293c3dd7f55b7d30f6014bd05fd7455 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp ccf80a771c59092cb72d4dac0d868eec7df2f088 3rdparty/libprocess/3rdparty/stout/include/stout/result.hpp ce8dd9b6468fb36daceba60d8b43bf2fbfceab6c 3rdparty/libprocess/3rdparty/stout/include/stout/stringify.hpp ed0a1ef2b617bd87261bf4836706cedb80fdf043 3rdparty/libprocess/3rdparty/stout/include/stout/thread.hpp b1af74f517da75525d0dedacaf4a1c6f8cbf55f4 3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp 87c5fc8391d5213f0d76fa5980176726a0366f56 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 9207c551170b747d25207efe7d3c03675b184953 Diff: https://reviews.apache.org/r/26472/diff/ Testing --- make distcheck Thanks, Cody Maloney
Re: Review Request 26472: stout: Always use stout ABORT() rather than system abort()
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26472/ --- (Updated Oct. 10, 2014, 12:44 a.m.) Review request for mesos, Adam B and Dominic Hamon. Changes --- Updated to address Ben Mahler's comments Bugs: MESOS-1870 https://issues.apache.org/jira/browse/MESOS-1870 Repository: mesos-git Description --- This makes it so any time there is an abort, we get a line number and at least a basic message as to why there was an abort. If you want a clean(er) exit, use stout/exit. Also adds an overload which takes a standard string and unwraps it to a const char * automatically, since a lot of the time we are building strings to pass them to abort. Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/abort.hpp 6b5b5d1faa488cb3048f6d59bae17123b1b05a33 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9d244b2275f7ef84e8c486f2090b67a57ca8c670 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp 7138bc24912f35bfd04d2341b2218ea349ab40b8 3rdparty/libprocess/3rdparty/stout/include/stout/os/fork.hpp 8aa21ed10293c3dd7f55b7d30f6014bd05fd7455 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp ccf80a771c59092cb72d4dac0d868eec7df2f088 3rdparty/libprocess/3rdparty/stout/include/stout/result.hpp ce8dd9b6468fb36daceba60d8b43bf2fbfceab6c 3rdparty/libprocess/3rdparty/stout/include/stout/stringify.hpp ed0a1ef2b617bd87261bf4836706cedb80fdf043 3rdparty/libprocess/3rdparty/stout/include/stout/thread.hpp b1af74f517da75525d0dedacaf4a1c6f8cbf55f4 3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp 87c5fc8391d5213f0d76fa5980176726a0366f56 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 9207c551170b747d25207efe7d3c03675b184953 Diff: https://reviews.apache.org/r/26472/diff/ Testing --- make distcheck Thanks, Cody Maloney
Re: Review Request 26472: stout: Always use stout ABORT() rather than system abort()
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26472/ --- (Updated Oct. 10, 2014, 12:46 a.m.) Review request for mesos, Adam B and Dominic Hamon. Changes --- Fix a bad indent in thread.hpp Bugs: MESOS-1870 https://issues.apache.org/jira/browse/MESOS-1870 Repository: mesos-git Description --- This makes it so any time there is an abort, we get a line number and at least a basic message as to why there was an abort. If you want a clean(er) exit, use stout/exit. Also adds an overload which takes a standard string and unwraps it to a const char * automatically, since a lot of the time we are building strings to pass them to abort. Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/abort.hpp 6b5b5d1faa488cb3048f6d59bae17123b1b05a33 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9d244b2275f7ef84e8c486f2090b67a57ca8c670 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp 7138bc24912f35bfd04d2341b2218ea349ab40b8 3rdparty/libprocess/3rdparty/stout/include/stout/os/fork.hpp 8aa21ed10293c3dd7f55b7d30f6014bd05fd7455 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp ccf80a771c59092cb72d4dac0d868eec7df2f088 3rdparty/libprocess/3rdparty/stout/include/stout/result.hpp ce8dd9b6468fb36daceba60d8b43bf2fbfceab6c 3rdparty/libprocess/3rdparty/stout/include/stout/stringify.hpp ed0a1ef2b617bd87261bf4836706cedb80fdf043 3rdparty/libprocess/3rdparty/stout/include/stout/thread.hpp b1af74f517da75525d0dedacaf4a1c6f8cbf55f4 3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp 87c5fc8391d5213f0d76fa5980176726a0366f56 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 9207c551170b747d25207efe7d3c03675b184953 Diff: https://reviews.apache.org/r/26472/diff/ Testing --- make distcheck Thanks, Cody Maloney
Review Request 26533: Memory cleanup: libprocess finalize
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26533/ --- Review request for mesos, Benjamin Hindman and Niklas Nielsen. Repository: mesos-git Description --- Introduce a finalize() function to match the initialize() function in process.hpp. Use this at the unit test runner for libprocess to start getting more valuable valgrind feedback. - Added cleanup of remaining Processes in ~ProcessManager() as a start. Diffs - 3rdparty/libprocess/include/process/process.hpp 270ca28 3rdparty/libprocess/src/process.cpp d30ed63 3rdparty/libprocess/src/tests/main.cpp 0a3ba4e Diff: https://reviews.apache.org/r/26533/diff/ Testing --- make check support/mesos-style valgrind 3rdparty/libprocess/tests (to see a reduction in definitely lost blocks) Thanks, Joris Van Remoortere
Re: Review Request 26517: Symlink sandbox directories in docker containerizer
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26517/ --- (Updated Oct. 10, 2014, 1:22 a.m.) Review request for mesos and Benjamin Hindman. Bugs: MESOS-1833 https://issues.apache.org/jira/browse/MESOS-1833 Repository: mesos-git Description --- Symlink sandbox directories in docker containerizer Diffs - src/slave/containerizer/docker.hpp fbbd45d77e5f2f74ca893552f85eb893b3dd948f src/slave/containerizer/docker.cpp 9a2948951f57f3ab16291df51cd9f33e5e96add4 src/tests/docker_containerizer_tests.cpp 67d60a885d65edbcbbf702bce83a54d1a5c0411f Diff: https://reviews.apache.org/r/26517/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 26533: Memory cleanup: libprocess finalize
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26533/#review56091 --- Patch looks great! Reviews applied: [26533] All tests passed. - Mesos ReviewBot On Oct. 10, 2014, 1:02 a.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26533/ --- (Updated Oct. 10, 2014, 1:02 a.m.) Review request for mesos, Benjamin Hindman and Niklas Nielsen. Repository: mesos-git Description --- Introduce a finalize() function to match the initialize() function in process.hpp. Use this at the unit test runner for libprocess to start getting more valuable valgrind feedback. - Added cleanup of remaining Processes in ~ProcessManager() as a start. Diffs - 3rdparty/libprocess/include/process/process.hpp 270ca28 3rdparty/libprocess/src/process.cpp d30ed63 3rdparty/libprocess/src/tests/main.cpp 0a3ba4e Diff: https://reviews.apache.org/r/26533/diff/ Testing --- make check support/mesos-style valgrind 3rdparty/libprocess/tests (to see a reduction in definitely lost blocks) Thanks, Joris Van Remoortere
Review Request 26535: Added batch sizes and timings to the Registrar logging.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26535/ --- Review request for mesos and Vinod Kone. Repository: mesos-git Description --- We currently lack logging visibility into the batch sizes in the registrar. While we have timing metrics, we also lack the ability to easily read timings for particular operations. Diffs - src/master/registrar.cpp ccc2fd23b1a1d56ab327cc73f6fef35d4bf6a552 Diff: https://reviews.apache.org/r/26535/diff/ Testing --- make check Thanks, Ben Mahler
Re: Review Request 25865: Pid namespace isolator for the MesosContainerizer.
On Oct. 7, 2014, 9:23 p.m., Ben Mahler wrote: src/slave/containerizer/linux_launcher.cpp, line 362 https://reviews.apache.org/r/25865/diff/3/?file=711229#file711229line362 This line is unused...? Ian Downes wrote: It's used in the os::getns call below. I capture it so I can erase from the map in one place before any return code paths. I'm confused, there's no os::getns scope in this method...? - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25865/#review55685 --- On Oct. 2, 2014, 6:23 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25865/ --- (Updated Oct. 2, 2014, 6:23 p.m.) Review request for mesos, Jie Yu and Vinod Kone. Repository: mesos-git Description --- Add namespaces/pid to --isolation slave flag. Places executor into a pid namespace so it and all descendants will be contained in the namespace. Requires the filesystem/shared isolator so /proc and /sys are remounted to reflect the different namespace. Diffs - src/Makefile.am 27c42dfde45a449750132e416b4eaf776f8c5e3b src/slave/containerizer/isolators/filesystem/shared.cpp PRE-CREATION src/slave/containerizer/isolators/namespaces/pid.hpp PRE-CREATION src/slave/containerizer/isolators/namespaces/pid.cpp PRE-CREATION src/slave/containerizer/linux_launcher.cpp f7bc894830a7ca3f55465dacc7b653cdc2d7758b src/slave/containerizer/mesos/containerizer.cpp 9d083294caa5c5a47ba3ceaa1b57346144cb795c src/tests/isolator_tests.cpp c38f87632cb6984543cb3767dbd656cde7459610 Diff: https://reviews.apache.org/r/25865/diff/ Testing --- Added test that command in pid namespaced container is in a different namespace and that the command is 'init' (verifies remount of /proc). Thanks, Ian Downes
Re: Review Request 26535: Added batch sizes and timings to the Registrar logging.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26535/#review56093 --- Ship it! Ship It! - Vinod Kone On Oct. 10, 2014, 1:51 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26535/ --- (Updated Oct. 10, 2014, 1:51 a.m.) Review request for mesos and Vinod Kone. Repository: mesos-git Description --- We currently lack logging visibility into the batch sizes in the registrar. While we have timing metrics, we also lack the ability to easily read timings for particular operations. Diffs - src/master/registrar.cpp ccc2fd23b1a1d56ab327cc73f6fef35d4bf6a552 Diff: https://reviews.apache.org/r/26535/diff/ Testing --- make check Thanks, Ben Mahler
Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25525/ --- (Updated Oct. 10, 2014, 2:29 a.m.) Review request for mesos, Adam B, Benjamin Hindman, Patrick Reilly, and Vinod Kone. Changes --- Add slaveUpdated to Allocator API, per comments on the design document. Rebase on top of latest mesos changes. Bugs: MESOS-1739 https://issues.apache.org/jira/browse/MESOS-1739 Repository: mesos-git Description --- Allows attributes and resources to be set to a superset of what they were previously on a slave restart. Incorporates all comments from: https://issues.apache.org/jira/browse/MESOS-1739 and the former review request: https://reviews.apache.org/r/25111/ Diffs (updated) - src/Makefile.am d503c8df73cda15a9d59254e8265e4a5d0e003a4 src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 src/common/slaveinfo_utils.hpp PRE-CREATION src/common/slaveinfo_utils.cpp PRE-CREATION src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 src/master/hierarchical_allocator_process.hpp 31dfb2cc86e2e74da43f05fbada10976ce65f3e4 src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205 src/master/master.cpp cb46cec0674b3aa031450c5b4f48f4f8bb92767d src/slave/slave.cpp cb3759993f863590cb1545c73072feb0331aa6c9 src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 src/tests/slave_tests.cpp 69be28f6e82b99e23424bd2be8294f715d8040d4 Diff: https://reviews.apache.org/r/25525/diff/ Testing --- make check on localhost Thanks, Cody Maloney
Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25525/ --- (Updated Oct. 10, 2014, 2:29 a.m.) Review request for mesos, Adam B and Vinod Kone. Bugs: MESOS-1739 https://issues.apache.org/jira/browse/MESOS-1739 Repository: mesos-git Description --- Allows attributes and resources to be set to a superset of what they were previously on a slave restart. Incorporates all comments from: https://issues.apache.org/jira/browse/MESOS-1739 and the former review request: https://reviews.apache.org/r/25111/ Diffs - src/Makefile.am d503c8df73cda15a9d59254e8265e4a5d0e003a4 src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 src/common/slaveinfo_utils.hpp PRE-CREATION src/common/slaveinfo_utils.cpp PRE-CREATION src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 src/master/hierarchical_allocator_process.hpp 31dfb2cc86e2e74da43f05fbada10976ce65f3e4 src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205 src/master/master.cpp cb46cec0674b3aa031450c5b4f48f4f8bb92767d src/slave/slave.cpp cb3759993f863590cb1545c73072feb0331aa6c9 src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 src/tests/slave_tests.cpp 69be28f6e82b99e23424bd2be8294f715d8040d4 Diff: https://reviews.apache.org/r/25525/diff/ Testing --- make check on localhost Thanks, Cody Maloney
Re: Review Request 26535: Added batch sizes and timings to the Registrar logging.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26535/#review56097 --- Patch looks great! Reviews applied: [26535] All tests passed. - Mesos ReviewBot On Oct. 10, 2014, 1:51 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26535/ --- (Updated Oct. 10, 2014, 1:51 a.m.) Review request for mesos and Vinod Kone. Repository: mesos-git Description --- We currently lack logging visibility into the batch sizes in the registrar. While we have timing metrics, we also lack the ability to easily read timings for particular operations. Diffs - src/master/registrar.cpp ccc2fd23b1a1d56ab327cc73f6fef35d4bf6a552 Diff: https://reviews.apache.org/r/26535/diff/ Testing --- make check Thanks, Ben Mahler
Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25525/#review56104 --- Patch looks great! Reviews applied: [25525] All tests passed. - Mesos ReviewBot On Oct. 10, 2014, 2:29 a.m., Cody Maloney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25525/ --- (Updated Oct. 10, 2014, 2:29 a.m.) Review request for mesos, Adam B and Vinod Kone. Bugs: MESOS-1739 https://issues.apache.org/jira/browse/MESOS-1739 Repository: mesos-git Description --- Allows attributes and resources to be set to a superset of what they were previously on a slave restart. Incorporates all comments from: https://issues.apache.org/jira/browse/MESOS-1739 and the former review request: https://reviews.apache.org/r/25111/ Diffs - src/Makefile.am d503c8df73cda15a9d59254e8265e4a5d0e003a4 src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 src/common/slaveinfo_utils.hpp PRE-CREATION src/common/slaveinfo_utils.cpp PRE-CREATION src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 src/master/hierarchical_allocator_process.hpp 31dfb2cc86e2e74da43f05fbada10976ce65f3e4 src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205 src/master/master.cpp cb46cec0674b3aa031450c5b4f48f4f8bb92767d src/slave/slave.cpp cb3759993f863590cb1545c73072feb0331aa6c9 src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 src/tests/slave_tests.cpp 69be28f6e82b99e23424bd2be8294f715d8040d4 Diff: https://reviews.apache.org/r/25525/diff/ Testing --- make check on localhost Thanks, Cody Maloney
