Re: Review Request 32150: Enabled the master to handle reservation operations.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/#review81139
---
Looks good to me, MPark.
Several high-level questions (some of them are duplicated in the issues):
* Why don't we want to check that request `principal` matches
`FrameworkInfo.principal` in unreserve operation?
* Can we dynamically unreserve statically reserved resources? Could you please
add a test for this?
src/master/master.cpp
https://reviews.apache.org/r/32150/#comment131401
These two fields are optional, `principal` doesn't have a default. Do we
need to check it? Can a framework without a principal reserve resources (the
answer is no, I suppose, because in the current desgin a principal register
resources, not a framework)?
DO you mind adding tests for these cases?
src/master/validation.cpp
https://reviews.apache.org/r/32150/#comment131404
Let's leave a comment here that `resource::validate` not only checks for
integrity of the `resources` instance, but also for inconsistent state related
to dynamic reservations. Since `resource::validate` doesn't have any comment
it's unclear here that it does a related but non-obvious check for * role.
src/master/validation.cpp
https://reviews.apache.org/r/32150/#comment131407
Shall we check framework and request principals are the same?
src/tests/master_validation_tests.cpp
https://reviews.apache.org/r/32150/#comment131410
const?
src/tests/master_validation_tests.cpp
https://reviews.apache.org/r/32150/#comment131411
As I have mentioned earlier, how about a test where we `FrameworkInfo`
doesn't have a principal?
src/tests/master_validation_tests.cpp
https://reviews.apache.org/r/32150/#comment131412
const? here and below.
src/tests/master_validation_tests.cpp
https://reviews.apache.org/r/32150/#comment131413
You don't use `principal` in unreserve, is this comment valid then? As I
have already mentioned, I would rather check for principal match though.
src/tests/master_validation_tests.cpp
https://reviews.apache.org/r/32150/#comment131414
Don't you get an unused variable warning?
- Alexander Rukletsov
On April 15, 2015, 3:55 p.m., Michael Park wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32150/
---
(Updated April 15, 2015, 3:55 p.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
Bugs: MESOS-2139
https://issues.apache.org/jira/browse/MESOS-2139
Repository: mesos
Description
---
Handled reservation operations in `Master::_accept`.
Added `validate` functions in `src/master/validation.{hpp,cpp}`.
Diffs
-
src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b
src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec
src/master/validation.cpp 2f2e4ea8ea123c5a0d01446cdec8b308ea60932e
src/tests/master_validation_tests.cpp
4f2ad58c3ae0f611fb476c4d91a37dd6a5541395
Diff: https://reviews.apache.org/r/32150/diff/
Testing
---
make check
Thanks,
Michael Park
Re: Review Request 32140: Enabled 'Resources' to handle 'Resource::ReservationInfo'.
On March 18, 2015, 12:27 a.m., Alexander Rukletsov wrote:
src/common/resources.cpp, lines 69-74
https://reviews.apache.org/r/32140/diff/1/?file=897349#file897349line69
Not yours, but resently, Vinod did a cleanup in equivalence operators
for our proto messages in `type_utils.{hpp|cpp}`:
https://reviews.apache.org/r/31904/. I think we should do the same here for
consistency (most probably in a separate RR). Also, one more point to
extract these out to `type_utils`.
Michael Park wrote:
Hm, I actually missed that review request. That pattern of comparisons
don't always work... doesn't seem like good practice to me.
In particular, it doesn't work when unset and the default message
mean different things. Consider our `ReservationInfo` example:
The (role, reservation) pairs:
- (role, None) means statically reserved for role
- (role, { framework_id: None }) means dynamically reserved for role
If we compare by `left.reservation() == right.reservation()`, the 2
states above are considered equal because `left.reservation()` returns `{
framework_id: None }`, according to [protobuf
documentation](https://developers.google.com/protocol-buffers/docs/cpptutorial):
optional: the field may or may not be set. If an optional field value
isn't set, a default value is used. For simple types, you can specify your
own default value, as we've done for the phone number type in the example.
Otherwise, a system default is used: zero for numeric types, the empty string
for strings, false for bools. **For embedded messages, the default value is
always the default instance or prototype of the message, which has none
of its fields set. Calling the accessor to get the value of an optional (or
required) field which has not been explicitly set always returns that field's
default value.**
We actually do need to check for `has_` conditions to get it right.
```
left.has_reservation() == right.has_reservation()
(!left.has_reservation() || left.reservation() == right.reservation());
```
`DiskInfo` is another embdedded message that has all optional fields
similar to `ReservationInfo`, except its unset is equivalent to default
message. So doing `left.disk() == right.disk()` works.
Alexander Rukletsov wrote:
The problem with `has_*()` checks is that if you specify the value which
is equal to the default, you check will fail and messages will be considered
different.
Michael Park wrote:
But that's exactly what we want. The following 2 `Resources` objects
ought to be considered different.
```js
// statically reserved for ads role.
{
name: cpus,
value : 2,
role: ads,
reservation : None,
...
}
```
```js
// dynamically reserved for ads role by principal.
{
name: cpus,
value : 2,
role: ads,
reservation : { prinicipal: , },
...
}
```
Without the `has_*()` checks, the above messages are incorrectly treated
equal. It may be the case that we explicitly disallow `` `principal`s, but
in general that's not the case. My main point is that omitting the `has_*()`
checks don't work in the general case.
Michael, that's a totally valid point, but let's document it (because it
differs from a default way we comapre protobufs now) and mark the issue fixed.
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review76752
---
On April 15, 2015, 4:12 p.m., Michael Park wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/
---
(Updated April 15, 2015, 4:12 p.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
Bugs: MESOS-2476
https://issues.apache.org/jira/browse/MESOS-2476
Repository: mesos
Description
---
`Resource::ReservationInfo` was introduced in
[r32139](https://reviews.apache.org/r/32139). We need to consider it in our
`Resources` class implementation.
### `Resources::flatten`
`flatten` is used as the utility function to cross reservation boundaries
without affecting the given resources. Since the reservation is now specified
by the (`role`, `reservation`) pair, `flatten` needs to consider
`ReservationInfo` as well.
### `Resources::validate`
If `role == *`, then `reservation` field must not be set.
### `Resources` comparators
`operator==`, `addable` and `substractable` need to test for
`ReservationInfo` as well.
Diffs
-
include/mesos/resources.hpp
Re: Review Request 33358: Moved implementation of StatusUpdateStream to a compilation unit.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33358/#review81136 --- Ship it! Let's not repeat summary in description. How about the following: StatusUpdateStream is not a template, hence we can reduce compilation time by moving method definitions into a `.cpp` file. As a drive-by change headers are cleaned up. Thank you for putting effort into reducing compilation time! - Alexander Rukletsov On April 22, 2015, 9:09 a.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33358/ --- (Updated April 22, 2015, 9:09 a.m.) Review request for mesos, Alexander Rukletsov, Joerg Schad, and Till Toenshoff. Bugs: MESOS-2609 https://issues.apache.org/jira/browse/MESOS-2609 Repository: mesos Description --- Moves the implementation of StatusUpdateStream to a compilation unit. Also cleans up headers. Diffs - src/slave/status_update_manager.hpp b4d91b22b515195fdb69c89af9c2864e469e7e54 src/slave/status_update_manager.cpp fab8c22d46b8ab0a3c3745541ddc650b574bfbd4 Diff: https://reviews.apache.org/r/33358/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 32140: Enabled 'Resources' to handle 'Resource::ReservationInfo'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32140/#review81143 --- src/common/resources.cpp https://reviews.apache.org/r/32140/#comment131406 Won't this be equivalent to the auto-generated version? - Alexander Rukletsov On April 15, 2015, 4:12 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32140/ --- (Updated April 15, 2015, 4:12 p.m.) Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu. Bugs: MESOS-2476 https://issues.apache.org/jira/browse/MESOS-2476 Repository: mesos Description --- `Resource::ReservationInfo` was introduced in [r32139](https://reviews.apache.org/r/32139). We need to consider it in our `Resources` class implementation. ### `Resources::flatten` `flatten` is used as the utility function to cross reservation boundaries without affecting the given resources. Since the reservation is now specified by the (`role`, `reservation`) pair, `flatten` needs to consider `ReservationInfo` as well. ### `Resources::validate` If `role == *`, then `reservation` field must not be set. ### `Resources` comparators `operator==`, `addable` and `substractable` need to test for `ReservationInfo` as well. Diffs - include/mesos/resources.hpp 56affd45e1e71e96ff5778b43271f81b9b9a9e4d src/common/resources.cpp 2c99b6884d7296099e19e2e3182cbe11b5e1e059 src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 src/tests/resources_tests.cpp 7e0ad98c3366f647f190363a0e6b576dbfc7d415 Diff: https://reviews.apache.org/r/32140/diff/ Testing --- make check Thanks, Michael Park
Re: Review Request 32139: Added 'Resource::ReservationInfo' protobuf message.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32139/#review81140 --- Ship it! include/mesos/mesos.proto https://reviews.apache.org/r/32139/#comment131402 Let's mention this principal should match `FrameworkInfo.princpal`. Actually all other as well: `Credential.principal` and `RateLimit.principal`, but I don't think we should reference them here. - Alexander Rukletsov On April 21, 2015, 8:35 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32139/ --- (Updated April 21, 2015, 8:35 p.m.) Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu. Bugs: MESOS-2475 https://issues.apache.org/jira/browse/MESOS-2475 Repository: mesos Description --- The beginning of `Resource::ReservationInfo`. This patch only introduces the `principal` field. Diffs - include/mesos/mesos.proto 3a8e8bf303e0576c212951f6028af77e54d93537 Diff: https://reviews.apache.org/r/32139/diff/ Testing --- make check Thanks, Michael Park
Re: Review Request 32149: Enabled 'Resources::apply' to handle reservation operations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32149/#review81148 --- Ship it! Modulo Tim's and Jie's comments. - Alexander Rukletsov On April 15, 2015, 4:12 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32149/ --- (Updated April 15, 2015, 4:12 p.m.) Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu. Bugs: MESOS-2477 https://issues.apache.org/jira/browse/MESOS-2477 Repository: mesos Description --- See summary. Diffs - src/common/resources.cpp 2c99b6884d7296099e19e2e3182cbe11b5e1e059 src/tests/resources_tests.cpp 7e0ad98c3366f647f190363a0e6b576dbfc7d415 Diff: https://reviews.apache.org/r/32149/diff/ Testing --- make check Thanks, Michael Park
Re: Review Request 32844: Added SUBSCRIBE call and SUBSCRIBED event.
On April 8, 2015, 5:52 p.m., Alexander Rukletsov wrote: Looks good to me, mind update the design doc? Vinod Kone wrote: isabel has kindly agreed to update the doc by EOD. Great! - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32844/#review79378 --- On April 20, 2015, 8:03 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32844/ --- (Updated April 20, 2015, 8:03 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- Instead of REGISTER and REREGISTER we now just have SUBSCRIBE. Similarly, instead of REGISTERED and REREGISTERED there is only SUBSCRIBED. This will simplify a scheduler's registration semantics. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/examples/low_level_scheduler_libprocess.cpp 63d34eefb60d13fe2b82905c1cec9b762340e997 src/examples/low_level_scheduler_pthread.cpp 6d1f938660c02db75bfcbf7c8de0d941cff1920d src/master/master.cpp e30b951eda2b3b0d5b2a80716f0b32c6bbe041bc src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32844/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 31265: Provided a factory for allocator in tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31265/ --- (Updated April 21, 2015, 6:03 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Improved variables naming. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The factory creates allocator instances in a way identical to how instances from modules are created. It allows us to use same typed tests for built-in and modularized allocators. Diffs (updated) - src/examples/test_allocator_module.cpp PRE-CREATION src/local/local.cpp 289b9bced7bab0d745fe14823efa4e90ec36905e src/master/allocator/mesos/allocator.hpp af27a9bd8299cbff01e04b74db47c86bf247b908 src/master/main.cpp 7cce3a0bb808a1cb7bac9acab31eb1c67a15ea9f src/tests/cluster.hpp a56b6541adcdebc5866571bbdbb6828df97b34ec src/tests/hierarchical_allocator_tests.cpp 0b564a74d3f04df46fe52fcbe1bf8d4d1e41c53c src/tests/mesos.hpp 7744df55a2a31446327da7bd2b16457e90711d22 Diff: https://reviews.apache.org/r/31265/diff/ Testing --- make check (Mac OS 10.9.5, Ubuntu 14.04) Thanks, Alexander Rukletsov
Re: Review Request 31265: Provided a factory for allocator in tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31265/ --- (Updated April 21, 2015, 6:45 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Minor cleanup. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The factory creates allocator instances in a way identical to how instances from modules are created. It allows us to use same typed tests for built-in and modularized allocators. Diffs (updated) - src/examples/test_allocator_module.cpp PRE-CREATION src/local/local.cpp 289b9bced7bab0d745fe14823efa4e90ec36905e src/master/allocator/mesos/allocator.hpp af27a9bd8299cbff01e04b74db47c86bf247b908 src/master/main.cpp 7cce3a0bb808a1cb7bac9acab31eb1c67a15ea9f src/tests/cluster.hpp a56b6541adcdebc5866571bbdbb6828df97b34ec src/tests/hierarchical_allocator_tests.cpp 0b564a74d3f04df46fe52fcbe1bf8d4d1e41c53c src/tests/mesos.hpp 7744df55a2a31446327da7bd2b16457e90711d22 Diff: https://reviews.apache.org/r/31265/diff/ Testing --- make check (Mac OS 10.9.5, Ubuntu 14.04) Thanks, Alexander Rukletsov
Re: Review Request 32845: Renamed UNREGISTER call to UNSUBSCRIBE.
On April 8, 2015, 5:52 p.m., Alexander Rukletsov wrote: src/master/master.cpp, lines 1626-1628 https://reviews.apache.org/r/32845/diff/1/?file=915074#file915074line1626 Shall we check `framework-pid == from` as we do in `Master::unregisterFramework()`? Also metrcis update `++metrics-messages_unregister_framework;` should be migrated to `removeFramework()`. Vinod Kone wrote: We do this check for all calls. It is above the switch statement in this function. Metrics, I'll add later. I see it now, sorry, my bad. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32845/#review79381 --- On April 20, 2015, 8:04 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32845/ --- (Updated April 20, 2015, 8:04 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- Renamed UNREGISTER call to UNSUBSCRIBE for consistency. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/examples/low_level_scheduler_libprocess.cpp 63d34eefb60d13fe2b82905c1cec9b762340e997 src/examples/low_level_scheduler_pthread.cpp 6d1f938660c02db75bfcbf7c8de0d941cff1920d src/master/master.cpp e30b951eda2b3b0d5b2a80716f0b32c6bbe041bc src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32845/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 33358: Moved implementation of StatusUpdateStream to a compilation unit.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33358/#review80972 --- Ship it! LGTM assuming no functional changes, just code move. src/slave/status_update_manager.hpp https://reviews.apache.org/r/33358/#comment131163 Do you need them here? src/slave/status_update_manager.hpp https://reviews.apache.org/r/33358/#comment131165 Do we need this include here? src/slave/status_update_manager.hpp https://reviews.apache.org/r/33358/#comment131164 Shall this include stay here? src/slave/status_update_manager.hpp https://reviews.apache.org/r/33358/#comment131157 Not yours, but let's remove empty newlines. src/slave/status_update_manager.hpp https://reviews.apache.org/r/33358/#comment131158 `#include process/future.hpp? src/slave/status_update_manager.hpp https://reviews.apache.org/r/33358/#comment131159 `#include mesos/mesos.hpp`? src/slave/status_update_manager.hpp https://reviews.apache.org/r/33358/#comment131160 `#include stout/try.hpp?` src/slave/status_update_manager.hpp https://reviews.apache.org/r/33358/#comment131161 Why have you left this one here? It's more than one line, I think you can move it to `.cpp` as well. src/slave/status_update_manager.hpp https://reviews.apache.org/r/33358/#comment131162 `#include stout/option.hpp`? - Alexander Rukletsov On April 21, 2015, 12:38 p.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33358/ --- (Updated April 21, 2015, 12:38 p.m.) Review request for mesos, Alexander Rukletsov, Joerg Schad, and Till Toenshoff. Bugs: MESOS-2609 https://issues.apache.org/jira/browse/MESOS-2609 Repository: mesos Description --- Moves the implementation of StatusUpdateStream to a compilation unit. Also cleans up headers. Diffs - src/slave/status_update_manager.hpp b4d91b22b515195fdb69c89af9c2864e469e7e54 src/slave/status_update_manager.cpp fab8c22d46b8ab0a3c3745541ddc650b574bfbd4 Diff: https://reviews.apache.org/r/33358/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 30612: Added /master/frameworks/{framework}/tasks/{task} endpoint.
On April 21, 2015, 12:51 a.m., Marco Massenzio wrote:
src/master/http.cpp, line 300
https://reviews.apache.org/r/30612/diff/12/?file=935462#file935462line300
can we have all routes patters as CONSTANTS defined in the class
header? definitely easier to debug etc.
Marco, we tend not to use constants for non-POD types:
https://issues.apache.org/jira/browse/MESOS-1023
An example of a prefered way is `MAX_REAP_INTERVAL()` from `reap.hpp`.
In this particular case, why do you want to increase the visibility scope of
this constant?
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30612/#review80865
---
On April 20, 2015, 4:27 p.m., Alexander Rojas wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30612/
---
(Updated April 20, 2015, 4:27 p.m.)
Review request for mesos, Ben Mahler, Marco Massenzio, Niklas Nielsen, and
Till Toenshoff.
Bugs: MESOS-2157
https://issues.apache.org/jira/browse/MESOS-2157
Repository: mesos
Description
---
Adds endpoints for paths /master/frameworks/{framework}/tasks/{task}.
Adds tests for the endpoints.
Works with [29883](https://reviews.apache.org/r/29883).
Builds on the abandoned patch 14286.
Diffs
-
src/master/http.cpp 00c22c43bd1f6cef7963b2ffa9c095c6cbd01cd3
src/master/master.hpp c10e7c08c191acef9d31d98018a47a2a952a4dfc
src/master/master.cpp e30b951eda2b3b0d5b2a80716f0b32c6bbe041bc
src/tests/master_tests.cpp 32b1e9bb58d8046e5363fafe2ab8f662b6c9a666
Diff: https://reviews.apache.org/r/30612/diff/
Testing
---
make check
Thanks,
Alexander Rojas
Re: Review Request 32843: Updated KILL to include SlaveID.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32843/#review80990 --- Ship it! Modulo Ben's comments. - Alexander Rukletsov On April 20, 2015, 8:02 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32843/ --- (Updated April 20, 2015, 8:02 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- Having SlaveID will help us do better reconciliation when the task is not found. Also, updated master to handle Call::Kill. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/master/master.hpp c10e7c08c191acef9d31d98018a47a2a952a4dfc src/master/master.cpp e30b951eda2b3b0d5b2a80716f0b32c6bbe041bc src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32843/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32506: Added output stream operators for scheduler Calls and Events.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32506/#review80992 --- Ship it! Ship It! - Alexander Rukletsov On April 20, 2015, 8:02 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32506/ --- (Updated April 20, 2015, 8:02 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- For readable output in logs. Diffs - include/mesos/type_utils.hpp cdf5864389a72002b538c263d70bcade2bdffa45 src/master/master.cpp e30b951eda2b3b0d5b2a80716f0b32c6bbe041bc Diff: https://reviews.apache.org/r/32506/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 31775: Removed Master::Flags dependency from Allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31775/ --- (Updated April 20, 2015, 1:30 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- On the way to moving allocator to public includes we need to remove the dependency to internal Master::Flags type. Diffs (updated) - src/master/allocator/allocator.hpp 5e3e6138a36ddd6852ae1875cb967ad8487644e8 src/master/allocator/mesos/allocator.hpp af27a9bd8299cbff01e04b74db47c86bf247b908 src/master/allocator/mesos/hierarchical.hpp 90ac19736fbd56847db925cb497c4e3d9070af0a src/master/master.cpp e30b951eda2b3b0d5b2a80716f0b32c6bbe041bc src/tests/hierarchical_allocator_tests.cpp 0b564a74d3f04df46fe52fcbe1bf8d4d1e41c53c src/tests/mesos.hpp 7744df55a2a31446327da7bd2b16457e90711d22 Diff: https://reviews.apache.org/r/31775/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31266: Added support for allocator modules.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31266/ --- (Updated April 20, 2015, 1:45 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- See summary. Diffs (updated) - include/mesos/module/allocator.hpp PRE-CREATION src/Makefile.am d15a37365bcdd5c3906160b46b389635b38b1673 src/module/manager.cpp 82a38f06e57d034650a6ac32fd73527b38cc97b8 Diff: https://reviews.apache.org/r/31266/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31262: Moved allocator actions before TestAllocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31262/ --- (Updated April 20, 2015, 1:46 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased updated the description. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description (updated) --- The allocator actions should be defined before they are used in TestAllocator, despite it compiles now since TestAllocator is a template. Diffs (updated) - src/tests/mesos.hpp 7744df55a2a31446327da7bd2b16457e90711d22 Diff: https://reviews.apache.org/r/31262/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31263: Refactored TestAllocator and allocator text fixture.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31263/ --- (Updated April 20, 2015, 1:48 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Addressed Vinod's and MPark's comments. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description (updated) --- TestAllocator owns a pointer to a real allocator. Each test should instantiate and destroy Allocator instances explicitly to avoid not expected calls. Diffs (updated) - src/tests/containerizer.cpp 26b87ac6b16dfeaf84888e80296ef540697bd775 src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 src/tests/mesos.hpp 7744df55a2a31446327da7bd2b16457e90711d22 src/tests/slave_recovery_tests.cpp 87f4a6aab27d142fa8eb7a6571f684a6ce59687e Diff: https://reviews.apache.org/r/31263/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31776: Moved allocator to public headers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31776/ --- (Updated April 20, 2015, 1:36 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Addressed Vinod's comment. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- This is required for out-of-tree allocator modules. RoleInfo protobuf message has to be extracted into its own public proto file. Diffs (updated) - include/mesos/master/allocator.proto PRE-CREATION src/Makefile.am d15a37365bcdd5c3906160b46b389635b38b1673 src/local/local.hpp 0aa50ef4f44c347eca4b3a409f2a8d03ba321d82 src/local/local.cpp 289b9bced7bab0d745fe14823efa4e90ec36905e src/master/allocator/allocator.hpp 5e3e6138a36ddd6852ae1875cb967ad8487644e8 src/master/allocator/mesos/allocator.hpp af27a9bd8299cbff01e04b74db47c86bf247b908 src/master/main.cpp 7cce3a0bb808a1cb7bac9acab31eb1c67a15ea9f src/master/master.hpp c10e7c08c191acef9d31d98018a47a2a952a4dfc src/master/master.cpp e30b951eda2b3b0d5b2a80716f0b32c6bbe041bc src/messages/messages.proto 2d242dcfc54f9146721a1b8fbef02e4de050cf58 src/tests/cluster.hpp a56b6541adcdebc5866571bbdbb6828df97b34ec src/tests/fault_tolerance_tests.cpp a637c32f004638a110390b22cf5b626e904097cf src/tests/hierarchical_allocator_tests.cpp 0b564a74d3f04df46fe52fcbe1bf8d4d1e41c53c src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 src/tests/master_authorization_tests.cpp ac79303645cc1af337b1dca8db244113d0ba6fce src/tests/master_slave_reconciliation_tests.cpp e60f601202fcdbb4cafac190e9b09ca6ce925260 src/tests/master_tests.cpp 32b1e9bb58d8046e5363fafe2ab8f662b6c9a666 src/tests/mesos.hpp 7744df55a2a31446327da7bd2b16457e90711d22 src/tests/mesos.cpp fc534e9febed1e293076e00e0f5c3879a78df90f src/tests/rate_limiting_tests.cpp d5c00b8db30fc529bc984417a632cf99eb50eb28 Diff: https://reviews.apache.org/r/31776/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31267: Added a test allocator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/ --- (Updated April 20, 2015, 1:46 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The test allocator module is a built-in HierarchicalDRFAllocator put in a module. NOTE: Here we add harness code to load the module, tests will be wired up later in the patch stack. Diffs (updated) - src/Makefile.am d15a37365bcdd5c3906160b46b389635b38b1673 src/examples/test_allocator_module.cpp PRE-CREATION src/tests/module.hpp 65e567f21744d9a2b07b8680d45bcd1ea47f9508 src/tests/module.cpp b81144f56e94034feecf3a6a4992af078cf60a81 Diff: https://reviews.apache.org/r/31267/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Review Request 33356: Moved RoleInfo to mesos.master namespace.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33356/ --- Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESO-2160 https://issues.apache.org/jira/browse/MESO-2160 Repository: mesos Description --- Since the Allocator is a public interface now, there is no reason to keep RoleInfo in mesos.internal. It is ok to change the package because RoleInfo is used solely by Master and Allocator, hence there is no external communication affected. NOTE: It looks like ReviewBoard can't get it right again, please check the diff directly. Diffs - include/mesos/master/allocator.hpp bb40b1c7d62131b8f3af040aedfc6be4c8590a26 include/mesos/master/allocator.proto PRE-CREATION src/master/allocator/mesos/allocator.hpp af27a9bd8299cbff01e04b74db47c86bf247b908 src/master/allocator/mesos/hierarchical.hpp 90ac19736fbd56847db925cb497c4e3d9070af0a src/master/master.hpp c10e7c08c191acef9d31d98018a47a2a952a4dfc src/master/master.cpp e30b951eda2b3b0d5b2a80716f0b32c6bbe041bc src/tests/hierarchical_allocator_tests.cpp 0b564a74d3f04df46fe52fcbe1bf8d4d1e41c53c src/tests/mesos.hpp 7744df55a2a31446327da7bd2b16457e90711d22 Diff: https://reviews.apache.org/r/33356/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31776: Moved allocator to public headers.
On April 2, 2015, 6:23 p.m., Vinod Kone wrote: include/mesos/master/allocator.proto, lines 19-23 https://reviews.apache.org/r/31776/diff/6/?file=912077#file912077line19 Since allocator is within the same Unix process as Master, what is the compatibility issue here? Alexander Rukletsov wrote: The comment is related to the protobuf namespace in which `RoleInfo` lives. Not sure I'm getting what you mean by mentioning that allocator and Master are in the same Unix process. I'll create a separate review request for this. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31776/#review78690 --- On April 15, 2015, 2:16 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31776/ --- (Updated April 15, 2015, 2:16 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- This is required for out-of-tree allocator modules. RoleInfo protobuf message has to be extracted into its own public proto file. Diffs - include/mesos/master/allocator.proto PRE-CREATION src/Makefile.am d15a37365bcdd5c3906160b46b389635b38b1673 src/local/local.hpp 0aa50ef4f44c347eca4b3a409f2a8d03ba321d82 src/local/local.cpp 289b9bced7bab0d745fe14823efa4e90ec36905e src/master/allocator/allocator.hpp 91f80501aa9bc733fd53f9cb1ac0f15949a74964 src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/master/main.cpp 7cce3a0bb808a1cb7bac9acab31eb1c67a15ea9f src/master/master.hpp 6141917644b84edfed9836fa0a005d55a36880e3 src/master/master.cpp 44b0a0147f5354824d86332a67b30018634c9a36 src/messages/messages.proto 2d242dcfc54f9146721a1b8fbef02e4de050cf58 src/tests/cluster.hpp a56b6541adcdebc5866571bbdbb6828df97b34ec src/tests/fault_tolerance_tests.cpp a637c32f004638a110390b22cf5b626e904097cf src/tests/hierarchical_allocator_tests.cpp 8861bf398e4bb17b0f74eab4f4af26202447ccef src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 src/tests/master_authorization_tests.cpp ac79303645cc1af337b1dca8db244113d0ba6fce src/tests/master_slave_reconciliation_tests.cpp e60f601202fcdbb4cafac190e9b09ca6ce925260 src/tests/master_tests.cpp 32b1e9bb58d8046e5363fafe2ab8f662b6c9a666 src/tests/mesos.hpp 42e42ac425a448fcc5e93db1cef1112cbf5e67c4 src/tests/mesos.cpp fc534e9febed1e293076e00e0f5c3879a78df90f src/tests/rate_limiting_tests.cpp d5c00b8db30fc529bc984417a632cf99eb50eb28 Diff: https://reviews.apache.org/r/31776/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31265: Provided a factory for allocator in tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31265/ --- (Updated April 20, 2015, 1:50 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Refactored based on MPark's suggestions. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The factory creates allocator instances in a way identical to how instances from modules are created. It allows us to use same typed tests for built-in and modularized allocators. Diffs (updated) - src/examples/test_allocator_module.cpp PRE-CREATION src/local/local.cpp 289b9bced7bab0d745fe14823efa4e90ec36905e src/master/allocator/mesos/allocator.hpp af27a9bd8299cbff01e04b74db47c86bf247b908 src/master/main.cpp 7cce3a0bb808a1cb7bac9acab31eb1c67a15ea9f src/tests/cluster.hpp a56b6541adcdebc5866571bbdbb6828df97b34ec src/tests/hierarchical_allocator_tests.cpp 0b564a74d3f04df46fe52fcbe1bf8d4d1e41c53c src/tests/mesos.hpp 7744df55a2a31446327da7bd2b16457e90711d22 Diff: https://reviews.apache.org/r/31265/diff/ Testing --- make check (Mac OS 10.9.5, Ubuntu 14.04) Thanks, Alexander Rukletsov
Re: Review Request 31268: Wired up test allocator to allocator tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31268/ --- (Updated April 20, 2015, 1:51 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- Same typed tests are used for built-in and modularized allocators. Diffs (updated) - src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 Diff: https://reviews.apache.org/r/31268/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31265: Provided a factory for allocator in tests.
On April 17, 2015, 1:31 p.m., Michael Park wrote:
src/local/local.cpp, lines 141-145
https://reviews.apache.org/r/31265/diff/8/?file=929832#file929832line141
What's the point of setting `_allocator` here? It looks like it only
gets passed to the `new Master(...)` call.
Can we just leave `_allocator` alone and do
```
if (allocator_.isError()) {
LOG(ERROR) Failed to create an instance of
HierarchicalDRFAllocator:
allocator_.error();
allocator = NULL;
}
// Save the instance for deleting later.
allocator = allocator_.get();
```
then pass `allocator` to `new Master` below?
`_allocator` is passed by non-const pointer, so theoretically we can modify
caller's data. I can't find any code where this is the case, but I still don't
feel like doing this drive-by change. Let's create a separate JIRA if you feel
this should be fixed.
On April 17, 2015, 1:31 p.m., Michael Park wrote:
src/tests/hierarchical_allocator_tests.cpp, line 77
https://reviews.apache.org/r/31265/diff/8/?file=929836#file929836line77
I see this is the place where you wanted to use `ASSERT_SOME` but
couldn't due to the [gtest
limitation](https://code.google.com/p/googletest/wiki/AdvancedGuide#Assertion_Placement).
One way to solve this I think is to use `SetUp/TearDown` rather than
`Ctor/Dtor`.
Another way may be to use `CHECK_SOME` in place of `ASSERT_SOME`. I
think this makes sense as well since success of
`HierarchicalDRFAllocator::create` isn't actually a testing condition.
Thoughts?
Michael Park wrote:
Actually, is it possible just use a `TestAllocator` constructed from
`TestAllocator::create()` as suggested below?
Good idea!
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31265/#review80445
---
On April 15, 2015, 2:19 p.m., Alexander Rukletsov wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31265/
---
(Updated April 15, 2015, 2:19 p.m.)
Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen.
Bugs: MESOS-2160
https://issues.apache.org/jira/browse/MESOS-2160
Repository: mesos
Description
---
The factory creates allocator instances in a way identical to how instances
from modules are created. It allows us to use same typed tests for built-in
and modularized allocators.
Diffs
-
src/examples/test_allocator_module.cpp PRE-CREATION
src/local/local.cpp 289b9bced7bab0d745fe14823efa4e90ec36905e
src/master/allocator/mesos/allocator.hpp
fb898f1175b61b442204e6e38c69ccc2838a646f
src/master/main.cpp 7cce3a0bb808a1cb7bac9acab31eb1c67a15ea9f
src/tests/cluster.hpp a56b6541adcdebc5866571bbdbb6828df97b34ec
src/tests/hierarchical_allocator_tests.cpp
8861bf398e4bb17b0f74eab4f4af26202447ccef
src/tests/master_allocator_tests.cpp
03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302
src/tests/mesos.hpp 42e42ac425a448fcc5e93db1cef1112cbf5e67c4
Diff: https://reviews.apache.org/r/31265/diff/
Testing
---
make check (Mac OS 10.9.5, Ubuntu 14.04)
Thanks,
Alexander Rukletsov
Re: Review Request 31263: Refactored TestAllocator and allocator text fixture.
On April 2, 2015, 7:58 p.m., Vinod Kone wrote:
src/tests/master_allocator_tests.cpp, lines 96-104
https://reviews.apache.org/r/31263/diff/7/?file=912106#file912106line96
While the TearDown() avoids flakiness by ensuring that an allocator
process doesn't exist after a test, it doesn't address cases where a master
is restarted with a new allocator in the same test (e.g.,
FrameworkReregistersFirst and SlaveReregistersFirst) causing both old and
new allocators to coexist at the same time and talking to the same master.
I suggest to have methods on the test fixture that creates and deletes
allocator explicitly (can be used by tests that restart master). The
TearDown() will still delete any active allocator. For safety, ensure that
only one allocator is active at any given time.
Does that make sense?
```
template typename T
class MasterAllocatorTest : public MesosTest
{
protected:
MasterAllocatorTest() : allocator(Null) {}
TryTestAllocator create()
{
// We don't support multiple allocators because...
if (allocator != NULL) {
return Error(Multiple active allocators are not supported);
}
allocator = new TestAllocator(process::OwnedAllocator(new T));
return *allocator;
}
void destroy()
{
delete allocator;
allocator = NULL;
}
virtual void TearDown()
{
destroy();
MeosTest::TearDown();
}
private:
TestAllocator* allocator;
};
```
Alexander Rukletsov wrote:
We used to have a lifecycle method but decided to remove it:
be6246a11276074dfb60a892a890b80cb7cd37cd
RR: https://reviews.apache.org/r/29927/
The two tests you point to are currently the only ones that create an
additional allocator. They both create a new leader instance and appoint the
slave instance to the new leader, and there is no possibility AFAIK to swap
allocators in a leader instance. So yes, there are two active allocators in
these tests, but they belong to different leaders.
Current design ensures there is only one active allocator in the fixture,
but allows to create more if needed (what those two tests do). The design you
propose doesn't prevent us from creating one more allocator in the test body
either.
Thoughts?
Michael Park wrote:
@Vinod: Just for clarification,
causing both old and new allocators to coexist at the same time and
talking to the same master.
They do coexist at the same time, but they don't talk to the same master.
My suggestion here is to contain each of the runs in their own lexical
scope. Here's what `FrameworkReregistersFirst` could look like with this
approach:
```
TYPED_TEST(MasterAllocatorTest, FrameworkReregistersFirst)
{
// Components that last both runs.
MockExecutor exec(DEFAULT_EXECUTOR_ID);
StandaloneMasterDetector slaveDetector;
slave::Flags flags = this-CreateSlaveFlags();
flags.resources = Some(cpus:2;mem:1024);
TryPIDSlave slave = this-StartSlave(exec, slaveDetector, flags);
ASSERT_SOME(slave);
MockScheduler sched;
StandaloneMasterDetector schedulerDetector;
TestingMesosSchedulerDriver driver(sched, schedulerDetector);
driver.start();
/* run1 */ {
TestAllocator allocator1 = TestAllocator::createTypeParam();
TryPIDMaster master1 = this-StartMaster(allocator1);
ASSERT_SOME(master1);
schedulerDetector.appoint(master1.get());
slaveDetector.appoint(master1.get());
...
this-ShutdownMasters();
}
/* run2 */ {
TestAllocator allocator2 = TestAllocator::createTypeParam();
TryPIDMaster master2 = this-StartMaster(allocator2);
ASSERT_SOME(master2);
schedulerDetector.appoint(master2.get());
slaveDetector.appoint(master2.get());
...
this-ShutdownMasters();
}
// Shut everything down.
driver.stop();
driver.join();
this-Shutdown();
}
```
I think the common components, and each of the runs are clearly captured
in this approach, the lifetimes of the objects are well contained within the
runs and we could use `allocator` and `master` in both scopes as well if we
felt like it.
The `TestAllocator::create` above is suggested in
https://reviews.apache.org/r/31265.
If we find `TestAllocator::createTypeParam()` to be too verbose, we
could also provide a `createTestAllocator` alias in `MasterAllocatorTest`:
```
template typename T
class
Re: Review Request 31265: Provided a factory for allocator in tests.
On April 17, 2015, 1:31 p.m., Michael Park wrote:
src/tests/mesos.hpp, lines 866-874
https://reviews.apache.org/r/31265/diff/8/?file=929838#file929838line866
An idea to avoid this: we could promote the `createAllocator` currently
in `MasterAllocatorTest` to this file. Then we could do:
```
TestAllocator(
process::Ownedmesos::master::allocator::Allocator realAllocator =
createAllocator
mesos::internal::master::allocator::HierarchicalDRFAllocator())
```
Michael Park wrote:
Scratch that. I think we can do better. I think rather than
`createAllocator` or `createTestAllocator`, a `create` factory function for
`TestAllocator` fits nicely!
```cpp
class TestAllocator : public mesos::master::allocator::Allocator
{
public:
template typename T =
mesos::internal::master::allocator::HierarchicalDRFAllocator
static TestAllocator create() {
// T represents the allocator type. It can be a default built-in
// allocator, or one provided by an allocator module.
TryAllocator* allocator = T::create();
CHECK_SOME(allocator);
// Wrap allocator instance in TestAllocator.
process::OwnedAllocator
realAllocator(CHECK_NOTNULL(allocator.get()));
return TestAllocator(realAllocator);
}
...
private:
// No need for default argument here.
TestAllocator(process::Ownedmesos::master::allocator::Allocator
realAllocator)
: real(realAllocator)
{
...
}
};
```
The problem with the latter approach is that we won't be able to create
`TestAllocator` instances on the heap: factory returns by value (which is not
common for Mesos codebase), copy construction is not well-defined. I would like
to let users decide where they want to have allocator instance.
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31265/#review80445
---
On April 15, 2015, 2:19 p.m., Alexander Rukletsov wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31265/
---
(Updated April 15, 2015, 2:19 p.m.)
Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen.
Bugs: MESOS-2160
https://issues.apache.org/jira/browse/MESOS-2160
Repository: mesos
Description
---
The factory creates allocator instances in a way identical to how instances
from modules are created. It allows us to use same typed tests for built-in
and modularized allocators.
Diffs
-
src/examples/test_allocator_module.cpp PRE-CREATION
src/local/local.cpp 289b9bced7bab0d745fe14823efa4e90ec36905e
src/master/allocator/mesos/allocator.hpp
fb898f1175b61b442204e6e38c69ccc2838a646f
src/master/main.cpp 7cce3a0bb808a1cb7bac9acab31eb1c67a15ea9f
src/tests/cluster.hpp a56b6541adcdebc5866571bbdbb6828df97b34ec
src/tests/hierarchical_allocator_tests.cpp
8861bf398e4bb17b0f74eab4f4af26202447ccef
src/tests/master_allocator_tests.cpp
03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302
src/tests/mesos.hpp 42e42ac425a448fcc5e93db1cef1112cbf5e67c4
Diff: https://reviews.apache.org/r/31265/diff/
Testing
---
make check (Mac OS 10.9.5, Ubuntu 14.04)
Thanks,
Alexander Rukletsov
Re: Review Request 33263: Extended SlaveTest.ShutdownUnregisteredExecutor test with a reason check.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33263/#review80447 --- Ship it! Ship It! - Alexander Rukletsov On April 16, 2015, 2:31 p.m., Andrey Dyatlov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33263/ --- (Updated April 16, 2015, 2:31 p.m.) Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Till Toenshoff. Bugs: MESOS-2625 https://issues.apache.org/jira/browse/MESOS-2625 Repository: mesos Description --- Extended SlaveTest.ShutdownUnregisteredExecutor test with a reason check. Check that the reason is REASON_COMMAND_EXECUTOR_FAILED. According to the Slave::sendExecutorTerminatedStatusUpdate member function, this reason is expected instead of more general REASON_EXECUTOR_TERMINATED because the command executer is used in this test. Diffs - src/tests/slave_tests.cpp b826000e0a4221690f956ea51f49ad4c99d5e188 Diff: https://reviews.apache.org/r/33263/diff/ Testing --- make check Thanks, Andrey Dyatlov
Re: Review Request 31265: Provided a factory for allocator in tests.
On April 2, 2015, 7:59 p.m., Vinod Kone wrote: src/master/allocator/mesos/allocator.hpp, line 47 https://reviews.apache.org/r/31265/diff/6/?file=912110#file912110line47 Do you still want the constructor public? Alexander Rukletsov wrote: I think there is no reason to hide the c-tor. I can imagine somebody creating an instance on the stack and do not see why we should forbid it. Same approach is used for example for `MesosContainerizer`. On second thought I agree with you and will fix it. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31265/#review78711 --- On April 15, 2015, 2:19 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31265/ --- (Updated April 15, 2015, 2:19 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The factory creates allocator instances in a way identical to how instances from modules are created. It allows us to use same typed tests for built-in and modularized allocators. Diffs - src/examples/test_allocator_module.cpp PRE-CREATION src/local/local.cpp 289b9bced7bab0d745fe14823efa4e90ec36905e src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/master/main.cpp 7cce3a0bb808a1cb7bac9acab31eb1c67a15ea9f src/tests/cluster.hpp a56b6541adcdebc5866571bbdbb6828df97b34ec src/tests/hierarchical_allocator_tests.cpp 8861bf398e4bb17b0f74eab4f4af26202447ccef src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 src/tests/mesos.hpp 42e42ac425a448fcc5e93db1cef1112cbf5e67c4 Diff: https://reviews.apache.org/r/31265/diff/ Testing --- make check (Mac OS 10.9.5, Ubuntu 14.04) Thanks, Alexander Rukletsov
Re: Review Request 31266: Added support for allocator modules.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31266/ --- (Updated April 15, 2015, 2:17 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- See summary. Diffs (updated) - include/mesos/module/allocator.hpp PRE-CREATION src/Makefile.am d15a37365bcdd5c3906160b46b389635b38b1673 src/module/manager.cpp 82a38f06e57d034650a6ac32fd73527b38cc97b8 Diff: https://reviews.apache.org/r/31266/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31267: Added a test allocator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/ --- (Updated April 15, 2015, 2:17 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The test allocator module is a built-in HierarchicalDRFAllocator put in a module. NOTE: Here we add harness code to load the module, tests will be wired up later in the patch stack. Diffs (updated) - src/Makefile.am d15a37365bcdd5c3906160b46b389635b38b1673 src/examples/test_allocator_module.cpp PRE-CREATION src/tests/module.hpp 65e567f21744d9a2b07b8680d45bcd1ea47f9508 src/tests/module.cpp b81144f56e94034feecf3a6a4992af078cf60a81 Diff: https://reviews.apache.org/r/31267/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31265: Provided a factory for allocator in tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31265/ --- (Updated April 15, 2015, 2:19 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Made c-tor private. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The factory creates allocator instances in a way identical to how instances from modules are created. It allows us to use same typed tests for built-in and modularized allocators. Diffs (updated) - src/examples/test_allocator_module.cpp PRE-CREATION src/local/local.cpp 289b9bced7bab0d745fe14823efa4e90ec36905e src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/master/main.cpp 7cce3a0bb808a1cb7bac9acab31eb1c67a15ea9f src/tests/cluster.hpp a56b6541adcdebc5866571bbdbb6828df97b34ec src/tests/hierarchical_allocator_tests.cpp 8861bf398e4bb17b0f74eab4f4af26202447ccef src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 src/tests/mesos.hpp 42e42ac425a448fcc5e93db1cef1112cbf5e67c4 Diff: https://reviews.apache.org/r/31265/diff/ Testing --- make check (Mac OS 10.9.5, Ubuntu 14.04) Thanks, Alexander Rukletsov
Re: Review Request 31263: Refactored TestAllocator and allocator text fixture.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31263/ --- (Updated April 15, 2015, 2:18 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- TestAllocator stores a pointer to a real allocator and takes over its ownership. MasterAllocatorTest fixture stores the test allocator in turn. Diffs (updated) - src/tests/containerizer.cpp 26b87ac6b16dfeaf84888e80296ef540697bd775 src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 src/tests/mesos.hpp 42e42ac425a448fcc5e93db1cef1112cbf5e67c4 src/tests/resource_offers_tests.cpp 882a9ff4d09aace486182828bf43b643b0d0c519 src/tests/slave_recovery_tests.cpp 87f4a6aab27d142fa8eb7a6571f684a6ce59687e Diff: https://reviews.apache.org/r/31263/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31262: Moved allocator actions before TestAllocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31262/ --- (Updated April 15, 2015, 2:17 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- Once TestAllocator is not a template, the allocator actions should be defined before they are used in TestAllocator. Diffs (updated) - src/tests/mesos.hpp 42e42ac425a448fcc5e93db1cef1112cbf5e67c4 Diff: https://reviews.apache.org/r/31262/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31776: Moved allocator to public headers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31776/ --- (Updated April 15, 2015, 2:16 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- This is required for out-of-tree allocator modules. RoleInfo protobuf message has to be extracted into its own public proto file. Diffs (updated) - include/mesos/master/allocator.proto PRE-CREATION src/Makefile.am d15a37365bcdd5c3906160b46b389635b38b1673 src/local/local.hpp 0aa50ef4f44c347eca4b3a409f2a8d03ba321d82 src/local/local.cpp 289b9bced7bab0d745fe14823efa4e90ec36905e src/master/allocator/allocator.hpp 91f80501aa9bc733fd53f9cb1ac0f15949a74964 src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/master/main.cpp 7cce3a0bb808a1cb7bac9acab31eb1c67a15ea9f src/master/master.hpp 6141917644b84edfed9836fa0a005d55a36880e3 src/master/master.cpp 44b0a0147f5354824d86332a67b30018634c9a36 src/messages/messages.proto 2d242dcfc54f9146721a1b8fbef02e4de050cf58 src/tests/cluster.hpp a56b6541adcdebc5866571bbdbb6828df97b34ec src/tests/fault_tolerance_tests.cpp a637c32f004638a110390b22cf5b626e904097cf src/tests/hierarchical_allocator_tests.cpp 8861bf398e4bb17b0f74eab4f4af26202447ccef src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 src/tests/master_authorization_tests.cpp ac79303645cc1af337b1dca8db244113d0ba6fce src/tests/master_slave_reconciliation_tests.cpp e60f601202fcdbb4cafac190e9b09ca6ce925260 src/tests/master_tests.cpp 32b1e9bb58d8046e5363fafe2ab8f662b6c9a666 src/tests/mesos.hpp 42e42ac425a448fcc5e93db1cef1112cbf5e67c4 src/tests/mesos.cpp fc534e9febed1e293076e00e0f5c3879a78df90f src/tests/rate_limiting_tests.cpp d5c00b8db30fc529bc984417a632cf99eb50eb28 Diff: https://reviews.apache.org/r/31776/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31268: Wired up test allocator to allocator tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31268/ --- (Updated April 15, 2015, 2:22 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- Same typed tests are used for built-in and modularized allocators. Diffs (updated) - src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 Diff: https://reviews.apache.org/r/31268/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 32140: Enabled 'Resources' to handle 'Resource::ReservationInfo'.
On April 8, 2015, 8:16 p.m., Jie Yu wrote: src/common/resources.cpp, lines 450-459 https://reviews.apache.org/r/32140/diff/5/?file=920081#file920081line450 The semantics of this function becomes a little weired now. For example, for a resource that has `role == *` and has reservation set, `isReserved(resource, *)` is going to return `true`? Given that 'resource' is invalid, we should return a `false` in that case? Alexander Rukletsov wrote: Can we have a resource with `role == *` and reservations set? Excuse my premature comment earlier, I'm slowly starting to understand what's going on here. It looks like in the case Jie describes, the function returns `true` indeed. Which is invalid by now, but _may_ become valid in the future. On the other side, I'm not sure that returning `false` in this case is an alternative: it will read as unreserved resource, not an invalid resource. We can wrap the return value in `Try` but I prefer the way it's done now. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32140/#review79410 --- On April 15, 2015, 4:12 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32140/ --- (Updated April 15, 2015, 4:12 p.m.) Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu. Bugs: MESOS-2476 https://issues.apache.org/jira/browse/MESOS-2476 Repository: mesos Description --- `Resource::ReservationInfo` was introduced in [r32139](https://reviews.apache.org/r/32139). We need to consider it in our `Resources` class implementation. ### `Resources::flatten` `flatten` is used as the utility function to cross reservation boundaries without affecting the given resources. Since the reservation is now specified by the (`role`, `reservation`) pair, `flatten` needs to consider `ReservationInfo` as well. ### `Resources::validate` If `role == *`, then `reservation` field must not be set. ### `Resources` comparators `operator==`, `addable` and `substractable` need to test for `ReservationInfo` as well. Diffs - include/mesos/resources.hpp 56affd45e1e71e96ff5778b43271f81b9b9a9e4d src/common/resources.cpp 2c99b6884d7296099e19e2e3182cbe11b5e1e059 src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 src/tests/resources_tests.cpp 7e0ad98c3366f647f190363a0e6b576dbfc7d415 Diff: https://reviews.apache.org/r/32140/diff/ Testing --- make check Thanks, Michael Park
Re: Review Request 32140: Enable 'Resources' to handle 'Resource::ReservationInfo'.
On April 8, 2015, 8:16 p.m., Jie Yu wrote: src/common/resources.cpp, lines 450-459 https://reviews.apache.org/r/32140/diff/5/?file=920081#file920081line450 The semantics of this function becomes a little weired now. For example, for a resource that has `role == *` and has reservation set, `isReserved(resource, *)` is going to return `true`? Given that 'resource' is invalid, we should return a `false` in that case? Can we have a resource with `role == *` and reservations set? - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32140/#review79410 --- On April 7, 2015, 9:56 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32140/ --- (Updated April 7, 2015, 9:56 p.m.) Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu. Bugs: MESOS-2476 https://issues.apache.org/jira/browse/MESOS-2476 Repository: mesos Description --- `Resource::ReservationInfo` was introduced in [r32139](https://reviews.apache.org/r/32139). We need to consider it in our `Resources` class implementation. ### `Resources::flatten` `flatten` is used as the utility function to cross reservation boundaries without affecting the given resources. Since the reservation is now specified by the (`role`, `reservation`) pair, `flatten` needs to consider `ReservationInfo` as well. ### `Resources::validate` If `role == *`, then `reservation` field must not be set. ### `Resources` comparators `operator==`, `addable` and `substractable` need to test for `ReservationInfo` as well. Diffs - include/mesos/resources.hpp 56affd45e1e71e96ff5778b43271f81b9b9a9e4d src/common/resources.cpp 2c99b6884d7296099e19e2e3182cbe11b5e1e059 src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 src/tests/resources_tests.cpp 7e0ad98c3366f647f190363a0e6b576dbfc7d415 Diff: https://reviews.apache.org/r/32140/diff/ Testing --- make check Thanks, Michael Park
Re: Review Request 32140: Enabled 'Resources' to handle 'Resource::ReservationInfo'.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review80201
---
src/common/resources.cpp
https://reviews.apache.org/r/32140/#comment129980
Can we replace it by
```
if !addable(left, right) {
return false;
}
```
? Or even add `addable()` check to the chain of checks below?
src/common/resources.cpp
https://reviews.apache.org/r/32140/#comment129981
Remove period at the end.
src/common/resources.cpp
https://reviews.apache.org/r/32140/#comment129988
Agree with Jie, I spent some time trying to understand what's going on here
and what the implications are.
* `.has_reservation()` means a resource is dynamically reserved;
* `.role() != *` means a resource is reserved for a role.
Currently we do not allow reservations for * role, this is validated
above. But what is the semantics of `isUnreserved()` function? It used to be a
resource belongs to a default role. Let's write a comment to document our
intention here, mentioning all assumptions that we currently make.
src/tests/mesos.hpp
https://reviews.apache.org/r/32140/#comment129998
Why not putting the definition into `tests/mesos.cpp`? Here and below.
- Alexander Rukletsov
On April 15, 2015, 4:12 p.m., Michael Park wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/
---
(Updated April 15, 2015, 4:12 p.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
Bugs: MESOS-2476
https://issues.apache.org/jira/browse/MESOS-2476
Repository: mesos
Description
---
`Resource::ReservationInfo` was introduced in
[r32139](https://reviews.apache.org/r/32139). We need to consider it in our
`Resources` class implementation.
### `Resources::flatten`
`flatten` is used as the utility function to cross reservation boundaries
without affecting the given resources. Since the reservation is now specified
by the (`role`, `reservation`) pair, `flatten` needs to consider
`ReservationInfo` as well.
### `Resources::validate`
If `role == *`, then `reservation` field must not be set.
### `Resources` comparators
`operator==`, `addable` and `substractable` need to test for
`ReservationInfo` as well.
Diffs
-
include/mesos/resources.hpp 56affd45e1e71e96ff5778b43271f81b9b9a9e4d
src/common/resources.cpp 2c99b6884d7296099e19e2e3182cbe11b5e1e059
src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751
src/tests/resources_tests.cpp 7e0ad98c3366f647f190363a0e6b576dbfc7d415
Diff: https://reviews.apache.org/r/32140/diff/
Testing
---
make check
Thanks,
Michael Park
Re: Review Request 32139: Added 'Resource::ReservationInfo' protobuf message.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32139/#review80198 --- include/mesos/mesos.proto https://reviews.apache.org/r/32139/#comment129976 While `required` here, `principal` is `optional` in `FrameworkInfo`. Let's update the comment there to hint folks they should provide `principal` of they want their framework to reserve resources. - Alexander Rukletsov On April 15, 2015, 4:13 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32139/ --- (Updated April 15, 2015, 4:13 p.m.) Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu. Bugs: MESOS-2475 https://issues.apache.org/jira/browse/MESOS-2475 Repository: mesos Description --- The beginning of `Resource::ReservationInfo`. This patch only introduces the `principal` field. Diffs - include/mesos/mesos.proto 3a8e8bf303e0576c212951f6028af77e54d93537 Diff: https://reviews.apache.org/r/32139/diff/ Testing --- make check Thanks, Michael Park
Re: Review Request 32850: Moved cram-md5 authenticatee process definition into implementation file.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32850/#review80020 --- src/authentication/cram_md5/authenticatee.hpp https://reviews.apache.org/r/32850/#comment129770 `#include stout/try.hpp`? src/authentication/cram_md5/authenticatee.hpp https://reviews.apache.org/r/32850/#comment129771 `#include process/future.hpp`? src/authentication/cram_md5/authenticatee.cpp https://reviews.apache.org/r/32850/#comment129772 Should stay in the header. src/authentication/cram_md5/authenticatee.cpp https://reviews.apache.org/r/32850/#comment129773 Let's move to newline to avoid jaggeddness. Btw, what does clang-format suggest? src/authentication/cram_md5/authenticatee.cpp https://reviews.apache.org/r/32850/#comment129774 `#include logging/logging.hpp`? src/authentication/cram_md5/authenticatee.cpp https://reviews.apache.org/r/32850/#comment129775 `#include process/dispatch.hpp? - Alexander Rukletsov On April 10, 2015, 2:58 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32850/ --- (Updated April 10, 2015, 2:58 a.m.) Review request for mesos, Adam B, Joris Van Remoortere, and switched to 'mcypark'. Bugs: MESOS-2584 https://issues.apache.org/jira/browse/MESOS-2584 Repository: mesos-incubating Description --- Removing the process from the header is much cleaner and also fixes the linked clang 3.4.2 JIRA. Apart from that moving, no code is changed. Diffs - src/Makefile.am fa609da src/authentication/cram_md5/authenticatee.hpp 55fac68 src/authentication/cram_md5/authenticatee.cpp PRE-CREATION Diff: https://reviews.apache.org/r/32850/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 32850: Moved cram-md5 authenticatee process definition into implementation file.
On April 14, 2015, 6:17 p.m., Joerg Schad wrote: src/authentication/cram_md5/authenticatee.cpp, line 33 https://reviews.apache.org/r/32850/diff/4/?file=927033#file927033line33 Actually according to Google's styleguide this should be the first include, shouldn't it? http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Order_of_Includes I know we don't do that at other places but actually it would prevent a number of Alexs comments (and be consistent with the styleguide)... I think we don't follow google styleguide whe including headers (update the styleguide?), we have our own grouping: by project, by directory, alphabetically. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32850/#review80072 --- On April 14, 2015, 2:20 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32850/ --- (Updated April 14, 2015, 2:20 p.m.) Review request for mesos, Adam B, Joris Van Remoortere, and switched to 'mcypark'. Bugs: MESOS-2584 https://issues.apache.org/jira/browse/MESOS-2584 Repository: mesos-incubating Description --- Removing the process from the header is much cleaner and also fixes the linked clang 3.4.2 JIRA. Apart from that moving, no code is changed. Diffs - src/Makefile.am d15a373 src/authentication/cram_md5/authenticatee.hpp 55fac68 src/authentication/cram_md5/authenticatee.cpp PRE-CREATION Diff: https://reviews.apache.org/r/32850/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 32850: Moved cram-md5 authenticatee process definition into implementation file.
On April 14, 2015, 2:16 p.m., Alexander Rukletsov wrote: src/authentication/cram_md5/authenticatee.cpp, lines 46-47 https://reviews.apache.org/r/32850/diff/3/?file=922370#file922370line46 Let's move to newline to avoid jaggeddness. Btw, what does clang-format suggest? Kapil Arya wrote: According to our style guide, this is okay but we keep seeing issues being raised about this. Should we just update the style guide instead? I would like to have just one way of doing this. I think the one with newline scales better. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32850/#review80020 --- On April 14, 2015, 2:20 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32850/ --- (Updated April 14, 2015, 2:20 p.m.) Review request for mesos, Adam B, Joris Van Remoortere, and switched to 'mcypark'. Bugs: MESOS-2584 https://issues.apache.org/jira/browse/MESOS-2584 Repository: mesos-incubating Description --- Removing the process from the header is much cleaner and also fixes the linked clang 3.4.2 JIRA. Apart from that moving, no code is changed. Diffs - src/Makefile.am d15a373 src/authentication/cram_md5/authenticatee.hpp 55fac68 src/authentication/cram_md5/authenticatee.cpp PRE-CREATION Diff: https://reviews.apache.org/r/32850/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 32536: Updated variable naming style.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32536/
---
(Updated April 14, 2015, 1:08 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Michael Park, Niklas
Nielsen, and Timothy Chen.
Changes
---
added JIRA
Bugs: MESOS-2616
https://issues.apache.org/jira/browse/MESOS-2616
Repository: mesos-incubating
Description
---
Documents the patterns we use to name variables and function arguments in our
codebase.
Leading underscores to avoid ambiguity.
===
We use this pattern extensively in libprocess, stout and mesos, a few examples
below.
* `stout/try.hpp:105`
```
Try(State _state, T* _t = NULL, const std::string _message = )
: state(_state), t(_t), message(_message) {}
```
* `process/http.hpp:480`
```
URL(const std::string _scheme,
const std::string _domain,
const uint16_t _port = 80,
const std::string _path = /,
const hashmapstd::string, std::string _query =
(hashmapstd::string, std::string()),
const Optionstd::string _fragment = None())
: scheme(_scheme),
domain(_domain),
port(_port),
path(_path),
query(_query),
fragment(_fragment) {}
```
* `slave/containerizer/linux_launcher.cpp:56`
```
LinuxLauncher::LinuxLauncher(
const Flags _flags,
int _namespaces,
const string _hierarchy)
: flags(_flags),
namespaces(_namespaces),
hierarchy(_hierarchy) {}
```
Trailing undescores as prime symbols.
=
We use this pattern in the code, though not extensively. I would like to see
more pass-by-value instead of creating copies from a variable passed by const
reference.
* `master.cpp:2942`
```
// Create and add the slave id.
SlaveInfo slaveInfo_ = slaveInfo;
slaveInfo_.mutable_id()-CopyFrom(newSlaveId());
```
* `slave.cpp:4180`
```
ExecutorInfo executorInfo_ = executor-info;
Resources resources = executorInfo_.resources();
resources += taskInfo.resources();
executorInfo_.mutable_resources()-CopyFrom(resources);
```
* `status_update_manager.cpp:474`
```
// Bounded exponential backoff.
Duration duration_ =
std::min(duration * 2, STATUS_UPDATE_RETRY_INTERVAL_MAX);
```
* `containerizer/mesos/containerizer.cpp:109`
```
// Modify the flags to include any changes to isolation.
Flags flags_ = flags;
flags_.isolation = isolation;
```
Passing arguments by value.
===
* `slave.cpp:2480`
```
void Slave::statusUpdate(StatusUpdate update, const UPID pid)
{
...
// Set the source before forwarding the status update.
update.mutable_status()-set_source(
pid == UPID() ? TaskStatus::SOURCE_SLAVE : TaskStatus::SOURCE_EXECUTOR);
...
}
```
* `process/metrics/timer.hpp:103`
```
static void _time(Time start, Timer that)
{
const Time stop = Clock::now();
double value;
process::internal::acquire(that.data-lock);
{
that.data-lastValue = T(stop - start).value();
value = that.data-lastValue.get();
}
process::internal::release(that.data-lock);
that.push(value);
}
```
Diffs
-
docs/mesos-c++-style-guide.md 439fe12
Diff: https://reviews.apache.org/r/32536/diff/
Testing
---
None (not a functional change).
Thanks,
Alexander Rukletsov
Re: Review Request 32967: Cleaned upstyle and comments in modules.
On April 8, 2015, 1:20 p.m., Alexander Rukletsov wrote: Please check my comment to https://reviews.apache.org/r/32608/. Also, please add Kapil Arya karya, myself alex-mesos and a committer to the list of reviewers. Aditi Dixit wrote: Would adding any committer be ok? Or should I email and ask a committer? tillt will shepherd this. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32967/#review79335 --- On April 8, 2015, 8:05 p.m., Aditi Dixit wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32967/ --- (Updated April 8, 2015, 8:05 p.m.) Review request for mesos, Alexander Rukletsov, Ben Mahler, Kapil Arya, and Vinod Kone. Bugs: MESOS-2565 https://issues.apache.org/jira/browse/MESOS-2565 Repository: mesos Description --- Fixed minor style issues in modules and the misleading comments. Diffs - include/mesos/module/authenticatee.hpp 94de988d524d6390a9f59edbc61f1f7cae514efd include/mesos/module/authenticator.hpp 6a83f17b76ab0bf7af8acd3ddb8044f1bdfa78c0 include/mesos/module/hook.hpp a474e2bd118a5451c0574e33e2cbe9d7d7e95fce include/mesos/module/isolator.hpp 452ad318b9d5445860fa49bcdb933925d750f900 src/examples/test_anonymous_module.cpp 859f4e19e0d50794172af4cff708edf171e6d02f src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41 src/examples/test_isolator_module.cpp 2c303f5ff2f17d56a840a5e0108281873c17ce6a src/examples/test_module.hpp 041570ef3af338bc51c3b3218c460cc8b3f4e4de Diff: https://reviews.apache.org/r/32967/diff/ Testing --- make check Thanks, Aditi Dixit
Re: Review Request 32967: Cleaned upstyle and comments in modules.
On April 8, 2015, 1:20 p.m., Alexander Rukletsov wrote: Please check my comment to https://reviews.apache.org/r/32608/. Also, please add Kapil Arya karya, myself alex-mesos and a committer to the list of reviewers. Aditi Dixit wrote: Would adding any committer be ok? Or should I email and ask a committer? Alexander Rukletsov wrote: tillt will shepherd this. As a consequence, let's remove Vinod and Ben Mahler from the list of reviewers. One committer (Till) is enough for this patch. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32967/#review79335 --- On April 8, 2015, 8:05 p.m., Aditi Dixit wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32967/ --- (Updated April 8, 2015, 8:05 p.m.) Review request for mesos, Alexander Rukletsov, Ben Mahler, Kapil Arya, and Vinod Kone. Bugs: MESOS-2565 https://issues.apache.org/jira/browse/MESOS-2565 Repository: mesos Description --- Fixed minor style issues in modules and the misleading comments. Diffs - include/mesos/module/authenticatee.hpp 94de988d524d6390a9f59edbc61f1f7cae514efd include/mesos/module/authenticator.hpp 6a83f17b76ab0bf7af8acd3ddb8044f1bdfa78c0 include/mesos/module/hook.hpp a474e2bd118a5451c0574e33e2cbe9d7d7e95fce include/mesos/module/isolator.hpp 452ad318b9d5445860fa49bcdb933925d750f900 src/examples/test_anonymous_module.cpp 859f4e19e0d50794172af4cff708edf171e6d02f src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41 src/examples/test_isolator_module.cpp 2c303f5ff2f17d56a840a5e0108281873c17ce6a src/examples/test_module.hpp 041570ef3af338bc51c3b3218c460cc8b3f4e4de Diff: https://reviews.apache.org/r/32967/diff/ Testing --- make check Thanks, Aditi Dixit
Re: Review Request 32997: Removed stale 'Code Internals' document.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32997/#review79534 --- Ship it! Ship It! - Alexander Rukletsov On April 9, 2015, 12:30 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32997/ --- (Updated April 9, 2015, 12:30 a.m.) Review request for mesos and Vinod Kone. Repository: mesos Description --- This document was fairly useless. In the future we should probably document our overall patterns (e.g. stout / libprocess). Diffs - docs/home.md 6ab61f85aa7d62e0f19267b886dffb4e0aa826ea docs/mesos-code-internals.md 7e5b8972807408670f36c5664f5b95d52c1a9a51 Diff: https://reviews.apache.org/r/32997/diff/ Testing --- N/A Thanks, Ben Mahler
Re: Review Request 30952: Adding scheduler validations to master
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30952/#review79536 --- src/master/master.cpp https://reviews.apache.org/r/30952/#comment128948 Minor nit: I believe we use trailing underscores in this case. Think prime symbols. - Alexander Rukletsov On April 8, 2015, 9:11 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30952/ --- (Updated April 8, 2015, 9:11 p.m.) Review request for mesos, Jie Yu, Joris Van Remoortere, and Vinod Kone. Bugs: MESOS-2290 https://issues.apache.org/jira/browse/MESOS-2290 Repository: mesos-incubating Description --- There is a scheduler validation (https://github.com/apache/mesos/blob/master/src/scheduler/scheduler.cpp#L275) missing in master. See motivation on MESOS-2288. Diffs - src/master/master.cpp dccd7c6 src/master/validation.cpp 2f2e4ea src/sched/sched.cpp 66fd2b3 src/scheduler/scheduler.cpp 584b042 Diff: https://reviews.apache.org/r/30952/diff/ Testing --- make check and distcheck. Tests will be added with new HTTP API endpoints tests. Thanks, Isabel Jimenez
Re: Review Request 32967: Cleaned upstyle and comments in modules.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32967/#review79505 --- include/mesos/module/authenticatee.hpp https://reviews.apache.org/r/32967/#comment128909 Kill the space between braces. Here and below. Also please correct `include/mesos/module.hpp` as well. src/examples/test_anonymous_module.cpp https://reviews.apache.org/r/32967/#comment128908 Let's keep this comment short as it used to be, just fix the symbol name as discussed in MESOS-2565. Same for other entries. - Alexander Rukletsov On April 8, 2015, 8:05 p.m., Aditi Dixit wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32967/ --- (Updated April 8, 2015, 8:05 p.m.) Review request for mesos, Alexander Rukletsov, Ben Mahler, Kapil Arya, and Vinod Kone. Bugs: MESOS-2565 https://issues.apache.org/jira/browse/MESOS-2565 Repository: mesos Description --- Fixed minor style issues in modules and the misleading comments. Diffs - include/mesos/module/authenticatee.hpp 94de988d524d6390a9f59edbc61f1f7cae514efd include/mesos/module/authenticator.hpp 6a83f17b76ab0bf7af8acd3ddb8044f1bdfa78c0 include/mesos/module/hook.hpp a474e2bd118a5451c0574e33e2cbe9d7d7e95fce include/mesos/module/isolator.hpp 452ad318b9d5445860fa49bcdb933925d750f900 src/examples/test_anonymous_module.cpp 859f4e19e0d50794172af4cff708edf171e6d02f src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41 src/examples/test_isolator_module.cpp 2c303f5ff2f17d56a840a5e0108281873c17ce6a src/examples/test_module.hpp 041570ef3af338bc51c3b3218c460cc8b3f4e4de Diff: https://reviews.apache.org/r/32967/diff/ Testing --- make check Thanks, Aditi Dixit
Re: Review Request 31666: Piped hashmapSlaveID, Resources from master through to allocator.
On March 6, 2015, 10:57 p.m., Alexander Rukletsov wrote: src/master/allocator/allocator.hpp, line 67 https://reviews.apache.org/r/31666/diff/2/?file=883424#file883424line67 In `updateAllocation()` and `recoverResources()` we pass both `SlaveID` and `FrameworkID` as function parameters. How about we change the signature not by introducing a `hashmap` but a `SlaveID`. This will effectively move the loop from `addFramework()` to `Master::addFramework()`. Michael Park wrote: Initially, I simply kept consistency with `addSlave`, which takes `const hashmapFrameworkID, Resources used`. After a bit of thought, I think it makes sense the way it is, because an `addFramework` function should take the `FrameworkID` of the framework as well as the total resources that framework is using. I don't think it makes sense to report the total resources for a single framework over a sequence of `addFramework` calls. We actually perform the following (I think perfectly reasonable) `CHECK` inside `addFramework` as well: ```cpp CHECK(!frameworkSorters[role]-contains(frameworkId.value())); ``` Agreed. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31666/#review75558 --- On March 7, 2015, 10:02 a.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31666/ --- (Updated March 7, 2015, 10:02 a.m.) Review request for mesos, Alexander Rukletsov and Ben Mahler. Bugs: MESOS-2373 https://issues.apache.org/jira/browse/MESOS-2373 Repository: mesos Description --- Piped the master through by `s/sum(framework-usedResources.values())/framework-usedResources/` Lots of `s/Resources()/hashmapSlaveID, Resources()/` in `src/tests/hierarchical_allocator_tests.cpp`. Diffs - src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/master/allocator/mesos/hierarchical.hpp c0b1da75565d9dc7728c5566f01815234163fc47 src/master/master.cpp 68ca19a9ae680e3ae5bd433a9842baf69f2360ec src/tests/hierarchical_allocator_tests.cpp 93753d1c04159a04a733927a487eb69505438e32 src/tests/mesos.hpp 45e35204d1aa876fa0c871acf0f21afcd5ababe8 Diff: https://reviews.apache.org/r/31666/diff/ Testing --- make check Thanks, Michael Park
Re: Review Request 32967: [MESOS-2565] Cleaned upstyle and comments in modules
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32967/#review79335 --- Please check my comment to https://reviews.apache.org/r/32608/. Also, please add Kapil Arya karya, myself alex-mesos and a committer to the list of reviewers. - Alexander Rukletsov On April 8, 2015, 12:44 p.m., Aditi Dixit wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32967/ --- (Updated April 8, 2015, 12:44 p.m.) Review request for mesos. Bugs: MESOS-2565 https://issues.apache.org/jira/browse/MESOS-2565 Repository: mesos Description --- [MESOS-2565] Cleaned upstyle and comments in modules Diffs - include/mesos/module/authenticatee.hpp 94de988d524d6390a9f59edbc61f1f7cae514efd include/mesos/module/authenticator.hpp 6a83f17b76ab0bf7af8acd3ddb8044f1bdfa78c0 include/mesos/module/hook.hpp a474e2bd118a5451c0574e33e2cbe9d7d7e95fce include/mesos/module/isolator.hpp 452ad318b9d5445860fa49bcdb933925d750f900 src/examples/test_anonymous_module.cpp 859f4e19e0d50794172af4cff708edf171e6d02f src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41 src/examples/test_isolator_module.cpp 2c303f5ff2f17d56a840a5e0108281873c17ce6a src/examples/test_module.hpp 041570ef3af338bc51c3b3218c460cc8b3f4e4de Diff: https://reviews.apache.org/r/32967/diff/ Testing --- make check Thanks, Aditi Dixit
Re: Review Request 31667: Piped hashmapSlaveID, Resources from allocator through to sorter.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31667/#review79345 --- Ship it! src/master/allocator/mesos/hierarchical.hpp https://reviews.apache.org/r/31667/#comment128609 Does it need to be inside the loop? src/master/allocator/sorter/drf/sorter.cpp https://reviews.apache.org/r/31667/#comment128611 Is it a TODO or a NOTE? src/master/allocator/sorter/drf/sorter.cpp https://reviews.apache.org/r/31667/#comment128612 `_allocations` is per-client allocation, let's reflect this in the name and get rid of leading underscore. How about `clientAllocations`? - Alexander Rukletsov On March 7, 2015, 10:03 a.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31667/ --- (Updated March 7, 2015, 10:03 a.m.) Review request for mesos, Alexander Rukletsov and Ben Mahler. Bugs: MESOS-2373 https://issues.apache.org/jira/browse/MESOS-2373 Repository: mesos Description --- `Sorter` changes: - Augmented `add`, `remove`, `allocated`, `unallocated`, `update` with `SlaveID`. - `allocation` returns `hashmapSlaveID, Resources`. `DRFSorter` changes: - `allocations` is updated from `hashmapstd::string, Resources` to `hashmapstd::string, hashmapSlaveID, Resources`. - `resources` is updated from `Resources` to `hashmapSlaveID, Resources`. Diffs - src/master/allocator/mesos/hierarchical.hpp c0b1da75565d9dc7728c5566f01815234163fc47 src/master/allocator/sorter/drf/sorter.hpp 4366710d6530b784aa5094813328d0e338239ba0 src/master/allocator/sorter/drf/sorter.cpp 2f69f384b95ff20d3ee429a4570a8cffa74d8e8b src/master/allocator/sorter/sorter.hpp e2efb27b11dbea42dd73f81e5db0d6d2b0a6034b src/tests/sorter_tests.cpp 42442353afe7bd3d1a5b43992f8ae191ac19bdcd Diff: https://reviews.apache.org/r/31667/diff/ Testing --- make check Thanks, Michael Park
Re: Review Request 31665: Updated master::Framework::usedResources and master::Framework::offerResources from Resources to hashmapSlaveID, Resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31665/#review79333 --- Ship it! Minor nits, otherwise looks good to me! src/master/http.cpp https://reviews.apache.org/r/31665/#comment128602 This comment should go down, `Resources::sum()` is problematic, not `operator +` for hashmap, right? src/master/http.cpp https://reviews.apache.org/r/31665/#comment128603 Same here. It's about deprecating the JSON entry, not the intermediate variable. src/master/http.cpp https://reviews.apache.org/r/31665/#comment128604 Not sure what you mean here. Is it worth creating a JIRA ticket? - Alexander Rukletsov On March 7, 2015, 10:02 a.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31665/ --- (Updated March 7, 2015, 10:02 a.m.) Review request for mesos, Alexander Rukletsov and Ben Mahler. Bugs: MESOS-2373 https://issues.apache.org/jira/browse/MESOS-2373 Repository: mesos Description --- `master::Framework` holds 2 member variables of type `Resources`: `usedResources` and `offerResources`. Both of these are aggregates of resources from multiple slaves and therefore are updated to be `hashmapSlaveID, Resources` instead. There are 3 places where these variables get propagated: (1) `allocator-addFramework(framework-id, framework-info, framework-usedResources)` (2) `src/master/http.cpp`: exposes them to `state.json`. (3) `master::Role::resources()`: needs to return `hashmapSlaveID, Resources` instead. For (3) we can simply change the function signature since we only use it once in `http.cpp` and nowhere else. For (1) and (2), we use the `sum(resources.values())` pattern to match the existing API in the other components. Diffs - src/master/http.cpp b8eef69505b147d4c8a0e005dff545b9fc12a220 src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d src/master/master.cpp 68ca19a9ae680e3ae5bd433a9842baf69f2360ec Diff: https://reviews.apache.org/r/31665/diff/ Testing --- make check Thanks, Michael Park
Re: Review Request 32843: Updated KILL to include SlaveID.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32843/#review79373 --- I'm still not sure whether it's necessary to provide `SlaveID` in the kill request. But I'd better comment on MESOS-1127 instead. src/tests/scheduler_tests.cpp https://reviews.apache.org/r/32843/#comment128649 s/ / here and below. - Alexander Rukletsov On April 3, 2015, 11:40 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32843/ --- (Updated April 3, 2015, 11:40 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- Having SlaveID will help us do better reconciliation when the task is not found. Also, updated master to handle Call::Kill. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5 src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32843/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32501: Removed REQUEST call from scheduler.proto.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32501/#review79354 --- Ship it! - Alexander Rukletsov On March 31, 2015, 12:06 a.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32501/ --- (Updated March 31, 2015, 12:06 a.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- Removed REQUEST call because it is unused. Diffs - include/mesos/mesos.proto 3c592d5ab3092ecbeddfaff95e0c1addc3ac58f8 include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5 src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a Diff: https://reviews.apache.org/r/32501/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32502: Updated RECONCILE call to always specifiy slave id.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32502/#review79358 --- include/mesos/mesos.proto https://reviews.apache.org/r/32502/#comment128618 For consistency, mind adding a comment in `Slave::statusUpdate()` that we should ensure `SlaveID` is set? Or even log warning in case it's not, so we start preparing framework authors for the upcoming change now. Also, maybe we need a JIRA for this. include/mesos/scheduler/scheduler.proto https://reviews.apache.org/r/32502/#comment128620 I see your point in making `SlaveID` required in `TaskStatus` and it makes sense to me. But does this mean it should be required here as well? My understanding is that if the master knows `TaskID` it can always deduce `SlaveID`; if the task is unknown to the master, then the task is lost in all cases except when there are transitionary slaves. Having `SlaveID` in reconciliation query *just* speeds up reconciliation for unknown tasks, therefore making it required feels too much for me. Maybe Ben Mahler can provide more background and validate my thoughts. If you opt for making it required, I would then suggest to add checks in reconciliation algorithm, so that `SlaveID` of known (to the master) tasks matches `SlaveID` from the query. Similar to what you do in a subsequent patch for `killTask`. I'll also leave a comment on MESOS-1127. src/master/master.cpp https://reviews.apache.org/r/32502/#comment128621 I would suggest we put the implementation after `Master::accept()` call for consistency. src/master/master.cpp https://reviews.apache.org/r/32502/#comment128633 Do you want to update metrics here? `++metrics-messages_reconcile_tasks;` - Alexander Rukletsov On April 3, 2015, 11:36 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32502/ --- (Updated April 3, 2015, 11:36 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- Having a slave id will help us do better reconciliation. So added it to the 'Reconcile' call. Also updated the master to receive the Reconcile call directly. Diffs - include/mesos/mesos.proto 3a8e8bf303e0576c212951f6028af77e54d93537 include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5 src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32502/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32509: Documented the scheduler Event/Call protobufs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32509/#review79383 --- LGTM, not a native speaker though, hence no ShipIt. include/mesos/scheduler/scheduler.proto https://reviews.apache.org/r/32509/#comment128674 Not yours, but could you fix the length please? - Alexander Rukletsov On April 3, 2015, 11:55 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32509/ --- (Updated April 3, 2015, 11:55 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- See summary. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 Diff: https://reviews.apache.org/r/32509/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32504: Removed MasterInfo from REGISTER and REREGISTER scheduler calls.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32504/#review79368 --- Ship it! - Alexander Rukletsov On March 31, 2015, 12:08 a.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32504/ --- (Updated March 31, 2015, 12:08 a.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- Removed MasterInfo from REGISTER and REREGISTER scheduler calls because they don't provide much value in a HTTP API world because the scheduler knows precisely who it is connecting to. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/examples/low_level_scheduler_libprocess.cpp 63d34eefb60d13fe2b82905c1cec9b762340e997 src/examples/low_level_scheduler_pthread.cpp 6d1f938660c02db75bfcbf7c8de0d941cff1920d src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a Diff: https://reviews.apache.org/r/32504/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32845: Renamed UNREGISTER call to UNSUBSCRIBE.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32845/#review79381 --- src/master/master.cpp https://reviews.apache.org/r/32845/#comment128670 Shall we check `framework-pid == from` as we do in `Master::unregisterFramework()`? Also metrcis update `++metrics-messages_unregister_framework;` should be migrated to `removeFramework()`. src/tests/scheduler_tests.cpp https://reviews.apache.org/r/32845/#comment128672 s/ / here and below. - Alexander Rukletsov On April 3, 2015, 11:50 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32845/ --- (Updated April 3, 2015, 11:50 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- Renamed UNREGISTER call to UNSUBSCRIBE for consistency. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/examples/low_level_scheduler_libprocess.cpp 63d34eefb60d13fe2b82905c1cec9b762340e997 src/examples/low_level_scheduler_pthread.cpp 6d1f938660c02db75bfcbf7c8de0d941cff1920d src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32845/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32505: Added SHUTDOWN scheduler call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32505/#review79369 --- src/master/master.cpp https://reviews.apache.org/r/32505/#comment128640 How about moving this after `Master::reconcile()`? src/tests/scheduler_tests.cpp https://reviews.apache.org/r/32505/#comment128645 s/ / here and below. src/tests/scheduler_tests.cpp https://reviews.apache.org/r/32505/#comment128648 Mind leaving a comment why we expect failure here? Maybe it makes sense to write a short comment to the test as well. - Alexander Rukletsov On April 3, 2015, 11:38 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32505/ --- (Updated April 3, 2015, 11:38 p.m.) Review request for mesos, Benjamin Hindman and Ben Mahler. Bugs: MESOS-1127 and MESOS-330 https://issues.apache.org/jira/browse/MESOS-1127 https://issues.apache.org/jira/browse/MESOS-330 Repository: mesos Description --- This is a new call added to explicitly shutdown an executor. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5 src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8 src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae src/slave/slave.cpp 0f70ebafb0f5b16c560decc66e22a43a52732d09 src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32505/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32844: Added SUBSCRIBE call and SUBSCRIBED event.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32844/#review79378 --- Ship it! Looks good to me, mind update the design doc? include/mesos/scheduler/scheduler.proto https://reviews.apache.org/r/32844/#comment128658 Does it make sense to pick up a new field id to avoid collisions? Same below for `Subscribed` field and `Call` message. I see you do a batch update in the last patch in the sequence, just want to confirm we do want to break it. src/master/master.cpp https://reviews.apache.org/r/32844/#comment128659 Could you please re-format this comment? src/scheduler/scheduler.cpp https://reviews.apache.org/r/32844/#comment128663 s/should not/may not/ - Alexander Rukletsov On April 3, 2015, 11:46 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32844/ --- (Updated April 3, 2015, 11:46 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- Instead of REGISTER and REREGISTER we now just have SUBSCRIBE. Similarly, instead of REGISTERED and REREGISTERED there is only SUBSCRIBED. This will simplify a scheduler's registration semantics. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/examples/low_level_scheduler_libprocess.cpp 63d34eefb60d13fe2b82905c1cec9b762340e997 src/examples/low_level_scheduler_pthread.cpp 6d1f938660c02db75bfcbf7c8de0d941cff1920d src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32844/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32506: Added output stream operators for scheduler Calls and Events.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32506/#review79376 --- Ship it! include/mesos/type_utils.hpp https://reviews.apache.org/r/32506/#comment128651 Shall we include ostream as well? include/mesos/type_utils.hpp https://reviews.apache.org/r/32506/#comment128655 `scheduler::Call_Type_Name()` for clarity? include/mesos/type_utils.hpp https://reviews.apache.org/r/32506/#comment128656 `scheduler::Event_Type_Name()` for clarity? - Alexander Rukletsov On April 3, 2015, 11:42 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32506/ --- (Updated April 3, 2015, 11:42 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- For readable output in logs. Diffs - include/mesos/type_utils.hpp cdf5864389a72002b538c263d70bcade2bdffa45 src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b Diff: https://reviews.apache.org/r/32506/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32500: Removed LAUNCH call from scheduler.proto.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32500/#review79351 --- include/mesos/scheduler/scheduler.proto https://reviews.apache.org/r/32500/#comment128617 FYI: this looks too long for a comment, clang-format shouldn't allow this. I think this is OK though, and helps readability. include/mesos/scheduler/scheduler.proto https://reviews.apache.org/r/32500/#comment128614 Do we specify an order in which offer operations are performed? I don't think you touched this in the design doc. Mind leaving a comment whether we provide any guarantees here? src/scheduler/scheduler.cpp https://reviews.apache.org/r/32500/#comment128615 IIUC, since this is not migrated to `case Call::ACCEPT`, tasks are launched via `Master::accept()` and not via `Master::launchTasks()`. If this is the case, then it looks like we do not update some metrics, namely: `++metrics-messages_launch_tasks;` `++metrics-messages_decline_offers;` Do you plan to adress this later on or am I missing something? - Alexander Rukletsov On March 30, 2015, 11:59 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32500/ --- (Updated March 30, 2015, 11:59 p.m.) Review request for mesos, Ben Mahler and Jie Yu. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- Removed LAUNCH call in favor of ACCEPT. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/examples/low_level_scheduler_libprocess.cpp 63d34eefb60d13fe2b82905c1cec9b762340e997 src/examples/low_level_scheduler_pthread.cpp 6d1f938660c02db75bfcbf7c8de0d941cff1920d src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32500/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 31776: Moved allocator to public headers.
On April 2, 2015, 6:23 p.m., Vinod Kone wrote: include/mesos/master/allocator.proto, lines 19-23 https://reviews.apache.org/r/31776/diff/6/?file=912077#file912077line19 Since allocator is within the same Unix process as Master, what is the compatibility issue here? The comment is related to the protobuf namespace in which `RoleInfo` lives. Not sure I'm getting what you mean by mentioning that allocator and Master are in the same Unix process. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31776/#review78690 --- On April 1, 2015, 2:25 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31776/ --- (Updated April 1, 2015, 2:25 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- This is required for out-of-tree allocator modules. RoleInfo protobuf message has to be extracted into its own public proto file. Diffs - include/mesos/master/allocator.proto PRE-CREATION src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8 src/local/local.hpp 0aa50ef4f44c347eca4b3a409f2a8d03ba321d82 src/local/local.cpp 19083368212b24ce1afef3a5f91d48766d1cd55e src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/master/main.cpp 7cce3a0bb808a1cb7bac9acab31eb1c67a15ea9f src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5 src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8 src/tests/cluster.hpp a56b6541adcdebc5866571bbdbb6828df97b34ec src/tests/fault_tolerance_tests.cpp a637c32f004638a110390b22cf5b626e904097cf src/tests/hierarchical_allocator_tests.cpp 8861bf398e4bb17b0f74eab4f4af26202447ccef src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 src/tests/master_authorization_tests.cpp ac79303645cc1af337b1dca8db244113d0ba6fce src/tests/master_slave_reconciliation_tests.cpp e60f601202fcdbb4cafac190e9b09ca6ce925260 src/tests/master_tests.cpp 2bfd0f8a20169649cff03509556b4cfa965a9837 src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 src/tests/mesos.cpp 02cbb4b8cf1206d0f32d160addc91d7e0f1ab28b src/tests/rate_limiting_tests.cpp d5c00b8db30fc529bc984417a632cf99eb50eb28 Diff: https://reviews.apache.org/r/31776/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31263: Refactored TestAllocator and allocator text fixture.
On April 2, 2015, 7:58 p.m., Vinod Kone wrote:
src/tests/master_allocator_tests.cpp, lines 96-104
https://reviews.apache.org/r/31263/diff/7/?file=912106#file912106line96
While the TearDown() avoids flakiness by ensuring that an allocator
process doesn't exist after a test, it doesn't address cases where a master
is restarted with a new allocator in the same test (e.g.,
FrameworkReregistersFirst and SlaveReregistersFirst) causing both old and
new allocators to coexist at the same time and talking to the same master.
I suggest to have methods on the test fixture that creates and deletes
allocator explicitly (can be used by tests that restart master). The
TearDown() will still delete any active allocator. For safety, ensure that
only one allocator is active at any given time.
Does that make sense?
```
template typename T
class MasterAllocatorTest : public MesosTest
{
protected:
MasterAllocatorTest() : allocator(Null) {}
TryTestAllocator create()
{
// We don't support multiple allocators because...
if (allocator != NULL) {
return Error(Multiple active allocators are not supported);
}
allocator = new TestAllocator(process::OwnedAllocator(new T));
return *allocator;
}
void destroy()
{
delete allocator;
allocator = NULL;
}
virtual void TearDown()
{
destroy();
MeosTest::TearDown();
}
private:
TestAllocator* allocator;
};
```
We used to have a lifecycle method but decided to remove it:
be6246a11276074dfb60a892a890b80cb7cd37cd
RR: https://reviews.apache.org/r/29927/
The two tests you point to are currently the only ones that create an
additional allocator. They both create a new leader instance and appoint the
slave instance to the new leader, and there is no possibility AFAIK to swap
allocators in a leader instance. So yes, there are two active allocators in
these tests, but they belong to different leaders.
Current design ensures there is only one active allocator in the fixture, but
allows to create more if needed (what those two tests do). The design you
propose doesn't prevent us from creating one more allocator in the test body
either.
Thoughts?
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31263/#review78710
---
On April 1, 2015, 2:27 p.m., Alexander Rukletsov wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31263/
---
(Updated April 1, 2015, 2:27 p.m.)
Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen.
Bugs: MESOS-2160
https://issues.apache.org/jira/browse/MESOS-2160
Repository: mesos
Description
---
TestAllocator stores a pointer to a real allocator and takes over its
ownership. MasterAllocatorTest fixture stores the test allocator in turn.
Diffs
-
src/tests/containerizer.cpp 26b87ac6b16dfeaf84888e80296ef540697bd775
src/tests/master_allocator_tests.cpp
03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302
src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751
src/tests/resource_offers_tests.cpp
882a9ff4d09aace486182828bf43b643b0d0c519
src/tests/slave_recovery_tests.cpp 87f4a6aab27d142fa8eb7a6571f684a6ce59687e
Diff: https://reviews.apache.org/r/31263/diff/
Testing
---
make check (Mac OS 10.9.5, CentOS 7.0)
Thanks,
Alexander Rukletsov
Re: Review Request 31265: Provided a factory for allocator in tests.
On April 2, 2015, 7:59 p.m., Vinod Kone wrote: src/master/allocator/mesos/allocator.hpp, line 47 https://reviews.apache.org/r/31265/diff/6/?file=912110#file912110line47 Do you still want the constructor public? I think there is no reason to hide the c-tor. I can imagine somebody creating an instance on the stack and do not see why we should forbid it. Same approach is used for example for `MesosContainerizer`. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31265/#review78711 --- On April 1, 2015, 2:28 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31265/ --- (Updated April 1, 2015, 2:28 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The factory creates allocator instances in a way identical to how instances from modules are created. It allows us to use same typed tests for built-in and modularized allocators. Diffs - src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 Diff: https://reviews.apache.org/r/31265/diff/ Testing --- make check (Mac OS 10.9.5, Ubuntu 14.04) Thanks, Alexander Rukletsov
Re: Review Request 31776: Moved allocator to public headers.
On April 2, 2015, 6:23 p.m., Vinod Kone wrote: src/Makefile.am, line 711 https://reviews.apache.org/r/31776/diff/6/?file=912078#file912078line711 not yours, but can you kill the trailing white space here? Sure. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31776/#review78690 --- On April 1, 2015, 2:25 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31776/ --- (Updated April 1, 2015, 2:25 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- This is required for out-of-tree allocator modules. RoleInfo protobuf message has to be extracted into its own public proto file. Diffs - include/mesos/master/allocator.proto PRE-CREATION src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8 src/local/local.hpp 0aa50ef4f44c347eca4b3a409f2a8d03ba321d82 src/local/local.cpp 19083368212b24ce1afef3a5f91d48766d1cd55e src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/master/main.cpp 7cce3a0bb808a1cb7bac9acab31eb1c67a15ea9f src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5 src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8 src/tests/cluster.hpp a56b6541adcdebc5866571bbdbb6828df97b34ec src/tests/fault_tolerance_tests.cpp a637c32f004638a110390b22cf5b626e904097cf src/tests/hierarchical_allocator_tests.cpp 8861bf398e4bb17b0f74eab4f4af26202447ccef src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 src/tests/master_authorization_tests.cpp ac79303645cc1af337b1dca8db244113d0ba6fce src/tests/master_slave_reconciliation_tests.cpp e60f601202fcdbb4cafac190e9b09ca6ce925260 src/tests/master_tests.cpp 2bfd0f8a20169649cff03509556b4cfa965a9837 src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 src/tests/mesos.cpp 02cbb4b8cf1206d0f32d160addc91d7e0f1ab28b src/tests/rate_limiting_tests.cpp d5c00b8db30fc529bc984417a632cf99eb50eb28 Diff: https://reviews.apache.org/r/31776/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31267: Added a test allocator module.
On April 2, 2015, 6:33 p.m., Vinod Kone wrote: src/Makefile.am, line 1351 https://reviews.apache.org/r/31267/diff/6/?file=912100#file912100line1351 s/drf// to be consistent (e.g., we didn't name libtestauthentication.la as libcrammd5authentication.la) Will do. On April 2, 2015, 6:33 p.m., Vinod Kone wrote: src/tests/module.cpp, line 151 https://reviews.apache.org/r/31267/diff/6/?file=912103#file912103line151 s/drf// Will do. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/#review78694 --- On April 1, 2015, 2:26 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/ --- (Updated April 1, 2015, 2:26 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The test allocator module is a built-in HierarchicalDRFAllocator put in a module. NOTE: Here we add harness code to load the module, tests will be wired up later in the patch stack. Diffs - src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8 src/examples/test_allocator_module.cpp PRE-CREATION src/tests/module.hpp 65e567f21744d9a2b07b8680d45bcd1ea47f9508 src/tests/module.cpp b81144f56e94034feecf3a6a4992af078cf60a81 Diff: https://reviews.apache.org/r/31267/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31776: Moved allocator to public headers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31776/ --- (Updated April 7, 2015, 12:46 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Fixed a space. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- This is required for out-of-tree allocator modules. RoleInfo protobuf message has to be extracted into its own public proto file. Diffs (updated) - include/mesos/master/allocator.proto PRE-CREATION src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8 src/local/local.hpp 0aa50ef4f44c347eca4b3a409f2a8d03ba321d82 src/local/local.cpp 19083368212b24ce1afef3a5f91d48766d1cd55e src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/master/main.cpp 7cce3a0bb808a1cb7bac9acab31eb1c67a15ea9f src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5 src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8 src/tests/cluster.hpp a56b6541adcdebc5866571bbdbb6828df97b34ec src/tests/fault_tolerance_tests.cpp a637c32f004638a110390b22cf5b626e904097cf src/tests/hierarchical_allocator_tests.cpp 8861bf398e4bb17b0f74eab4f4af26202447ccef src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 src/tests/master_authorization_tests.cpp ac79303645cc1af337b1dca8db244113d0ba6fce src/tests/master_slave_reconciliation_tests.cpp e60f601202fcdbb4cafac190e9b09ca6ce925260 src/tests/master_tests.cpp 2bfd0f8a20169649cff03509556b4cfa965a9837 src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 src/tests/mesos.cpp 02cbb4b8cf1206d0f32d160addc91d7e0f1ab28b src/tests/rate_limiting_tests.cpp d5c00b8db30fc529bc984417a632cf99eb50eb28 Diff: https://reviews.apache.org/r/31776/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31775: Removed Master::Flags dependency from Allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31775/ --- (Updated April 7, 2015, 12:46 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- On the way to moving allocator to public includes we need to remove the dependency to internal Master::Flags type. Diffs (updated) - src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/master/allocator/mesos/hierarchical.hpp 9f9a631ee52a3ab194e678f9be139e8dabc7f3db src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/tests/hierarchical_allocator_tests.cpp 8861bf398e4bb17b0f74eab4f4af26202447ccef src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 Diff: https://reviews.apache.org/r/31775/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31266: Added support for allocator modules.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31266/ --- (Updated April 7, 2015, 12:47 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- See summary. Diffs (updated) - include/mesos/module/allocator.hpp PRE-CREATION src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8 src/module/manager.cpp 82a38f06e57d034650a6ac32fd73527b38cc97b8 Diff: https://reviews.apache.org/r/31266/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31267: Added a test allocator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/ --- (Updated April 7, 2015, 12:47 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Addressed Vinod's comments. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The test allocator module is a built-in HierarchicalDRFAllocator put in a module. NOTE: Here we add harness code to load the module, tests will be wired up later in the patch stack. Diffs (updated) - src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8 src/examples/test_allocator_module.cpp PRE-CREATION src/tests/module.hpp 65e567f21744d9a2b07b8680d45bcd1ea47f9508 src/tests/module.cpp b81144f56e94034feecf3a6a4992af078cf60a81 Diff: https://reviews.apache.org/r/31267/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31262: Moved allocator actions before TestAllocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31262/ --- (Updated April 7, 2015, 12:48 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- Once TestAllocator is not a template, the allocator actions should be defined before they are used in TestAllocator. Diffs (updated) - src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 Diff: https://reviews.apache.org/r/31262/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31265: Provided a factory for allocator in tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31265/ --- (Updated April 7, 2015, 12:49 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The factory creates allocator instances in a way identical to how instances from modules are created. It allows us to use same typed tests for built-in and modularized allocators. Diffs (updated) - src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 Diff: https://reviews.apache.org/r/31265/diff/ Testing --- make check (Mac OS 10.9.5, Ubuntu 14.04) Thanks, Alexander Rukletsov
Re: Review Request 31268: Wired up test allocator to allocator tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31268/ --- (Updated April 7, 2015, 12:49 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- Same typed tests are used for built-in and modularized allocators. Diffs (updated) - src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 Diff: https://reviews.apache.org/r/31268/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.
On March 26, 2015, 4:32 p.m., Alexander Rukletsov wrote: 3rdparty/libprocess/src/process.cpp, line 6 https://reviews.apache.org/r/30032/diff/4/?file=834184#file834184line6 One thing captures my attention is how we include C headers. AFAIK, the standard requires to include them like ``` #include ctime #include climits #include cstdio ``` and so on. Could you please create a cleanup newbie JIRA for this? Alexander Rojas wrote: Not very sure about that, what it does by using the c… versions of the headers is to put its functions in the std namespace. Check the C++11 Standard annex *D.6 C standard library headers*: 2 Every C header, each of which has a name of the form name.h, behaves as if each name placed in the standard library namespace by the corresponding cname header is placed within the global namespace scope. It is unspecified whether these names are first declared or defined within namespace scope (3.3.6) of the namespace std and are then injected into the global namespace scope by explicit using-declarations (7.3.3). 3 Example: The header cstdlib assuredly provides its declarations and definitions within the namespace std. It may also provide these names within the global namespace. The header stdlib.h assuredly provides the same declarations and definitions within the global namespace, much as in the C Standard. It may also provide these names within the namespace std. —end example. As it reads, using functions from the `cname` versions of the `name.h` headers, may or may not (stdlib implementation specific) require to add the namespace std too all functions and structs provided by the header. Till Toenshoff wrote: Seems we got three options here; - start using the C++ wrappers and allowed for a smooth transition (file by file as needed/touched) - - not sure what the implications here are from an optimizing (linker) point of view - global cleanup and `std` namespace added everywhere - stick with the C variants May be worth a dev-list discussion? C++11 Standard 17.6.1.2 p4, p8, and reference 177 hint at a preferred form for C++ programs. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/#review77914 --- On March 26, 2015, 9:53 a.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/ --- (Updated March 26, 2015, 9:53 a.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, Michael Park, and Till Toenshoff. Bugs: mesos-708 https://issues.apache.org/jira/browse/mesos-708 Repository: mesos Description --- When serving a static file, libprocess returns the header `Last-Modified` which is used by browsers to control Cache. When a http request arrives containing the header `If-Modified-Since`, a response `304 Not Modified` is returned if the date in the request and the modification time (as returned by doing `stat` in the file) coincide. Unit tests added. Diffs - 3rdparty/libprocess/include/process/http.hpp 9cf05ac 3rdparty/libprocess/src/process.cpp 67b6b3b 3rdparty/libprocess/src/tests/process_tests.cpp 3bbfe0a Diff: https://reviews.apache.org/r/30032/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 32543: Documented problem and solution with slave recovery and systemd settings.
On March 27, 2015, 9:17 a.m., Adam B wrote: docs/slave-recovery.md, line 71 https://reviews.apache.org/r/32543/diff/2/?file=907123#file907123line71 (If the slave does not come back, each executorDriver shuts itself down after $MESOS_RECOVERY_TIMEOUT.) Important question: If an executor is killed, does this systemd mode affect whether its tasks would get killed? Alexander Rukletsov wrote: Adam, could you please explain what use case do you have in mind and how it is related to slave recovery? Adam B wrote: It's not related to slave recovery necessarily, but to how this KillMode impacts other processes like a custom executor. Some frameworks (like HDFS) have a custom executor that launches task(s) as a separate process/subprocess. If the executor is killed (kill -9, or shutdown by the framework/admin), will this change in KillMode affect whether the executors task subprocesses also get killed? I'm mostly worried about this KillMode change suddenly leaving stranded task processes if/when executors are killed. Alexander Rukletsov wrote: I thought that's exactly why we have containerizers: clean-up all stranded processes. Adam B wrote: Fair enough, when the slave is running. But what if the executor is killed while the slave (thus also the containerizer) is shutdown/recovering? I'm not claiming there's anything necessarily wrong with using this KillMode. I just ask the question to make sure we don't recommend a setting that may fix one issue but cause others. I see your point. I would be surprised if this setting will cause the issue, but let's check: better safe than sorry. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32543/#review78025 --- On March 27, 2015, 2:09 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32543/ --- (Updated March 27, 2015, 2:09 p.m.) Review request for mesos, Alexander Rukletsov and Brenden Matthews. Bugs: Mesos-2555 https://issues.apache.org/jira/browse/Mesos-2555 Repository: mesos Description --- Documented the problem and solution encountered in MESOS-2419. Diffs - docs/slave-recovery.md 4bb4a71c6945bd70121743a1e9209a26906773c1 docs/upgrades.md 2a15694607c079ad95ef6cf7f1490872ab9a5976 Diff: https://reviews.apache.org/r/32543/diff/ Testing --- markdown check Thanks, Joerg Schad
Re: Review Request 32543: Documented problem and solution with slave recovery and systemd settings.
On March 27, 2015, 9:17 a.m., Adam B wrote: docs/slave-recovery.md, line 71 https://reviews.apache.org/r/32543/diff/2/?file=907123#file907123line71 (If the slave does not come back, each executorDriver shuts itself down after $MESOS_RECOVERY_TIMEOUT.) Important question: If an executor is killed, does this systemd mode affect whether its tasks would get killed? Alexander Rukletsov wrote: Adam, could you please explain what use case do you have in mind and how it is related to slave recovery? Adam B wrote: It's not related to slave recovery necessarily, but to how this KillMode impacts other processes like a custom executor. Some frameworks (like HDFS) have a custom executor that launches task(s) as a separate process/subprocess. If the executor is killed (kill -9, or shutdown by the framework/admin), will this change in KillMode affect whether the executors task subprocesses also get killed? I'm mostly worried about this KillMode change suddenly leaving stranded task processes if/when executors are killed. I thought that's exactly why we have containerizers: clean-up all stranded processes. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32543/#review78025 --- On March 27, 2015, 2:09 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32543/ --- (Updated March 27, 2015, 2:09 p.m.) Review request for mesos, Alexander Rukletsov and Brenden Matthews. Bugs: Mesos-2555 https://issues.apache.org/jira/browse/Mesos-2555 Repository: mesos Description --- Documented the problem and solution encountered in MESOS-2419. Diffs - docs/slave-recovery.md 4bb4a71c6945bd70121743a1e9209a26906773c1 docs/upgrades.md 2a15694607c079ad95ef6cf7f1490872ab9a5976 Diff: https://reviews.apache.org/r/32543/diff/ Testing --- markdown check Thanks, Joerg Schad
Re: Review Request 31267: Added a test allocator module.
On March 27, 2015, 4:36 p.m., Alexander Rojas wrote: src/tests/module.cpp, line 137 https://reviews.apache.org/r/31267/diff/4-5/?file=903021#file903021line137 You may need a `CHECK(modules != NULL)` or a log and return in the NULL case? If you follow the invokation path, you'll see that modules is a stack variable (see `initModules()`) and can't be null. I agree this is not clear at the place where we use a pointer to it and may change in the future, but this is more a general question about the design decision that should go to @tillt. Pass by pointer was introduced in Commit: 7ee3b7b672a4d8fee4fe4eb5f0aa2e7e3bf6b049 Review: https://reviews.apache.org/r/31253 Author: Till Toenshoff toensh...@me.com - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/#review78070 --- On March 27, 2015, 4:26 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/ --- (Updated March 27, 2015, 4:26 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The test allocator module is a built-in HierarchicalDRFAllocator put in a module. NOTE: Here we add harness code to load the module, tests will be wired up later in the patch stack. Diffs - src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/examples/test_allocator_module.cpp PRE-CREATION src/tests/module.hpp 65e567f21744d9a2b07b8680d45bcd1ea47f9508 src/tests/module.cpp b81144f56e94034feecf3a6a4992af078cf60a81 Diff: https://reviews.apache.org/r/31267/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31268: Wired up test allocator to allocator tests.
On March 24, 2015, 2:49 p.m., Michael Park wrote: src/tests/master_allocator_tests.cpp, lines 123-126 https://reviews.apache.org/r/31268/diff/4/?file=903030#file903030line123 Could we indent this similar to this example from `src/tests/isolator_tests.cpp`: ``` typedef ::testing::Types CgroupsMemIsolatorProcess, CgroupsCpushareIsolatorProcess, CgroupsPerfEventIsolatorProcess CgroupsIsolatorTypes; ``` ``` typedef ::testing::Types HierarchicalDRFAllocator, tests::ModuleAllocator, TestDRFAllocator AllocatorTypes; ``` Alexander Rukletsov wrote: I don't know what is consistent here, `src/tests/cram_md5_authentication_tests.cpp` use the indentation I use. Also, clang-format gives something that is more similar to the way I propose. Michael Park wrote: Yeah I did see the one from `src/tests/cram_md5_authentication_tests.cpp`. I think that one is long enough that we can't do too much better. I would prefer a style that visually separates the list of types with the resulting type. `clang-format` seems to format it as: ``` typedef ::testing::TypesHierarchicalDRFAllocator, tests::ModuleAllocator, TestDRFAllocator AllocatorTypes; ``` which also visually separates the list of types and the resulting type so I'm ok with that also. Just explaining exactly what I was pushing for. Having said that, do whatever you feel is right here :) Ok, then let's go for the tool! - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31268/#review77572 --- On March 27, 2015, 4:27 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31268/ --- (Updated March 27, 2015, 4:27 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- Same typed tests are used for built-in and modularized allocators. Diffs - src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 Diff: https://reviews.apache.org/r/31268/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31775: Removed Master::Flags dependency from Allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31775/ --- (Updated April 1, 2015, 2:24 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Addressed Niklas' comment. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- On the way to moving allocator to public includes we need to remove the dependency to internal Master::Flags type. Diffs (updated) - src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/master/allocator/mesos/hierarchical.hpp 9f9a631ee52a3ab194e678f9be139e8dabc7f3db src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/tests/hierarchical_allocator_tests.cpp 8861bf398e4bb17b0f74eab4f4af26202447ccef src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 Diff: https://reviews.apache.org/r/31775/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31266: Added support for allocator modules.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31266/ --- (Updated April 1, 2015, 2:26 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- See summary. Diffs (updated) - include/mesos/module/allocator.hpp PRE-CREATION src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8 src/module/manager.cpp 82a38f06e57d034650a6ac32fd73527b38cc97b8 Diff: https://reviews.apache.org/r/31266/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31776: Moved allocator to public headers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31776/ --- (Updated April 1, 2015, 2:25 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- This is required for out-of-tree allocator modules. RoleInfo protobuf message has to be extracted into its own public proto file. Diffs (updated) - include/mesos/master/allocator.proto PRE-CREATION src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8 src/local/local.hpp 0aa50ef4f44c347eca4b3a409f2a8d03ba321d82 src/local/local.cpp 19083368212b24ce1afef3a5f91d48766d1cd55e src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/master/main.cpp 7cce3a0bb808a1cb7bac9acab31eb1c67a15ea9f src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5 src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8 src/tests/cluster.hpp a56b6541adcdebc5866571bbdbb6828df97b34ec src/tests/fault_tolerance_tests.cpp a637c32f004638a110390b22cf5b626e904097cf src/tests/hierarchical_allocator_tests.cpp 8861bf398e4bb17b0f74eab4f4af26202447ccef src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 src/tests/master_authorization_tests.cpp ac79303645cc1af337b1dca8db244113d0ba6fce src/tests/master_slave_reconciliation_tests.cpp e60f601202fcdbb4cafac190e9b09ca6ce925260 src/tests/master_tests.cpp 2bfd0f8a20169649cff03509556b4cfa965a9837 src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 src/tests/mesos.cpp 02cbb4b8cf1206d0f32d160addc91d7e0f1ab28b src/tests/rate_limiting_tests.cpp d5c00b8db30fc529bc984417a632cf99eb50eb28 Diff: https://reviews.apache.org/r/31776/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31267: Added a test allocator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/ --- (Updated April 1, 2015, 2:26 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Addressed Alexander's comment. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The test allocator module is a built-in HierarchicalDRFAllocator put in a module. NOTE: Here we add harness code to load the module, tests will be wired up later in the patch stack. Diffs (updated) - src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8 src/examples/test_allocator_module.cpp PRE-CREATION src/tests/module.hpp 65e567f21744d9a2b07b8680d45bcd1ea47f9508 src/tests/module.cpp b81144f56e94034feecf3a6a4992af078cf60a81 Diff: https://reviews.apache.org/r/31267/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31265: Provided a factory for allocator in tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31265/ --- (Updated April 1, 2015, 2:28 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The factory creates allocator instances in a way identical to how instances from modules are created. It allows us to use same typed tests for built-in and modularized allocators. Diffs (updated) - src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 Diff: https://reviews.apache.org/r/31265/diff/ Testing --- make check (Mac OS 10.9.5, Ubuntu 14.04) Thanks, Alexander Rukletsov
Re: Review Request 31263: Refactored TestAllocator and allocator text fixture.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31263/ --- (Updated April 1, 2015, 2:27 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- TestAllocator stores a pointer to a real allocator and takes over its ownership. MasterAllocatorTest fixture stores the test allocator in turn. Diffs (updated) - src/tests/containerizer.cpp 26b87ac6b16dfeaf84888e80296ef540697bd775 src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 src/tests/resource_offers_tests.cpp 882a9ff4d09aace486182828bf43b643b0d0c519 src/tests/slave_recovery_tests.cpp 87f4a6aab27d142fa8eb7a6571f684a6ce59687e Diff: https://reviews.apache.org/r/31263/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31268: Wired up test allocator to allocator tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31268/ --- (Updated April 1, 2015, 2:29 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Addressed MPark's comment. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- Same typed tests are used for built-in and modularized allocators. Diffs (updated) - src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 Diff: https://reviews.apache.org/r/31268/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31776: Moved allocator to public headers.
On March 23, 2015, 10:27 p.m., Michael Park wrote: include/mesos/master/allocator.proto, lines 20-21 https://reviews.apache.org/r/31776/diff/4/?file=902995#file902995line20 `s/breaking the backward compatibility/breaking backwards compatibility/` Also this is referring to the fact that we can't communicate with the old master if we pull this message out of `internal`, correct? Correct, namespace is part of the type. On March 23, 2015, 10:27 p.m., Michael Park wrote: src/Makefile.am, line 709 https://reviews.apache.org/r/31776/diff/4/?file=902996#file902996line709 Is this a trailing whitespace? Not exactly sure what red highlight means. No, rather RB's quirk : ) On March 23, 2015, 10:27 p.m., Michael Park wrote: src/local/local.hpp, line 28 https://reviews.apache.org/r/31776/diff/4/?file=902997#file902997line28 This comment looks useless. Can we just get rid of it? Alexander Rojas wrote: +1 Totally agree. However, we have a lot of them in our codebase. I would like to get rid of them, but let's do it in a consistent manner: https://issues.apache.org/jira/browse/MESOS-2564 - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31776/#review77477 --- On March 23, 2015, 2:02 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31776/ --- (Updated March 23, 2015, 2:02 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- This is required for out-of-tree allocator modules. RoleInfo protobuf message has to be extracted into its own public proto file. Diffs - include/mesos/master/allocator.proto PRE-CREATION src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/local/local.hpp 0aa50ef4f44c347eca4b3a409f2a8d03ba321d82 src/local/local.cpp 19083368212b24ce1afef3a5f91d48766d1cd55e src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/master/main.cpp 7cce3a0bb808a1cb7bac9acab31eb1c67a15ea9f src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8 src/tests/cluster.hpp a56b6541adcdebc5866571bbdbb6828df97b34ec src/tests/fault_tolerance_tests.cpp 9ac75b1f601e14a3d3d117775f37a4a48b291dc6 src/tests/hierarchical_allocator_tests.cpp 93753d1c04159a04a733927a487eb69505438e32 src/tests/master_allocator_tests.cpp a432d0207e1a92532a495bf9ad2826414ee4f6f0 src/tests/master_authorization_tests.cpp ff706ed6f8537207b30a548b0ce2121c5df71ab9 src/tests/master_slave_reconciliation_tests.cpp e60f601202fcdbb4cafac190e9b09ca6ce925260 src/tests/master_tests.cpp e69348be676a80017062e3abbd15b8008a6009d7 src/tests/mesos.hpp 45e35204d1aa876fa0c871acf0f21afcd5ababe8 src/tests/mesos.cpp c8f43d21b214e75eaac2870cbdf4f03fd18707d1 src/tests/rate_limiting_tests.cpp d5c00b8db30fc529bc984417a632cf99eb50eb28 Diff: https://reviews.apache.org/r/31776/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31266: Added support for allocator modules.
On March 24, 2015, 5:55 a.m., Michael Park wrote: Michael Park wrote: Hm, upon looking at other files in `include/mesos/module/` it looks like the 2 style issues I mentioned below is apparent in all of them. Is there a reason for this? or shoud they be fixed as a batch in a separate review request? Yes, I followed the general pattern for modules here. Let's do a batch cleanup here: https://issues.apache.org/jira/browse/MESOS-2565 - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31266/#review77528 --- On March 23, 2015, 2:03 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31266/ --- (Updated March 23, 2015, 2:03 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- See summary. Diffs - include/mesos/module/allocator.hpp PRE-CREATION src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/module/manager.cpp 82a38f06e57d034650a6ac32fd73527b38cc97b8 Diff: https://reviews.apache.org/r/31266/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31267: Added a test allocator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/ --- (Updated March 27, 2015, 3:08 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Update description. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description (updated) --- The test allocator module is a built-in HierarchicalDRFAllocator put in a module. NOTE: Here we add harness code to load the module, tests will be wired up later in the patch stack. Diffs - src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/examples/test_allocator_module.cpp PRE-CREATION src/tests/module.hpp 65e567f21744d9a2b07b8680d45bcd1ea47f9508 src/tests/module.cpp b81144f56e94034feecf3a6a4992af078cf60a81 Diff: https://reviews.apache.org/r/31267/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31265: Provided a factory for allocator in tests.
On March 24, 2015, 2:41 p.m., Michael Park wrote: src/tests/master_allocator_tests.cpp, line 97 https://reviews.apache.org/r/31265/diff/4/?file=903029#file903029line97 1. Do we need this to be `virtual` right now? 2. `s/createAllocatorInstance/CreateAllocator/` 3. Can we move this either above `SetUp` or below `TearDown`? seems weird to have it sitting in the middle. 1. I though it make sense to mark it virtual, but I'll remove it if you think it's cleaner. 2. We use `camelCase` 3. Sure. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31265/#review77570 --- On March 23, 2015, 2:11 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31265/ --- (Updated March 23, 2015, 2:11 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The factory creates allocator instances in a way identical to how instances from modules are created. It allows us to use same typed tests for built-in and modularized allocators. Diffs - src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/tests/master_allocator_tests.cpp a432d0207e1a92532a495bf9ad2826414ee4f6f0 Diff: https://reviews.apache.org/r/31265/diff/ Testing --- make check (Mac OS 10.9.5, Ubuntu 14.04) Thanks, Alexander Rukletsov
Re: Review Request 31267: Added a test allocator module.
On March 24, 2015, 6:07 a.m., Michael Park wrote: src/tests/module.cpp, line 135 https://reviews.apache.org/r/31267/diff/4/?file=903021#file903021line135 Can we take `Modules*` here instead to keep consistency with `addAnonymousModules`, etc? or perhaps a patch before this one to make all of them `Modules`? Good catch! This is a recent change, functions signatures used to have references when I started the patch : ) On March 24, 2015, 6:07 a.m., Michael Park wrote: src/tests/module.cpp, line 141 https://reviews.apache.org/r/31267/diff/4/?file=903021#file903021line141 Looking at the `Isolator` example which has `Cpu` and `Mem` isolators, it seems to me like we want `s/testdrfalloactor/testallocator/` here. Alexander Rojas wrote: No very sure about this issue, as I see it, we can have multiple isolators, but you cannot have more than one allocator (though I may be wrong). In that sense, it makes sense to have a test for each of them. Just wondering how that will work if there are more than one allocator module that needs to be tested. Michael Park wrote: Ah, good point! I'm not sure about it either. Alex, please feel free to drop the issue. wondering how that will work if there are more than one allocator module that needs to be tested. Typed tests! Yes, I preferred clarity to consistency here. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/#review77531 --- On March 27, 2015, 3:08 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/ --- (Updated March 27, 2015, 3:08 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The test allocator module is a built-in HierarchicalDRFAllocator put in a module. NOTE: Here we add harness code to load the module, tests will be wired up later in the patch stack. Diffs - src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/examples/test_allocator_module.cpp PRE-CREATION src/tests/module.hpp 65e567f21744d9a2b07b8680d45bcd1ea47f9508 src/tests/module.cpp b81144f56e94034feecf3a6a4992af078cf60a81 Diff: https://reviews.apache.org/r/31267/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31776: Moved allocator to public headers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31776/ --- (Updated March 27, 2015, 4:25 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Addressed comments. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- This is required for out-of-tree allocator modules. RoleInfo protobuf message has to be extracted into its own public proto file. Diffs (updated) - include/mesos/master/allocator.proto PRE-CREATION src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/local/local.hpp 0aa50ef4f44c347eca4b3a409f2a8d03ba321d82 src/local/local.cpp 19083368212b24ce1afef3a5f91d48766d1cd55e src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/master/main.cpp 7cce3a0bb808a1cb7bac9acab31eb1c67a15ea9f src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d src/master/master.cpp ffccc1259b62db89404f6aa6225beeb4c9828684 src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8 src/tests/cluster.hpp a56b6541adcdebc5866571bbdbb6828df97b34ec src/tests/fault_tolerance_tests.cpp a637c32f004638a110390b22cf5b626e904097cf src/tests/hierarchical_allocator_tests.cpp 8861bf398e4bb17b0f74eab4f4af26202447ccef src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 src/tests/master_authorization_tests.cpp ac79303645cc1af337b1dca8db244113d0ba6fce src/tests/master_slave_reconciliation_tests.cpp e60f601202fcdbb4cafac190e9b09ca6ce925260 src/tests/master_tests.cpp 2bfd0f8a20169649cff03509556b4cfa965a9837 src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 src/tests/mesos.cpp 02cbb4b8cf1206d0f32d160addc91d7e0f1ab28b src/tests/rate_limiting_tests.cpp d5c00b8db30fc529bc984417a632cf99eb50eb28 Diff: https://reviews.apache.org/r/31776/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31266: Added support for allocator modules.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31266/ --- (Updated March 27, 2015, 4:25 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Addressed MPark's comments. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- See summary. Diffs (updated) - include/mesos/module/allocator.hpp PRE-CREATION src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/module/manager.cpp 82a38f06e57d034650a6ac32fd73527b38cc97b8 Diff: https://reviews.apache.org/r/31266/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31775: Removed Master::Flags dependency from Allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31775/ --- (Updated March 27, 2015, 4:23 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- On the way to moving allocator to public includes we need to remove the dependency to internal Master::Flags type. Diffs (updated) - src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/master/allocator/mesos/hierarchical.hpp 9f9a631ee52a3ab194e678f9be139e8dabc7f3db src/master/master.cpp ffccc1259b62db89404f6aa6225beeb4c9828684 src/tests/hierarchical_allocator_tests.cpp 8861bf398e4bb17b0f74eab4f4af26202447ccef src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 Diff: https://reviews.apache.org/r/31775/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31262: Moved allocator actions before TestAllocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31262/ --- (Updated March 27, 2015, 4:26 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- Once TestAllocator is not a template, the allocator actions should be defined before they are used in TestAllocator. Diffs (updated) - src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 Diff: https://reviews.apache.org/r/31262/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31267: Added a test allocator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/ --- (Updated March 27, 2015, 4:26 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Addressed MParks's comments. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The test allocator module is a built-in HierarchicalDRFAllocator put in a module. NOTE: Here we add harness code to load the module, tests will be wired up later in the patch stack. Diffs (updated) - src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/examples/test_allocator_module.cpp PRE-CREATION src/tests/module.hpp 65e567f21744d9a2b07b8680d45bcd1ea47f9508 src/tests/module.cpp b81144f56e94034feecf3a6a4992af078cf60a81 Diff: https://reviews.apache.org/r/31267/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31268: Wired up test allocator to allocator tests.
On March 24, 2015, 2:49 p.m., Michael Park wrote: src/tests/master_allocator_tests.cpp, lines 123-126 https://reviews.apache.org/r/31268/diff/4/?file=903030#file903030line123 Could we indent this similar to this example from `src/tests/isolator_tests.cpp`: ``` typedef ::testing::Types CgroupsMemIsolatorProcess, CgroupsCpushareIsolatorProcess, CgroupsPerfEventIsolatorProcess CgroupsIsolatorTypes; ``` ``` typedef ::testing::Types HierarchicalDRFAllocator, tests::ModuleAllocator, TestDRFAllocator AllocatorTypes; ``` I don't know what is consistent here, `src/tests/cram_md5_authentication_tests.cpp` use the indentation I use. Also, clang-format gives something that is more similar to the way I propose. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31268/#review77572 --- On March 23, 2015, 2:27 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31268/ --- (Updated March 23, 2015, 2:27 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- Same typed tests are used for built-in and modularized allocators. Diffs - src/tests/master_allocator_tests.cpp a432d0207e1a92532a495bf9ad2826414ee4f6f0 Diff: https://reviews.apache.org/r/31268/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31268: Wired up test allocator to allocator tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31268/ --- (Updated March 27, 2015, 4:27 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- Same typed tests are used for built-in and modularized allocators. Diffs (updated) - src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 Diff: https://reviews.apache.org/r/31268/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31263: Refactored TestAllocator and allocator text fixture.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31263/ --- (Updated March 27, 2015, 4:27 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- TestAllocator stores a pointer to a real allocator and takes over its ownership. MasterAllocatorTest fixture stores the test allocator in turn. Diffs (updated) - src/tests/containerizer.cpp 26b87ac6b16dfeaf84888e80296ef540697bd775 src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 src/tests/resource_offers_tests.cpp 882a9ff4d09aace486182828bf43b643b0d0c519 src/tests/slave_recovery_tests.cpp 87f4a6aab27d142fa8eb7a6571f684a6ce59687e Diff: https://reviews.apache.org/r/31263/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
