Re: Review Request 25848: Introducing mesos modules.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25848/ --- (Updated Oct. 8, 2014, 2:31 a.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and Timothy St. Clair. Changes --- Bug fixes to module version checking and testing; added ModuleManager::shutdown() to reinitialize the data structures (for make check only). From a given list of modules, if some modules fail verfication checks, etc., load the remaining (verified) modules regardless. Bugs: MESOS-1384 https://issues.apache.org/jira/browse/MESOS-1384 Repository: mesos-git Description --- Adding a first class primitive, abstraction and process for dynamic library writing and loading can make it easier to extend inner workings of Mesos. Making it possible to have dynamic loadable resource allocators, isolators, containerizes, authenticators and much more. Diffs (updated) - include/mesos/module.hpp PRE-CREATION src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724 src/examples/example_module_impl.cpp PRE-CREATION src/examples/test_module.hpp PRE-CREATION src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d src/module/manager.hpp PRE-CREATION src/module/manager.cpp PRE-CREATION src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b src/tests/module_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/25848/diff/ Testing --- Ran make check with added tests for verifying library/module loading and simple version check. Thanks, Kapil Arya
Jekin build steps
Hi all, I'm working on MESOS-1416 (mesos-0.19.0 build directory is read-only), so i'd like to know the steps to reproduce it. Can anyone help me? Regards,Da Ma (马达), PMP® | CEL3 Team LeadPlatform Symphony MapReduce Development Support, STG, IBM GCG+86-10-8245 4084 | mad...@cn.ibm.com | http://www.cguru.net
Re: Review Request 25848: Introducing mesos modules.
On Oct. 8, 2014, 1:17 a.m., Michael Park wrote:
src/module/manager.hpp, lines 95-105
https://reviews.apache.org/r/25848/diff/17/?file=714903#file714903line95
I would suggest changing these to be static functions that return
static singletons as per
[MESOS-1023](https://issues.apache.org/jira/browse/MESOS-1023). Something
like,
```cpp
static pthread_mutex_t mutex() {
static pthread_mutex_t singleton = PTHREAD_MUTEX_INITIALIZER;
return singleton;
}
```
Benjamin Hindman wrote:
Maybe I'm missing something here, but how does this help? The destructors
will still get called for static variables in functions (even static
functions).
The difference is that initialization order of global/static member variables
across translation units are non-determistic. In this case, the variables
aren't accessible from another translation unit, but we might make one of them
`public` in the future.
- Michael
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/#review55735
---
On Oct. 8, 2014, 6:31 a.m., Kapil Arya wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25848/
---
(Updated Oct. 8, 2014, 6:31 a.m.)
Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen,
and Timothy St. Clair.
Bugs: MESOS-1384
https://issues.apache.org/jira/browse/MESOS-1384
Repository: mesos-git
Description
---
Adding a first class primitive, abstraction and process for dynamic library
writing and loading can make it easier to extend inner workings of Mesos.
Making it possible to have dynamic loadable resource allocators, isolators,
containerizes, authenticators and much more.
Diffs
-
include/mesos/module.hpp PRE-CREATION
src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724
src/examples/example_module_impl.cpp PRE-CREATION
src/examples/test_module.hpp PRE-CREATION
src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d
src/module/manager.hpp PRE-CREATION
src/module/manager.cpp PRE-CREATION
src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b
src/tests/module_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/25848/diff/
Testing
---
Ran make check with added tests for verifying library/module loading and
simple version check.
Thanks,
Kapil Arya
Re: Reserved names
Just as a note, here's the JIRA ticket: https://issues.apache.org/jira/browse/MESOS-1046 On 6 October 2014 13:54, Dominic Hamon dha...@twopensource.com wrote: Hello I think this is correctly observed, and I'm surprised that it hasn't yet bitten us given our propensity for short names and use of double underscores. I think renaming the guard macros should be trivial enough that we just do it to conform to the standard. Maybe open a JIRA ticket for this specifically that someone can grab if they're inspired. This thread could devolve into bikeshedding pretty quickly, but I don't know of another way to thrash out what might be the correct approach. Renaming symbols is fairly trivial, the hard part is choosing the naming scheme[1]. I'm not a fan of numbering as I find it difficult to parse quickly. I thought the issue was only with leading underscores, but as you point out it isn't. Perhaps we should use prefix or suffixed 'c' for continuation. Example: launch - c_launch - cc_launch or launch - launch_c - launch_cc or we could be explicit in naming: launch - launchCont - launchContCont Thoughts? 1. there are 2 hard problems in computer science: caching, naming things, and off-by-one errors. On Sun, Oct 5, 2014 at 4:20 PM, Michael Park mcyp...@gmail.com wrote: Hello, I just wanted to mention an issue with our naming convention that goes against the C++ standard due to the rules around reserved names. From N3797, 17.6.4.3 Reserved names [reserved.names] . . . 17.6.4.3.2 Global names [global.names] 1 Certain sets of names and function signatures are always reserved to the implementation: — *Each name that contains a double underscore _ _ or begins with an underscore followed by an uppercase letter (2.12) is reserved to the implementation for any use. * — *Each name that begins with an underscore is reserved to the implementation for use as a name in the global namespace.* The biggest offender is the include guard since all of them start and end with double underscores. A few other examples are *stout/exit.hpp*'s *__Exit* struct, *src/zookeeper/zookeeper.cpp*'s *__create*, *src/slave/containerizer/docker.cpp*'s *__launch*. We use leading double underscore for internal implementation sometimes, which I think should live in the *internal* namespace instead or have names such as *createImpl* or *createHelper*. In the case of *__launch*, we start out with *launch* then go onto *_launch* (this one is allowed because it's a member function and therefore not in the global namespace and it started with an underscore but is not followed by an uppercase letter.) then we get to *__launch* which is not allowed since it contains a double underscore. We should probably give these kinds of functions better names associated with their phase, or even just numbering them would be ok. Changing them to be trailing underscores wouldn't help much either since the rule around double underscores is *contains* rather than *starts with*. Anyway, practically speaking it hasn't been a big issue for us yet. It's just something I noticed and thought should bring up to the group. Thanks, MPark -- Dominic Hamon | @mrdo | Twitter *There are no bad ideas; only good ideas that go horribly wrong.*
Re: Review Request 26229: Expose poll interval from the reaper.
On Oct. 6, 2014, 10:02 p.m., Ben Mahler wrote: 3rdparty/libprocess/src/reap.cpp, lines 124-127 https://reviews.apache.org/r/26229/diff/1/?file=710088#file710088line124 Why do you need a variable for this? Can't this just be a 'return' statement? If there's a reason to keep this statement separate from the return, please avoid 'auto', we'd like to start using it very conservatively. Alexander Rukletsov wrote: Naming the result of the expression serves two purposes here: clarity about what we calculate and return and facilitating NRWO. Since auto is replaced, I mark this issue as fixed, please reopen if you think further discussion about `return` is needed. Michael Park wrote: My thoughts on the two purposes: 1. I actually think a comment would do serve the purpose better in terms of being clear on what we're calculating. `adjustedInterval` doesn't actually give me that much more information. 2. Facilitating NRVO I don't think is a good reason. It's much more likely for URVO to kick in than NRVO. 1. We already have a top-level comment describing the approach and one more right before interpolation code. That's why I think third comment is redundant and naming a variable suffices. 2. After the closer look at the code I agree with you: NRVO won't kick in here. I'll amend it to facilitate URVO. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26229/#review55582 --- On Oct. 7, 2014, 4:39 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26229/ --- (Updated Oct. 7, 2014, 4:39 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 26229: Expose poll interval from the reaper.
On Oct. 7, 2014, 9:46 p.m., Ben Mahler wrote: I'm curious why you need to expose both sides of the bounds? Our tests currently hard-code 1 second as the reap interval, and since Ian did not change the maximum, the tests continue to function as expected. Are you planning to follow up and adjust the tests? E.g. subprocess_tests.cpp, slave_recovery_tests.cpp, etc. Naming wise, how about: ``` process::REAP_INTERVAL(); ``` The examples in the ticket you referenced fall into two cases: (1) Static member functions (e.g. Duration::max() and Time::epoch()), and (2) Global functions (e.g. MAX_PING_TIMEOUT()). Since for (2) we've kept ALL_CAPS, we should stick to ALL_CAPS here for consistency. The benefit of ALL_CAPS for globals is that it provides a signal to the reader of the code that this is a constant value, despite the fact that we had to make it a function (for non-POD global safety). The lower bound may be needed for testing situation when the reaper should not kick in, e.g. by setting the graceful shutdown timeout to the lower bound we test the force shutdwon path. The idea for the patch came when I had to hard-code both 1s and 100ms in my tests. I would propose we create a separate JIRA for updating the tests. Names are updated to `LOW_REAP_INTERVAL()` and `HIGH_REAP_INTERVAL()`. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26229/#review55718 --- On Oct. 7, 2014, 4:39 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26229/ --- (Updated Oct. 7, 2014, 4:39 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 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. 8, 2014, 9:30 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, and Till Toenshoff. Changes --- Rename functions; eliminate intermediate variable. 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 26436: Avoid docker inspect on each usage call
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26436/#review55772 --- src/slave/containerizer/docker.cpp https://reviews.apache.org/r/26436/#comment96166 I've seen the pattern such as: `usage` - `_usage` - `__usage` for sequence of continuation functions. It doesn't seem like `__usage` in this case is actually a continuation function, but rather a refactoring of common code path. Maybe it should be named something else? src/slave/containerizer/docker.cpp https://reviews.apache.org/r/26436/#comment96167 Are these checks necessary? In both calls to `__usage` directly rather than through `defer` or something similar. - Michael Park On Oct. 8, 2014, 4:51 a.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26436/ --- (Updated Oct. 8, 2014, 4:51 a.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.
On Oct. 6, 2014, 10:02 p.m., Ben Mahler wrote: 3rdparty/libprocess/src/reap.cpp, lines 124-127 https://reviews.apache.org/r/26229/diff/1/?file=710088#file710088line124 Why do you need a variable for this? Can't this just be a 'return' statement? If there's a reason to keep this statement separate from the return, please avoid 'auto', we'd like to start using it very conservatively. Alexander Rukletsov wrote: Naming the result of the expression serves two purposes here: clarity about what we calculate and return and facilitating NRWO. Since auto is replaced, I mark this issue as fixed, please reopen if you think further discussion about `return` is needed. Michael Park wrote: My thoughts on the two purposes: 1. I actually think a comment would do serve the purpose better in terms of being clear on what we're calculating. `adjustedInterval` doesn't actually give me that much more information. 2. Facilitating NRVO I don't think is a good reason. It's much more likely for URVO to kick in than NRVO. Alexander Rukletsov wrote: 1. We already have a top-level comment describing the approach and one more right before interpolation code. That's why I think third comment is redundant and naming a variable suffices. 2. After the closer look at the code I agree with you: NRVO won't kick in here. I'll amend it to facilitate URVO. 1. Sorry! I missed the above comments. 2. Well, in this case it will. But it also doesn't matter since the only member in `Duration` is a single `int64_t`. All I was saying is that in general URVO is more likely to kick in than NRVO. So if there's an option to choose, we should prefer URVO (unless of course, naming a variable helps readability). - Michael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26229/#review55582 --- On Oct. 8, 2014, 9:30 a.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26229/ --- (Updated Oct. 8, 2014, 9:30 a.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 26229: Expose poll interval from the reaper.
On Oct. 6, 2014, 10:02 p.m., Ben Mahler wrote: 3rdparty/libprocess/src/reap.cpp, lines 124-127 https://reviews.apache.org/r/26229/diff/1/?file=710088#file710088line124 Why do you need a variable for this? Can't this just be a 'return' statement? If there's a reason to keep this statement separate from the return, please avoid 'auto', we'd like to start using it very conservatively. Alexander Rukletsov wrote: Naming the result of the expression serves two purposes here: clarity about what we calculate and return and facilitating NRWO. Since auto is replaced, I mark this issue as fixed, please reopen if you think further discussion about `return` is needed. Michael Park wrote: My thoughts on the two purposes: 1. I actually think a comment would do serve the purpose better in terms of being clear on what we're calculating. `adjustedInterval` doesn't actually give me that much more information. 2. Facilitating NRVO I don't think is a good reason. It's much more likely for URVO to kick in than NRVO. Alexander Rukletsov wrote: 1. We already have a top-level comment describing the approach and one more right before interpolation code. That's why I think third comment is redundant and naming a variable suffices. 2. After the closer look at the code I agree with you: NRVO won't kick in here. I'll amend it to facilitate URVO. Michael Park wrote: 1. Sorry! I missed the above comments. 2. Well, in this case it will. But it also doesn't matter since the only member in `Duration` is a single `int64_t`. All I was saying is that in general URVO is more likely to kick in than NRVO. So if there's an option to choose, we should prefer URVO (unless of course, naming a variable helps readability). Why would NRVO work here? We have two other paths returning rvalues. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26229/#review55582 --- On Oct. 8, 2014, 9:30 a.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26229/ --- (Updated Oct. 8, 2014, 9:30 a.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 23912: Fix MESOS-947: Slave should properly handle a killTask() that arrives between runTask() and _runTask()
On Oct. 7, 2014, 10:38 a.m., Vinod Kone wrote: I don't see any changes in the diff? Bad diff? Sorry, git pilot error. Will re-post. - Bernd --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23912/#review55665 --- On Oct. 7, 2014, 8:18 a.m., Bernd Mathiske wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23912/ --- (Updated Oct. 7, 2014, 8:18 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 26229: Expose poll interval from the reaper.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26229/#review55779 --- Patch looks great! Reviews applied: [26229] All tests passed. - Mesos ReviewBot On Oct. 8, 2014, 9:30 a.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26229/ --- (Updated Oct. 8, 2014, 9:30 a.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 23912: Fix MESOS-947: Slave should properly handle a killTask() that arrives between runTask() and _runTask()
On Oct. 7, 2014, 11:22 a.m., Michael Park wrote:
src/slave/slave.cpp, lines 1354-1356
https://reviews.apache.org/r/23912/diff/6/?file=714590#file714590line1354
It looks like `taskMap.erase(taskId)` needs to modify the
`framework-pending` hashmap. I think we want `foreachvalue (TaskMap
taskMap, framework-pending) { ... }` here?
Yes, thanks! Much appreciated that you caught this glitch.
- Bernd
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23912/#review55671
---
On Oct. 7, 2014, 8:18 a.m., Bernd Mathiske wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23912/
---
(Updated Oct. 7, 2014, 8:18 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. 8, 2014, 3:57 a.m.) Review request for mesos. Changes --- Fixed MPark's review issue. 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 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 26229: Expose poll interval from the reaper.
On Oct. 6, 2014, 10:02 p.m., Ben Mahler wrote: 3rdparty/libprocess/src/reap.cpp, lines 124-127 https://reviews.apache.org/r/26229/diff/1/?file=710088#file710088line124 Why do you need a variable for this? Can't this just be a 'return' statement? If there's a reason to keep this statement separate from the return, please avoid 'auto', we'd like to start using it very conservatively. Alexander Rukletsov wrote: Naming the result of the expression serves two purposes here: clarity about what we calculate and return and facilitating NRWO. Since auto is replaced, I mark this issue as fixed, please reopen if you think further discussion about `return` is needed. Michael Park wrote: My thoughts on the two purposes: 1. I actually think a comment would do serve the purpose better in terms of being clear on what we're calculating. `adjustedInterval` doesn't actually give me that much more information. 2. Facilitating NRVO I don't think is a good reason. It's much more likely for URVO to kick in than NRVO. Alexander Rukletsov wrote: 1. We already have a top-level comment describing the approach and one more right before interpolation code. That's why I think third comment is redundant and naming a variable suffices. 2. After the closer look at the code I agree with you: NRVO won't kick in here. I'll amend it to facilitate URVO. Michael Park wrote: 1. Sorry! I missed the above comments. 2. Well, in this case it will. But it also doesn't matter since the only member in `Duration` is a single `int64_t`. All I was saying is that in general URVO is more likely to kick in than NRVO. So if there's an option to choose, we should prefer URVO (unless of course, naming a variable helps readability). Alexander Rukletsov wrote: Why would NRVO work here? We have two other paths returning rvalues. Ah, yes you're correct :) Perhaps my mistake here can support reasons for preferring URVO. - Michael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26229/#review55582 --- On Oct. 8, 2014, 9:30 a.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26229/ --- (Updated Oct. 8, 2014, 9:30 a.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 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/#review55787 --- Patch looks great! Reviews applied: [23912] All tests passed. - Mesos ReviewBot On Oct. 8, 2014, 10: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, 10: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 26229: Expose poll interval from the reaper.
On Oct. 6, 2014, 10:02 p.m., Ben Mahler wrote: 3rdparty/libprocess/src/reap.cpp, lines 124-127 https://reviews.apache.org/r/26229/diff/1/?file=710088#file710088line124 Why do you need a variable for this? Can't this just be a 'return' statement? If there's a reason to keep this statement separate from the return, please avoid 'auto', we'd like to start using it very conservatively. Alexander Rukletsov wrote: Naming the result of the expression serves two purposes here: clarity about what we calculate and return and facilitating NRWO. Since auto is replaced, I mark this issue as fixed, please reopen if you think further discussion about `return` is needed. Michael Park wrote: My thoughts on the two purposes: 1. I actually think a comment would do serve the purpose better in terms of being clear on what we're calculating. `adjustedInterval` doesn't actually give me that much more information. 2. Facilitating NRVO I don't think is a good reason. It's much more likely for URVO to kick in than NRVO. Alexander Rukletsov wrote: 1. We already have a top-level comment describing the approach and one more right before interpolation code. That's why I think third comment is redundant and naming a variable suffices. 2. After the closer look at the code I agree with you: NRVO won't kick in here. I'll amend it to facilitate URVO. Michael Park wrote: 1. Sorry! I missed the above comments. 2. Well, in this case it will. But it also doesn't matter since the only member in `Duration` is a single `int64_t`. All I was saying is that in general URVO is more likely to kick in than NRVO. So if there's an option to choose, we should prefer URVO (unless of course, naming a variable helps readability). Alexander Rukletsov wrote: Why would NRVO work here? We have two other paths returning rvalues. Michael Park wrote: Ah, yes you're correct :) Perhaps my mistake here can support reasons for preferring URVO. I have updated the patch to make use of URVO. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26229/#review55582 --- On Oct. 8, 2014, 9:30 a.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26229/ --- (Updated Oct. 8, 2014, 9:30 a.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 25848: Introducing mesos modules.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25848/ --- (Updated Oct. 8, 2014, 12:22 p.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and Timothy St. Clair. Changes --- Make return type of MM::load() void since it always succeeds; Replace CHECK with Error in MM::create(); Bugs: MESOS-1384 https://issues.apache.org/jira/browse/MESOS-1384 Repository: mesos-git Description --- Adding a first class primitive, abstraction and process for dynamic library writing and loading can make it easier to extend inner workings of Mesos. Making it possible to have dynamic loadable resource allocators, isolators, containerizes, authenticators and much more. Diffs (updated) - include/mesos/module.hpp PRE-CREATION src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724 src/examples/example_module_impl.cpp PRE-CREATION src/examples/test_module.hpp PRE-CREATION src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d src/module/manager.hpp PRE-CREATION src/module/manager.cpp PRE-CREATION src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b src/tests/module_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/25848/diff/ Testing --- Ran make check with added tests for verifying library/module loading and simple version check. Thanks, Kapil Arya
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/#review55807 --- src/slave/containerizer/docker.cpp https://reviews.apache.org/r/26436/#comment96184 __usage is a continuation call from _usage. src/slave/containerizer/docker.cpp https://reviews.apache.org/r/26436/#comment96185 It's necessary because the container can be removed in between any callbacks. - Timothy Chen On Oct. 8, 2014, 4:51 a.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26436/ --- (Updated Oct. 8, 2014, 4:51 a.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 26289: Replace some shared_ptr with Owned to clarify ownership passing.
On Oct. 7, 2014, 5:32 p.m., Jie Yu wrote: Can you use unique_ptr directly now? If yes, I would suggest using unique_ptr directly in libprocess. Here are the reasons: 1) We use lots of shared_ptr in libprocess. Mixing Owned with shared_ptr seems to be confusing. 2) What if the implementation of Owned/Shared depends on dispatch, there will be cyclic dependencies (though it's not a problem now). 3) I think directly using std::shared_ptr and std::unique_ptr is fine in libprocess (we even use pthread in libprocess:)) What do you think? Michael Park wrote: +1, I'd like to see movement toward `std::unique_ptr` and `std::shared_ptr` as well. Both are available in `gcc-4.4` and up. I discussed this with vinod and he prefered a migration approach: 1. migrate to Owned pointer to clarify ownership 2. replace Owned with unique_ptr I was originally going to move straight to unique_ptr (yes we can use it now in g++-4.4, and std::move which is important for unique_ptr in APIs) but that was his suggestion. I'm open to the alternative but we'll need to hash it out with him. - Dominic --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26289/#review55748 --- On Oct. 7, 2014, 5:15 p.m., Dominic Hamon wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26289/ --- (Updated Oct. 7, 2014, 5:15 p.m.) Review request for mesos and Jie Yu. Repository: mesos-git Description --- replace shared_ptr with Owned where ownership is being passed. Diffs - 3rdparty/libprocess/include/process/c++11/dispatch.hpp 76da2828cf36b6143d627dac66f3e0cc4416bae4 3rdparty/libprocess/include/process/defer.hpp ebe6f2db47cab2a3306288d8ebabb720e7cd8976 3rdparty/libprocess/include/process/delay.hpp 487f652c9e9b7c8c3aa8b4edc9e59789cffd8da9 3rdparty/libprocess/include/process/dispatch.hpp bceda2a2ae8963921e8e19660243a8644feab227 3rdparty/libprocess/include/process/event.hpp bf689d7270df2c8f1f5c9165d2bbcfdca06e15a8 3rdparty/libprocess/src/process.cpp d30ed636ee24d0fe6e62f69a921307bde1f32765 Diff: https://reviews.apache.org/r/26289/diff/ Testing --- make g++-4.4 and clang++-3.5 make check clang++-3.5 Thanks, Dominic Hamon
Re: Review Request 25848: Introducing mesos modules.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25848/#review55823 --- Ship it! Ship It! - Niklas Nielsen On Oct. 8, 2014, 9:22 a.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25848/ --- (Updated Oct. 8, 2014, 9:22 a.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and Timothy St. Clair. Bugs: MESOS-1384 https://issues.apache.org/jira/browse/MESOS-1384 Repository: mesos-git Description --- Adding a first class primitive, abstraction and process for dynamic library writing and loading can make it easier to extend inner workings of Mesos. Making it possible to have dynamic loadable resource allocators, isolators, containerizes, authenticators and much more. Diffs - include/mesos/module.hpp PRE-CREATION src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724 src/examples/example_module_impl.cpp PRE-CREATION src/examples/test_module.hpp PRE-CREATION src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d src/module/manager.hpp PRE-CREATION src/module/manager.cpp PRE-CREATION src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b src/tests/module_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/25848/diff/ Testing --- Ran make check with added tests for verifying library/module loading and simple version check. Thanks, Kapil Arya
Re: Review Request 26150: Libprocess Benchmark
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26150/#review55824
---
A high-level comment: It is a bit hard to understand what's going on here. The
test body is pretty dense and not well commented - mind spending a few cycles
breaking it up and make it more consumable?
Also, we need to do a style scan :-)
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/26150/#comment96201
Let's include the usual license blob.
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/26150/#comment96203
We try to use camel case, so _numIter, _maxOutstanding and so on.
Here and below.
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/26150/#comment96202
Out of curiosity - wouldn't plain '0' do?
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/26150/#comment96204
Bring '{' down on new line
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/26150/#comment96205
We try to spell out variable names. How about message?
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/26150/#comment96206
s/max_outstanding/maxOutstanding/
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/26150/#comment96208
Instead of doing char* and strlen. Can we use a std::string instead?
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/26150/#comment96210
For wrapping boolean expressions, see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Boolean_Expressions
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/26150/#comment96214
Would you mind adding a comment on what the test is doing here?
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/26150/#comment96211
+1 - can we do this the old way for now?
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/26150/#comment96212
Same here
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/26150/#comment96213
I am not sure we have graced range based loops yet. Do you have any
references to whether it is supported by our set of graced compilers?
- Niklas Nielsen
On Sept. 29, 2014, 2:38 p.m., Joris Van Remoortere wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26150/
---
(Updated Sept. 29, 2014, 2:38 p.m.)
Review request for mesos and Niklas Nielsen.
Bugs: MESOS-1840
https://issues.apache.org/jira/browse/MESOS-1840
Repository: mesos-git
Description
---
A benchmark for libprocess.
It forks num_proc times and launched num_threads libprocess processes in
each child. They are aware of the master's (parent) address and all play
ping pong with it.
This benchmark measures throughput in terms of the number of RPCs handled per
second using persistent (linked) connections.
A new test file (benchmarks) is introduced because we want to fork before
libprocess is initialized. This allows us to prevent short-circuiting of
message passing between processes under the same ProcessManager. This way we
force the execution path of the underlying event management system.
Diffs
-
3rdparty/libprocess/Makefile.am 616618e
3rdparty/libprocess/src/tests/benchmarks.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/26150/diff/
Testing
---
make check in 3rdparty/libprocess
support/mesos-style.py
Thanks,
Joris Van Remoortere
Re: Review Request 23710: Add line comments end punctuation style rule
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23710/#review55831 --- Hey Tim, did you get to address Adam's comment on the off-by-one issue? - Niklas Nielsen On Aug. 25, 2014, 5:12 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23710/ --- (Updated Aug. 25, 2014, 5:12 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Niklas Nielsen. Repository: mesos-git Description --- Review: https://reviews.apache.org/r/23710 Diffs - support/cpplint.patch 1dd69a03044c0c17ca4ee555c6c9d27ea043d4f0 support/cpplint.py bfd3390002a680b07aa3fcf785279ad19625294b support/mesos-style.py d24cb11adc06bc0ebaaa206301616c8b597f09e8 Diff: https://reviews.apache.org/r/23710/diff/ Testing --- make check Thanks, Timothy Chen
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/#review55834 --- src/slave/slave.cpp https://reviews.apache.org/r/23912/#comment96218 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. src/tests/slave_tests.cpp https://reviews.apache.org/r/23912/#comment96219 This has not been moved !? - Vinod Kone On Oct. 8, 2014, 10: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, 10: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 26150: Libprocess Benchmark
On Oct. 8, 2014, 5:32 p.m., Niklas Nielsen wrote: 3rdparty/libprocess/src/tests/benchmarks.cpp, line 200 https://reviews.apache.org/r/26150/diff/1/?file=708531#file708531line200 I am not sure we have graced range based loops yet. Do you have any references to whether it is supported by our set of graced compilers? It's not :( Not supported by `gcc-4.4`. - Michael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26150/#review55824 --- On Sept. 29, 2014, 9:38 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26150/ --- (Updated Sept. 29, 2014, 9:38 p.m.) Review request for mesos and Niklas Nielsen. Bugs: MESOS-1840 https://issues.apache.org/jira/browse/MESOS-1840 Repository: mesos-git Description --- A benchmark for libprocess. It forks num_proc times and launched num_threads libprocess processes in each child. They are aware of the master's (parent) address and all play ping pong with it. This benchmark measures throughput in terms of the number of RPCs handled per second using persistent (linked) connections. A new test file (benchmarks) is introduced because we want to fork before libprocess is initialized. This allows us to prevent short-circuiting of message passing between processes under the same ProcessManager. This way we force the execution path of the underlying event management system. Diffs - 3rdparty/libprocess/Makefile.am 616618e 3rdparty/libprocess/src/tests/benchmarks.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26150/diff/ Testing --- make check in 3rdparty/libprocess support/mesos-style.py Thanks, Joris Van Remoortere
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. 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 (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
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26436/#review55867 --- Patch looks great! Reviews applied: [26436] All tests passed. - Mesos ReviewBot 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 25848: Introducing mesos modules.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25848/#review55874 --- src/module/manager.cpp https://reviews.apache.org/r/25848/#comment96244 Just noticed this - shutdown() doesn't describe what you expect the function to do or how you use it. How about ::clear() or ::unloadAll()? - Niklas Nielsen On Oct. 8, 2014, 9:22 a.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25848/ --- (Updated Oct. 8, 2014, 9:22 a.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and Timothy St. Clair. Bugs: MESOS-1384 https://issues.apache.org/jira/browse/MESOS-1384 Repository: mesos-git Description --- Adding a first class primitive, abstraction and process for dynamic library writing and loading can make it easier to extend inner workings of Mesos. Making it possible to have dynamic loadable resource allocators, isolators, containerizes, authenticators and much more. Diffs - include/mesos/module.hpp PRE-CREATION src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724 src/examples/example_module_impl.cpp PRE-CREATION src/examples/test_module.hpp PRE-CREATION src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d src/module/manager.hpp PRE-CREATION src/module/manager.cpp PRE-CREATION src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b src/tests/module_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/25848/diff/ Testing --- Ran make check with added tests for verifying library/module loading and simple version check. Thanks, Kapil Arya
Re: Review Request 25848: Introducing mesos modules.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25848/ --- (Updated Oct. 8, 2014, 4:52 p.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and Timothy St. Clair. Changes --- Rename ModuleManager::shutdown() - ModuleManager::unloadAll(). Bugs: MESOS-1384 https://issues.apache.org/jira/browse/MESOS-1384 Repository: mesos-git Description --- Adding a first class primitive, abstraction and process for dynamic library writing and loading can make it easier to extend inner workings of Mesos. Making it possible to have dynamic loadable resource allocators, isolators, containerizes, authenticators and much more. Diffs (updated) - include/mesos/module.hpp PRE-CREATION src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724 src/examples/example_module_impl.cpp PRE-CREATION src/examples/test_module.hpp PRE-CREATION src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d src/module/manager.hpp PRE-CREATION src/module/manager.cpp PRE-CREATION src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b src/tests/module_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/25848/diff/ Testing --- Ran make check with added tests for verifying library/module loading and simple version check. Thanks, Kapil Arya
Review Request 26459: Differentiate between slave and offer ids
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26459/ --- Review request for mesos and Vinod Kone. Repository: mesos-git Description --- slave and offer ids can be identical which is confusing in logs Diffs - src/master/master.cpp f05275b00635cee82007ed851bba1cd30a7aa74f Diff: https://reviews.apache.org/r/26459/diff/ Testing --- make check Thanks, Dominic Hamon
Build failed in Jenkins: Mesos-Trunk-Ubuntu-Build-In-Src-Set-JAVA_HOME #2152
See https://builds.apache.org/job/Mesos-Trunk-Ubuntu-Build-In-Src-Set-JAVA_HOME/2152/changes Changes: [bmahler] Fixed a log line in the master. [bmahler] Eliminated redundant resource accounting in the master. [bmahler] Added validation for missing ExecutorInfo::framework_id. [bmahler] Properly deprecated ReregisterSlaveMessage::slave_id. [bmahler] Introduced a version during slave (re-)registration. [bmahler] Removed redundant logging in the Master. [bmahler] Removed the need for Master::readdSlave. [bmahler] Cleaned up Master::addFramework. -- [...truncated 76074 lines...] I1008 22:10:09.400147 22334 hierarchical_allocator_process.hpp:563] Recovered cpus(*):1; mem(*):10112; disk(*):3.70122e+06; ports(*):[31000-32000] (total allocatable: cpus(*):1; mem(*):10112; disk(*):3.70122e+06; ports(*):[31000-32000]) on slave 20141008-214550-3142697795-46263-22299-2 from framework 20141008-214550-3142697795-46263-22299- I1008 22:10:09.400185 22334 hierarchical_allocator_process.hpp:599] Framework 20141008-214550-3142697795-46263-22299- filtered slave 20141008-214550-3142697795-46263-22299-2 for 5secs I1008 22:10:09.400285 22334 hierarchical_allocator_process.hpp:563] Recovered mem(*):10240; disk(*):3.70122e+06; ports(*):[31000-32000]; cpus(*):2 (total allocatable: mem(*):10240; disk(*):3.70122e+06; ports(*):[31000-32000]; cpus(*):2) on slave 20141008-214550-3142697795-46263-22299-0 from framework 20141008-214550-3142697795-46263-22299- I1008 22:10:09.400321 22334 hierarchical_allocator_process.hpp:599] Framework 20141008-214550-3142697795-46263-22299- filtered slave 20141008-214550-3142697795-46263-22299-0 for 5secs I1008 22:10:09.400420 22334 hierarchical_allocator_process.hpp:563] Recovered mem(*):10240; disk(*):3.70122e+06; ports(*):[31000-32000]; cpus(*):2 (total allocatable: mem(*):10240; disk(*):3.70122e+06; ports(*):[31000-32000]; cpus(*):2) on slave 20141008-214550-3142697795-46263-22299-1 from framework 20141008-214550-3142697795-46263-22299- I1008 22:10:09.400454 22334 hierarchical_allocator_process.hpp:599] Framework 20141008-214550-3142697795-46263-22299- filtered slave 20141008-214550-3142697795-46263-22299-1 for 5secs I1008 22:10:10.264557 22328 monitor.cpp:140] Failed to collect resource usage for container '95b303f4-1473-42d6-be4c-6189e0fa52c5' for executor 'default' of framework '20141008-214550-3142697795-46263-22299-': Unknown container: 95b303f4-1473-42d6-be4c-6189e0fa52c5 I1008 22:10:10.264597 22328 monitor.cpp:140] Failed to collect resource usage for container 'f5826921-58a0-481f-92ec-a1b4b2c95953' for executor 'default' of framework '20141008-214550-3142697795-46263-22299-': Unknown container: f5826921-58a0-481f-92ec-a1b4b2c95953 I1008 22:10:10.399091 22334 hierarchical_allocator_process.hpp:816] Filtered mem(*):10240; disk(*):3.70122e+06; ports(*):[31000-32000]; cpus(*):2 on slave 20141008-214550-3142697795-46263-22299-0 for framework 20141008-214550-3142697795-46263-22299- I1008 22:10:10.399163 22334 hierarchical_allocator_process.hpp:816] Filtered cpus(*):1; mem(*):10112; disk(*):3.70122e+06; ports(*):[31000-32000] on slave 20141008-214550-3142697795-46263-22299-2 for framework 20141008-214550-3142697795-46263-22299- I1008 22:10:10.399199 22334 hierarchical_allocator_process.hpp:816] Filtered mem(*):10240; disk(*):3.70122e+06; ports(*):[31000-32000]; cpus(*):2 on slave 20141008-214550-3142697795-46263-22299-1 for framework 20141008-214550-3142697795-46263-22299- I1008 22:10:10.399217 22334 hierarchical_allocator_process.hpp:659] Performed allocation for 3 slaves in 219678ns 2014-10-08 22:10:10,607:10976(0x2afa44e06700):ZOO_ERROR@handle_socket_error_msg@1697: Socket [127.0.0.1:52422] zk retcode=-4, errno=111(Connection refused): server refused to accept the client I1008 22:10:11.133446 22324 master.cpp:120] No whitelist given. Advertising offers for all slaves I1008 22:10:11.265040 22333 monitor.cpp:140] Failed to collect resource usage for container '95b303f4-1473-42d6-be4c-6189e0fa52c5' for executor 'default' of framework '20141008-214550-3142697795-46263-22299-': Unknown container: 95b303f4-1473-42d6-be4c-6189e0fa52c5 I1008 22:10:11.265087 22333 monitor.cpp:140] Failed to collect resource usage for container 'f5826921-58a0-481f-92ec-a1b4b2c95953' for executor 'default' of framework '20141008-214550-3142697795-46263-22299-': Unknown container: f5826921-58a0-481f-92ec-a1b4b2c95953 I1008 22:10:11.400532 22323 hierarchical_allocator_process.hpp:816] Filtered mem(*):10240; disk(*):3.70122e+06; ports(*):[31000-32000]; cpus(*):2 on slave 20141008-214550-3142697795-46263-22299-0 for framework 20141008-214550-3142697795-46263-22299- I1008 22:10:11.400604 22323 hierarchical_allocator_process.hpp:816] Filtered mem(*):10240; disk(*):3.70122e+06; ports(*):[31000-32000]; cpus(*):2 on slave 20141008-214550-3142697795-46263-22299-1 for framework
Re: Review Request 25848: Introducing mesos modules.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25848/#review55886 --- Ship it! Alright! Kapil and I have made a few minor style/syntax changes together and this is looking good to go! Will commit now! ;-) - Benjamin Hindman On Oct. 8, 2014, 8:52 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25848/ --- (Updated Oct. 8, 2014, 8:52 p.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, and Timothy St. Clair. Bugs: MESOS-1384 https://issues.apache.org/jira/browse/MESOS-1384 Repository: mesos-git Description --- Adding a first class primitive, abstraction and process for dynamic library writing and loading can make it easier to extend inner workings of Mesos. Making it possible to have dynamic loadable resource allocators, isolators, containerizes, authenticators and much more. Diffs - include/mesos/module.hpp PRE-CREATION src/Makefile.am c62a974dcc80f3c3dd6060aee51f8367a0abe724 src/examples/example_module_impl.cpp PRE-CREATION src/examples/test_module.hpp PRE-CREATION src/messages/messages.proto 9ff06b38086010df362036c695a5222371f70f4d src/module/manager.hpp PRE-CREATION src/module/manager.cpp PRE-CREATION src/tests/master_tests.cpp 705e5f2c0f9d693946e722cef63f38f3bff0d46b src/tests/module_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/25848/diff/ Testing --- Ran make check with added tests for verifying library/module loading and simple version check. Thanks, Kapil Arya
Re: Review Request 26459: Differentiate between slave and offer ids
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26459/#review55888 --- Patch looks great! Reviews applied: [26459] All tests passed. - Mesos ReviewBot On Oct. 8, 2014, 9:55 p.m., Dominic Hamon wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26459/ --- (Updated Oct. 8, 2014, 9:55 p.m.) Review request for mesos and Vinod Kone. Repository: mesos-git Description --- slave and offer ids can be identical which is confusing in logs Diffs - src/master/master.cpp f05275b00635cee82007ed851bba1cd30a7aa74f Diff: https://reviews.apache.org/r/26459/diff/ Testing --- make check Thanks, Dominic Hamon
Re: Review Request 26229: Expose poll interval from the reaper.
On Oct. 7, 2014, 9:46 p.m., Ben Mahler wrote: I'm curious why you need to expose both sides of the bounds? Our tests currently hard-code 1 second as the reap interval, and since Ian did not change the maximum, the tests continue to function as expected. Are you planning to follow up and adjust the tests? E.g. subprocess_tests.cpp, slave_recovery_tests.cpp, etc. Naming wise, how about: ``` process::REAP_INTERVAL(); ``` The examples in the ticket you referenced fall into two cases: (1) Static member functions (e.g. Duration::max() and Time::epoch()), and (2) Global functions (e.g. MAX_PING_TIMEOUT()). Since for (2) we've kept ALL_CAPS, we should stick to ALL_CAPS here for consistency. The benefit of ALL_CAPS for globals is that it provides a signal to the reader of the code that this is a constant value, despite the fact that we had to make it a function (for non-POD global safety). Alexander Rukletsov wrote: The lower bound may be needed for testing situation when the reaper should not kick in, e.g. by setting the graceful shutdown timeout to the lower bound we test the force shutdwon path. The idea for the patch came when I had to hard-code both 1s and 100ms in my tests. I would propose we create a separate JIRA for updating the tests. Names are updated to `LOW_REAP_INTERVAL()` and `HIGH_REAP_INTERVAL()`. by setting the graceful shutdown timeout to the lower bound we test the force shutdwon path I didn't quite grasp this, and it sounds a bit racy to me. Can you explain a bit more or point me to the patch where you need to know the minimum? I would grab you on IRC but I don't see you on there. :) - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26229/#review55718 --- On Oct. 8, 2014, 9:30 a.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26229/ --- (Updated Oct. 8, 2014, 9:30 a.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
Build failed in Jenkins: mesos-reviewbot #1899
See https://builds.apache.org/job/mesos-reviewbot/1899/changes
Changes:
[benjamin.hindman] Added style/syntax changes made to modules abstractions.
--
Started by an SCM change
Building remotely on ubuntu-1 (docker Ubuntu ubuntu) in workspace
https://builds.apache.org/job/mesos-reviewbot/ws/
git rev-parse --is-inside-work-tree
Fetching changes from the remote Git repository
git config remote.origin.url
https://git-wip-us.apache.org/repos/asf/mesos.git
Fetching upstream changes from https://git-wip-us.apache.org/repos/asf/mesos.git
git --version
git fetch --tags --progress
https://git-wip-us.apache.org/repos/asf/mesos.git
+refs/heads/*:refs/remotes/origin/*
git rev-parse origin/master^{commit}
Checking out Revision 93701846c88e1da3f45cb7b14b54d81f9f3bc68f (origin/master)
git config core.sparsecheckout
git checkout -f 93701846c88e1da3f45cb7b14b54d81f9f3bc68f
git rev-list bf1e343c4745420b8b77e7c9213dc6515375
git tag -a -f -m Jenkins Build #1899 jenkins-mesos-reviewbot-1899
[mesos-reviewbot] $ /bin/bash -xe /tmp/hudson3423355435651075875.sh
+ export JAVA_HOME=/home/jenkins/tools/java/jdk1.6.0_20-64
+ JAVA_HOME=/home/jenkins/tools/java/jdk1.6.0_20-64
+ export
PATH=/home/jenkins/tools/java/jdk1.6.0_20-64/bin:/home/jenkins/tools/java/latest1.6/bin:/home/jenkins/tools/java/latest1.6/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
+
PATH=/home/jenkins/tools/java/jdk1.6.0_20-64/bin:/home/jenkins/tools/java/latest1.6/bin:/home/jenkins/tools/java/latest1.6/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
+ export M2_HOME=/home/jenkins/tools/maven/latest
+ M2_HOME=/home/jenkins/tools/maven/latest
+ export
PATH=/home/jenkins/tools/maven/latest/bin:/home/jenkins/tools/java/jdk1.6.0_20-64/bin:/home/jenkins/tools/java/latest1.6/bin:/home/jenkins/tools/java/latest1.6/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
+
PATH=/home/jenkins/tools/maven/latest/bin:/home/jenkins/tools/java/jdk1.6.0_20-64/bin:/home/jenkins/tools/java/latest1.6/bin:/home/jenkins/tools/java/latest1.6/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
+ date
Wed Oct 8 23:21:07 UTC 2014
+ chmod -R +w 3rdparty bin bootstrap CHANGELOG configure.ac docs Doxyfile ec2
frameworks include LICENSE m4 Makefile.am mesos.pc.in mpi NOTICE README.md src
support
+ git clean -fdx
+ git reset --hard HEAD
HEAD is now at 9370184 Added style/syntax changes made to modules abstractions.
+ ./support/mesos-style.py
Checking 518 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/module/manager.cpp:194: Tab found; better to use spaces [whitespace/tab]
[1]
src/module/manager.cpp:199: Tab found; better to use spaces [whitespace/tab]
[1]
src/module/manager.cpp:200: Tab found; better to use spaces [whitespace/tab]
[1]
Total errors found: 3
Build step 'Execute shell' marked build as failure
Re: Review Request 26289: Replace some shared_ptr with Owned to clarify ownership passing.
On Oct. 8, 2014, 12:32 a.m., Jie Yu wrote: Can you use unique_ptr directly now? If yes, I would suggest using unique_ptr directly in libprocess. Here are the reasons: 1) We use lots of shared_ptr in libprocess. Mixing Owned with shared_ptr seems to be confusing. 2) What if the implementation of Owned/Shared depends on dispatch, there will be cyclic dependencies (though it's not a problem now). 3) I think directly using std::shared_ptr and std::unique_ptr is fine in libprocess (we even use pthread in libprocess:)) What do you think? Michael Park wrote: +1, I'd like to see movement toward `std::unique_ptr` and `std::shared_ptr` as well. Both are available in `gcc-4.4` and up. Dominic Hamon wrote: I discussed this with vinod and he prefered a migration approach: 1. migrate to Owned pointer to clarify ownership 2. replace Owned with unique_ptr I was originally going to move straight to unique_ptr (yes we can use it now in g++-4.4, and std::move which is important for unique_ptr in APIs) but that was his suggestion. I'm open to the alternative but we'll need to hash it out with him. Talked to Vinod. The consensus is: for libprocess primitives (essential for the runtime like dispatch, delay, Future, ProcessBase, SocketManager, etc.), we should use std::unique_ptr and std::shared_ptr directly. For Mesos code, try to use Owned and Shared as much as possible. - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26289/#review55748 --- On Oct. 8, 2014, 12:15 a.m., Dominic Hamon wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26289/ --- (Updated Oct. 8, 2014, 12:15 a.m.) Review request for mesos and Jie Yu. Repository: mesos-git Description --- replace shared_ptr with Owned where ownership is being passed. Diffs - 3rdparty/libprocess/include/process/c++11/dispatch.hpp 76da2828cf36b6143d627dac66f3e0cc4416bae4 3rdparty/libprocess/include/process/defer.hpp ebe6f2db47cab2a3306288d8ebabb720e7cd8976 3rdparty/libprocess/include/process/delay.hpp 487f652c9e9b7c8c3aa8b4edc9e59789cffd8da9 3rdparty/libprocess/include/process/dispatch.hpp bceda2a2ae8963921e8e19660243a8644feab227 3rdparty/libprocess/include/process/event.hpp bf689d7270df2c8f1f5c9165d2bbcfdca06e15a8 3rdparty/libprocess/src/process.cpp d30ed636ee24d0fe6e62f69a921307bde1f32765 Diff: https://reviews.apache.org/r/26289/diff/ Testing --- make g++-4.4 and clang++-3.5 make check clang++-3.5 Thanks, Dominic Hamon
Build failed in Jenkins: mesos-reviewbot #1900
See https://builds.apache.org/job/mesos-reviewbot/1900/
--
[URLTrigger] A change within the response URL invocation (log)
Building remotely on ubuntu-5 (docker Ubuntu ubuntu5 ubuntu) in workspace
https://builds.apache.org/job/mesos-reviewbot/ws/
git rev-parse --is-inside-work-tree
Fetching changes from the remote Git repository
git config remote.origin.url
https://git-wip-us.apache.org/repos/asf/mesos.git
Fetching upstream changes from https://git-wip-us.apache.org/repos/asf/mesos.git
git --version
git fetch --tags --progress
https://git-wip-us.apache.org/repos/asf/mesos.git
+refs/heads/*:refs/remotes/origin/*
git rev-parse origin/master^{commit}
Checking out Revision 93701846c88e1da3f45cb7b14b54d81f9f3bc68f (origin/master)
git config core.sparsecheckout
git checkout -f 93701846c88e1da3f45cb7b14b54d81f9f3bc68f
git rev-list 93701846c88e1da3f45cb7b14b54d81f9f3bc68f
git tag -a -f -m Jenkins Build #1900 jenkins-mesos-reviewbot-1900
[mesos-reviewbot] $ /bin/bash -xe /tmp/hudson1051060795229853582.sh
+ export JAVA_HOME=/home/jenkins/tools/java/jdk1.6.0_20-64
+ JAVA_HOME=/home/jenkins/tools/java/jdk1.6.0_20-64
+ export
PATH=/home/jenkins/tools/java/jdk1.6.0_20-64/bin:/home/jenkins/tools/java/latest1.6/bin:/home/jenkins/tools/java/latest1.6/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
+
PATH=/home/jenkins/tools/java/jdk1.6.0_20-64/bin:/home/jenkins/tools/java/latest1.6/bin:/home/jenkins/tools/java/latest1.6/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
+ export M2_HOME=/home/jenkins/tools/maven/latest
+ M2_HOME=/home/jenkins/tools/maven/latest
+ export
PATH=/home/jenkins/tools/maven/latest/bin:/home/jenkins/tools/java/jdk1.6.0_20-64/bin:/home/jenkins/tools/java/latest1.6/bin:/home/jenkins/tools/java/latest1.6/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
+
PATH=/home/jenkins/tools/maven/latest/bin:/home/jenkins/tools/java/jdk1.6.0_20-64/bin:/home/jenkins/tools/java/latest1.6/bin:/home/jenkins/tools/java/latest1.6/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
+ date
Wed Oct 8 23:31:47 UTC 2014
+ chmod -R +w 3rdparty bin bootstrap CHANGELOG configure.ac docs Doxyfile ec2
frameworks include LICENSE m4 Makefile.am mesos-0.20.0 mesos.pc.in mpi NOTICE
README.md src support
+ git clean -fdx
Skipping repository mesos-0.20.0/_build/3rdparty/leveldb
+ git reset --hard HEAD
HEAD is now at 9370184 Added style/syntax changes made to modules abstractions.
+ ./support/mesos-style.py
Checking 518 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/module/manager.cpp:194: Tab found; better to use spaces [whitespace/tab]
[1]
src/module/manager.cpp:199: Tab found; better to use spaces [whitespace/tab]
[1]
src/module/manager.cpp:200: Tab found; better to use spaces [whitespace/tab]
[1]
Total errors found: 3
Build step 'Execute shell' marked build as failure
Review Request 26467: Replaced hard tabs with spaces in module/manager.cpp.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26467/ --- Review request for mesos and Niklas Nielsen. Repository: mesos-git Description --- Replaced hard tabs with spaces in module/manager.cpp. Diffs - src/module/manager.cpp 72041c06b28191dea244a39ba99666e53198decc Diff: https://reviews.apache.org/r/26467/diff/ Testing --- Thanks, Kapil Arya
Re: Review Request 26467: Replaced hard tabs with spaces in module/manager.cpp.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26467/#review55896 --- Ship it! Ship It! - Niklas Nielsen On Oct. 8, 2014, 4:33 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26467/ --- (Updated Oct. 8, 2014, 4:33 p.m.) Review request for mesos and Niklas Nielsen. Repository: mesos-git Description --- Replaced hard tabs with spaces in module/manager.cpp. Diffs - src/module/manager.cpp 72041c06b28191dea244a39ba99666e53198decc Diff: https://reviews.apache.org/r/26467/diff/ Testing --- Thanks, Kapil Arya
Re: Build failed in Jenkins: mesos-reviewbot #1900
+Kapil, benh
Can you guys take a look?
Kapil, can you use the pre-commit hook to catch this kind of error?
http://mesos.apache.org/documentation/latest/mesos-developers-guide/
https://github.com/apache/mesos/blob/master/support/hooks/pre-commit#L10
On Wed, Oct 8, 2014 at 4:32 PM, Apache Jenkins Server
jenk...@builds.apache.org wrote:
See https://builds.apache.org/job/mesos-reviewbot/1900/
--
[URLTrigger] A change within the response URL invocation (log)
Building remotely on ubuntu-5 (docker Ubuntu ubuntu5 ubuntu) in workspace
https://builds.apache.org/job/mesos-reviewbot/ws/
git rev-parse --is-inside-work-tree
Fetching changes from the remote Git repository
git config remote.origin.url
https://git-wip-us.apache.org/repos/asf/mesos.git
Fetching upstream changes from
https://git-wip-us.apache.org/repos/asf/mesos.git
git --version
git fetch --tags --progress
https://git-wip-us.apache.org/repos/asf/mesos.git
+refs/heads/*:refs/remotes/origin/*
git rev-parse origin/master^{commit}
Checking out Revision 93701846c88e1da3f45cb7b14b54d81f9f3bc68f
(origin/master)
git config core.sparsecheckout
git checkout -f 93701846c88e1da3f45cb7b14b54d81f9f3bc68f
git rev-list 93701846c88e1da3f45cb7b14b54d81f9f3bc68f
git tag -a -f -m Jenkins Build #1900 jenkins-mesos-reviewbot-1900
[mesos-reviewbot] $ /bin/bash -xe /tmp/hudson1051060795229853582.sh
+ export JAVA_HOME=/home/jenkins/tools/java/jdk1.6.0_20-64
+ JAVA_HOME=/home/jenkins/tools/java/jdk1.6.0_20-64
+ export
PATH=/home/jenkins/tools/java/jdk1.6.0_20-64/bin:/home/jenkins/tools/java/latest1.6/bin:/home/jenkins/tools/java/latest1.6/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
+
PATH=/home/jenkins/tools/java/jdk1.6.0_20-64/bin:/home/jenkins/tools/java/latest1.6/bin:/home/jenkins/tools/java/latest1.6/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
+ export M2_HOME=/home/jenkins/tools/maven/latest
+ M2_HOME=/home/jenkins/tools/maven/latest
+ export
PATH=/home/jenkins/tools/maven/latest/bin:/home/jenkins/tools/java/jdk1.6.0_20-64/bin:/home/jenkins/tools/java/latest1.6/bin:/home/jenkins/tools/java/latest1.6/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
+
PATH=/home/jenkins/tools/maven/latest/bin:/home/jenkins/tools/java/jdk1.6.0_20-64/bin:/home/jenkins/tools/java/latest1.6/bin:/home/jenkins/tools/java/latest1.6/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
+ date
Wed Oct 8 23:31:47 UTC 2014
+ chmod -R +w 3rdparty bin bootstrap CHANGELOG configure.ac docs Doxyfile
ec2 frameworks include LICENSE m4 Makefile.am mesos-0.20.0 mesos.pc.in
mpi NOTICE README.md src support
+ git clean -fdx
Skipping repository mesos-0.20.0/_build/3rdparty/leveldb
+ git reset --hard HEAD
HEAD is now at 9370184 Added style/syntax changes made to modules
abstractions.
+ ./support/mesos-style.py
Checking 518 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/module/manager.cpp:194: Tab found; better to use spaces
[whitespace/tab] [1]
src/module/manager.cpp:199: Tab found; better to use spaces
[whitespace/tab] [1]
src/module/manager.cpp:200: Tab found; better to use spaces
[whitespace/tab] [1]
Total errors found: 3
Build step 'Execute shell' marked build as failure
Re: Build failed in Jenkins: mesos-reviewbot #1900
Fixed in https://reviews.apache.org/r/26467/ - sorry for the noise
Niklas
On 8 October 2014 16:32, Apache Jenkins Server jenk...@builds.apache.org
wrote:
See https://builds.apache.org/job/mesos-reviewbot/1900/
--
[URLTrigger] A change within the response URL invocation (log)
Building remotely on ubuntu-5 (docker Ubuntu ubuntu5 ubuntu) in workspace
https://builds.apache.org/job/mesos-reviewbot/ws/
git rev-parse --is-inside-work-tree
Fetching changes from the remote Git repository
git config remote.origin.url
https://git-wip-us.apache.org/repos/asf/mesos.git
Fetching upstream changes from
https://git-wip-us.apache.org/repos/asf/mesos.git
git --version
git fetch --tags --progress
https://git-wip-us.apache.org/repos/asf/mesos.git
+refs/heads/*:refs/remotes/origin/*
git rev-parse origin/master^{commit}
Checking out Revision 93701846c88e1da3f45cb7b14b54d81f9f3bc68f
(origin/master)
git config core.sparsecheckout
git checkout -f 93701846c88e1da3f45cb7b14b54d81f9f3bc68f
git rev-list 93701846c88e1da3f45cb7b14b54d81f9f3bc68f
git tag -a -f -m Jenkins Build #1900 jenkins-mesos-reviewbot-1900
[mesos-reviewbot] $ /bin/bash -xe /tmp/hudson1051060795229853582.sh
+ export JAVA_HOME=/home/jenkins/tools/java/jdk1.6.0_20-64
+ JAVA_HOME=/home/jenkins/tools/java/jdk1.6.0_20-64
+ export
PATH=/home/jenkins/tools/java/jdk1.6.0_20-64/bin:/home/jenkins/tools/java/latest1.6/bin:/home/jenkins/tools/java/latest1.6/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
+
PATH=/home/jenkins/tools/java/jdk1.6.0_20-64/bin:/home/jenkins/tools/java/latest1.6/bin:/home/jenkins/tools/java/latest1.6/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
+ export M2_HOME=/home/jenkins/tools/maven/latest
+ M2_HOME=/home/jenkins/tools/maven/latest
+ export
PATH=/home/jenkins/tools/maven/latest/bin:/home/jenkins/tools/java/jdk1.6.0_20-64/bin:/home/jenkins/tools/java/latest1.6/bin:/home/jenkins/tools/java/latest1.6/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
+
PATH=/home/jenkins/tools/maven/latest/bin:/home/jenkins/tools/java/jdk1.6.0_20-64/bin:/home/jenkins/tools/java/latest1.6/bin:/home/jenkins/tools/java/latest1.6/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
+ date
Wed Oct 8 23:31:47 UTC 2014
+ chmod -R +w 3rdparty bin bootstrap CHANGELOG configure.ac docs Doxyfile
ec2 frameworks include LICENSE m4 Makefile.am mesos-0.20.0 mesos.pc.in
mpi NOTICE README.md src support
+ git clean -fdx
Skipping repository mesos-0.20.0/_build/3rdparty/leveldb
+ git reset --hard HEAD
HEAD is now at 9370184 Added style/syntax changes made to modules
abstractions.
+ ./support/mesos-style.py
Checking 518 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/module/manager.cpp:194: Tab found; better to use spaces
[whitespace/tab] [1]
src/module/manager.cpp:199: Tab found; better to use spaces
[whitespace/tab] [1]
src/module/manager.cpp:200: Tab found; better to use spaces
[whitespace/tab] [1]
Total errors found: 3
Build step 'Execute shell' marked build as failure
Re: Review Request 26208: Added a test for the Master - Slave reconciliation race.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26208/ --- (Updated Oct. 8, 2014, 11:39 p.m.) Review request for mesos and Vinod Kone. Changes --- Vinod's review. Bugs: MESOS-1696 https://issues.apache.org/jira/browse/MESOS-1696 Repository: mesos-git Description --- See summary. Diffs (updated) - src/tests/master_slave_reconciliation_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26208/diff/ Testing --- Ran this new test in repetition. Thanks, Ben Mahler
Re: Review Request 26206: Introduced Master - Slave reconciliation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26206/ --- (Updated Oct. 8, 2014, 11:39 p.m.) Review request for mesos and Vinod Kone. Changes --- The TASK_LOST during master - slave reconciliation is now sent through the StatusUpdateManager, due to MESOS-1879. Bugs: MESOS-1696 https://issues.apache.org/jira/browse/MESOS-1696 Repository: mesos-git Description --- The master must rely on the slave to reconcile tasks that were missing in the re-registration message. Otherwise, the master may incorrectly send TASK_LOST in the event of a race. See MESOS-1696 for further details. Diffs (updated) - src/master/master.hpp 37ce31abb45b6d1c4a9c88b0f1e81d1265d382b9 src/master/master.cpp 0286353babdb1ef44ed954e19f02998bc272a6c7 src/messages/messages.proto b8039efa1638995c2846f5cb515919d5e51cde5c src/slave/slave.hpp 28697102047b972ecb3b6b627ee089b430549fc0 src/slave/slave.cpp 809b008b1502b80cce4d8b4be0a233117c92ed56 src/tests/fault_tolerance_tests.cpp e8f532232c091849489971d7fc96ae615ffb6de0 Diff: https://reviews.apache.org/r/26206/diff/ Testing --- make check, and modified the test that captured the TASK_LOST case. Added another test in a subsequent review. Thanks, Ben Mahler
Jenkins build is back to normal : mesos-reviewbot #1901
See https://builds.apache.org/job/mesos-reviewbot/1901/changes
Re: Review Request 26208: Added a test for the Master - Slave reconciliation race.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26208/ --- (Updated Oct. 9, 2014, 12:11 a.m.) Review request for mesos and Vinod Kone. Changes --- Remove dependency. Bugs: MESOS-1696 https://issues.apache.org/jira/browse/MESOS-1696 Repository: mesos-git Description --- See summary. Diffs - src/tests/master_slave_reconciliation_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26208/diff/ Testing --- Ran this new test in repetition. Thanks, Ben Mahler
Re: Review Request 26206: Introduced Master - Slave reconciliation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26206/ --- (Updated Oct. 9, 2014, 12:14 a.m.) Review request for mesos and Vinod Kone. Changes --- Removed submitted dependencies for reviewbot. Bugs: MESOS-1696 https://issues.apache.org/jira/browse/MESOS-1696 Repository: mesos-git Description --- The master must rely on the slave to reconcile tasks that were missing in the re-registration message. Otherwise, the master may incorrectly send TASK_LOST in the event of a race. See MESOS-1696 for further details. Diffs - src/master/master.hpp 37ce31abb45b6d1c4a9c88b0f1e81d1265d382b9 src/master/master.cpp 0286353babdb1ef44ed954e19f02998bc272a6c7 src/messages/messages.proto b8039efa1638995c2846f5cb515919d5e51cde5c src/slave/slave.hpp 28697102047b972ecb3b6b627ee089b430549fc0 src/slave/slave.cpp 809b008b1502b80cce4d8b4be0a233117c92ed56 src/tests/fault_tolerance_tests.cpp e8f532232c091849489971d7fc96ae615ffb6de0 Diff: https://reviews.apache.org/r/26206/diff/ Testing --- make check, and modified the test that captured the TASK_LOST case. Added another test in a subsequent review. Thanks, Ben Mahler
Re: Review Request 26208: Added a test for the Master - Slave reconciliation race.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26208/ --- (Updated Oct. 9, 2014, 12:15 a.m.) Review request for mesos and Vinod Kone. Changes --- Re-added dependency for reviewbot. Bugs: MESOS-1696 https://issues.apache.org/jira/browse/MESOS-1696 Repository: mesos-git Description --- See summary. Diffs - src/tests/master_slave_reconciliation_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26208/diff/ Testing --- Ran this new test in repetition. Thanks, Ben Mahler
Jenkins build is back to normal : Mesos-Trunk-Ubuntu-Build-In-Src-Set-JAVA_HOME #2153
See https://builds.apache.org/job/Mesos-Trunk-Ubuntu-Build-In-Src-Set-JAVA_HOME/2153/changes
Re: Review Request 26208: Added a test for the Master - Slave reconciliation race.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26208/#review55914 --- Bad patch! Reviews applied: [26206, 26207] Failed command: git apply --index 26207.patch Error: error: patch failed: src/Makefile.am:1104 error: src/Makefile.am: patch does not apply - Mesos ReviewBot On Oct. 9, 2014, 12:15 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26208/ --- (Updated Oct. 9, 2014, 12:15 a.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-1696 https://issues.apache.org/jira/browse/MESOS-1696 Repository: mesos-git Description --- See summary. Diffs - src/tests/master_slave_reconciliation_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26208/diff/ Testing --- Ran this new test in repetition. Thanks, Ben Mahler
Review Request 26470: Removed unused resources and duplicated attributes from Slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26470/ --- Review request for mesos, Ben Mahler and Vinod Kone. Repository: mesos-git Description --- See summary. Right now, the 'resources' obtained from localhost:5051/state.json is not correct. Diffs - src/slave/http.cpp ec7c6b992f72246db0044734785c4d851a27734a src/slave/slave.hpp 28697102047b972ecb3b6b627ee089b430549fc0 src/slave/slave.cpp 809b008b1502b80cce4d8b4be0a233117c92ed56 Diff: https://reviews.apache.org/r/26470/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 26470: Removed unused resources and duplicated attributes from Slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26470/#review55920 --- Should probably file a ticket for the state.json inaccuracy? A test would be nice if you're feeling up for it! :) - Ben Mahler On Oct. 9, 2014, 12:29 a.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26470/ --- (Updated Oct. 9, 2014, 12:29 a.m.) Review request for mesos, Ben Mahler and Vinod Kone. Repository: mesos-git Description --- See summary. Right now, the 'resources' obtained from localhost:5051/state.json is not correct. Diffs - src/slave/http.cpp ec7c6b992f72246db0044734785c4d851a27734a src/slave/slave.hpp 28697102047b972ecb3b6b627ee089b430549fc0 src/slave/slave.cpp 809b008b1502b80cce4d8b4be0a233117c92ed56 Diff: https://reviews.apache.org/r/26470/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 26159: Fixed framework logging in master.cpp.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26159/ --- (Updated Oct. 9, 2014, 12:41 a.m.) Review request for mesos and Ben Mahler. Changes --- benm's. NNFR. Repository: mesos-git Description --- Like we did for slave, wanted to standardize how we log about a framework in master.cpp. Included framework.name() because I think it's more useful for debugging in a multi-framework world. No semantic changes. Diffs (updated) - src/master/master.hpp 5c0f224f196bc85e012b550d01b954a8f2079b50 src/master/master.cpp 86dc544da56a7c64d92c902cc41805af7dcdcef4 Diff: https://reviews.apache.org/r/26159/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 26206: Introduced Master - Slave reconciliation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26206/#review55922 --- Ship it! src/slave/slave.cpp https://reviews.apache.org/r/26206/#comment96291 Kill this. I think we should always use SUM in the slave when sending updates. - Vinod Kone On Oct. 9, 2014, 12:14 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26206/ --- (Updated Oct. 9, 2014, 12:14 a.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-1696 https://issues.apache.org/jira/browse/MESOS-1696 Repository: mesos-git Description --- The master must rely on the slave to reconcile tasks that were missing in the re-registration message. Otherwise, the master may incorrectly send TASK_LOST in the event of a race. See MESOS-1696 for further details. Diffs - src/master/master.hpp 37ce31abb45b6d1c4a9c88b0f1e81d1265d382b9 src/master/master.cpp 0286353babdb1ef44ed954e19f02998bc272a6c7 src/messages/messages.proto b8039efa1638995c2846f5cb515919d5e51cde5c src/slave/slave.hpp 28697102047b972ecb3b6b627ee089b430549fc0 src/slave/slave.cpp 809b008b1502b80cce4d8b4be0a233117c92ed56 src/tests/fault_tolerance_tests.cpp e8f532232c091849489971d7fc96ae615ffb6de0 Diff: https://reviews.apache.org/r/26206/diff/ Testing --- make check, and modified the test that captured the TASK_LOST case. Added another test in a subsequent review. Thanks, Ben Mahler
Re: Review Request 26206: Introduced Master - Slave reconciliation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26206/ --- (Updated Oct. 9, 2014, 1:12 a.m.) Review request for mesos and Vinod Kone. Changes --- This also fixes MESOS-1869. Bugs: MESOS-1696 and MESOS-1869 https://issues.apache.org/jira/browse/MESOS-1696 https://issues.apache.org/jira/browse/MESOS-1869 Repository: mesos-git Description --- The master must rely on the slave to reconcile tasks that were missing in the re-registration message. Otherwise, the master may incorrectly send TASK_LOST in the event of a race. See MESOS-1696 for further details. Diffs - src/master/master.hpp 37ce31abb45b6d1c4a9c88b0f1e81d1265d382b9 src/master/master.cpp 0286353babdb1ef44ed954e19f02998bc272a6c7 src/messages/messages.proto b8039efa1638995c2846f5cb515919d5e51cde5c src/slave/slave.hpp 28697102047b972ecb3b6b627ee089b430549fc0 src/slave/slave.cpp 809b008b1502b80cce4d8b4be0a233117c92ed56 src/tests/fault_tolerance_tests.cpp e8f532232c091849489971d7fc96ae615ffb6de0 Diff: https://reviews.apache.org/r/26206/diff/ Testing --- make check, and modified the test that captured the TASK_LOST case. Added another test in a subsequent review. Thanks, Ben Mahler
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, 1:18 a.m.) Review request for mesos and Vinod Kone. Changes --- set Groups and Bugs. 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 26470: Removed unused resources and duplicated attributes from Slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26470/#review55929 --- Patch looks great! Reviews applied: [26470] All tests passed. - Mesos ReviewBot On Oct. 9, 2014, 12:29 a.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26470/ --- (Updated Oct. 9, 2014, 12:29 a.m.) Review request for mesos, Ben Mahler and Vinod Kone. Repository: mesos-git Description --- See summary. Right now, the 'resources' obtained from localhost:5051/state.json is not correct. Diffs - src/slave/http.cpp ec7c6b992f72246db0044734785c4d851a27734a src/slave/slave.hpp 28697102047b972ecb3b6b627ee089b430549fc0 src/slave/slave.cpp 809b008b1502b80cce4d8b4be0a233117c92ed56 Diff: https://reviews.apache.org/r/26470/diff/ Testing --- make check Thanks, Jie Yu
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/ --- 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
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/ --- 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
Review Request 26476: Remove dynamic allocation from Option.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26476/ --- 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/#review55936 --- Bad patch! Reviews applied: [26382] Failed command: ./support/mesos-style.py Error: Checking 518 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/sched/sched.cpp:856: Lines should very rarely be longer than 100 characters [whitespace/line_length] [4] src/sched/sched.cpp:874: Lines should be = 80 characters long [whitespace/line_length] [2] Total errors found: 2 - Mesos ReviewBot 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 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/#review55942 --- Patch looks great! Reviews applied: [26472, 26473] All tests passed. - Mesos ReviewBot On Oct. 9, 2014, 1:20 a.m., Cody Maloney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26473/ --- (Updated Oct. 9, 2014, 1:20 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. 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 26476: Remove dynamic allocation from Option.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26476/#review55944 --- Patch looks great! Reviews applied: [26476] All tests passed. - Mesos ReviewBot 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 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/#review55948 --- Ship it! Ship It! - Dominic Hamon 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 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/#review55947 --- 3rdparty/libprocess/src/httpd.cpp https://reviews.apache.org/r/26473/#comment96312 oh no! what if malloc fails in this file?! oy... - Dominic Hamon 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 26476: Remove dynamic allocation from Option.
On Oct. 9, 2014, 3:45 a.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 This makes Option arbitrarily large which could be problematic where we copy it (we can't assume move semantics). I don't understand the benefit of this change. We have so many dynamic allocations throughout the code-base, it seems like a strange place to focus attention. In the original implementation of Option, a large T would still be deep copied; it would just be done on the heap. To avoid large copies one should use a reference counted abstraction such as shared_ptr (e.g. Optionstd::shared_ptrT or std::shared_ptrOptionT). Option is meant to augment a type with 1 extra bit of (nullable / unknownable, whichever you prefer) state. Tackling Option is one way of reducing a significant number of dynamic allocations as it is a heavily used library. Option is also something that is commonly assumed to be a light-weight abstraction; so it is less of a surprise if it doesn't have an underlying dynamic allocation. - Joris --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26476/#review55945 --- 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 26476: Remove dynamic allocation from Option.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26476/#review55951 --- Flying by. You may wanna take a look at: https://github.com/facebook/folly/blob/master/folly/Optional.h Not sure if we can use unstricted union? Does g++44 supports that? - Jie Yu 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 26476: Remove dynamic allocation from Option.
On Oct. 9, 2014, 4:41 a.m., Jie Yu wrote: Flying by. You may wanna take a look at: https://github.com/facebook/folly/blob/master/folly/Optional.h Not sure if we can use unstricted union? Does g++44 supports that? According to https://gcc.gnu.org/projects/cxx0x.html unrestricted unions are supported from gcc4.6+. In-place new is an old c-ism. - Joris --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26476/#review55951 --- 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
