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.
Alexander Rukletsov wrote:
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.
Jie Yu wrote:
Michael, I committed this patch. Follow up with another patch if you want
to add some document as Alex suggested.
I've created [MESOS-2710](https://issues.apache.org/jira/browse/MESOS-2710) to
keep track of this issue.
- Michael
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/#review76752
---
On April 27, 2015, 5:28 p.m., Michael Park wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32140/
---
(Updated April 27, 2015, 5:28 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
Re: Review Request 32982: Added reservation user guide.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32982/#review83054 --- docs/reservation.md https://reviews.apache.org/r/32982/#comment133920 In the following sections docs/reservation.md https://reviews.apache.org/r/32982/#comment133922 existed prior to 0.23.0 seems to imply it no longer exists after 0.23.0? docs/reservation.md https://reviews.apache.org/r/32982/#comment133924 A notable difference between static reservation is the principal here, are you going to cover that? docs/reservation.md https://reviews.apache.org/r/32982/#comment133926 What happens if you try to unreserve a reservation that is being used right now? - Timothy Chen On May 8, 2015, 6:57 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32982/ --- (Updated May 8, 2015, 6:57 p.m.) Review request for mesos, Alexander Rukletsov and Timothy Chen. Bugs: MESOS-2205 https://issues.apache.org/jira/browse/MESOS-2205 Repository: mesos Description --- See summary. NOTE: Do not commit until master API endpoints are implemented for dynamic reservations. Diffs - docs/reservation.md PRE-CREATION Diff: https://reviews.apache.org/r/32982/diff/ Testing --- Documentation. Thanks, Michael Park
Re: Review Request 32982: Added reservation user guide.
On April 24, 2015, 8:17 a.m., Alexander Rukletsov wrote: LGTM (not a native speaker). Shouldn't we commit this RR after introducing master endpoints : )? Shouldn't we commit this RR after introducing master endpoints : )? Updated the description to make this clear. On April 24, 2015, 8:17 a.m., Alexander Rukletsov wrote: docs/reservation.md, lines 97-100 https://reviews.apache.org/r/32982/diff/1/?file=921006#file921006line97 Do you want to mention HTTP response codes for response? Yes. Thanks for pointing that out. My plan is to punt the decision to the implementation of master API endpoints. - Michael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32982/#review81459 --- On May 8, 2015, 6:57 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32982/ --- (Updated May 8, 2015, 6:57 p.m.) Review request for mesos, Alexander Rukletsov and Timothy Chen. Bugs: MESOS-2205 https://issues.apache.org/jira/browse/MESOS-2205 Repository: mesos Description --- See summary. NOTE: Do not commit until master API endpoints are implemented for dynamic reservations. Diffs - docs/reservation.md PRE-CREATION Diff: https://reviews.apache.org/r/32982/diff/ Testing --- Documentation. Thanks, Michael Park
Re: Review Request 33918: Added resources estimator abstraction for oversubscription.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33918/ --- (Updated May 8, 2015, 8:24 p.m.) Review request for mesos, Ben Mahler, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone. Changes --- Addressed review comments. Bugs: MESOS-2649 https://issues.apache.org/jira/browse/MESOS-2649 Repository: mesos Description --- Added resources estimator abstraction for oversubscription. This patch defines the interface of the resources estimator and creates a default stub implementation. Diffs (updated) - include/mesos/slave/resource_estimator.hpp PRE-CREATION src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b src/slave/resource_estimator.hpp PRE-CREATION src/slave/resource_estimator.cpp PRE-CREATION Diff: https://reviews.apache.org/r/33918/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 33514: Wired up --allocator flag in master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33514/ --- (Updated May 8, 2015, 8:44 p.m.) Review request for mesos, Kapil Arya, Niklas Nielsen, and Till Toenshoff. Changes --- Addressed Vinod's comment. Bugs: MESOS-2597 https://issues.apache.org/jira/browse/MESOS-2597 Repository: mesos Description --- See summary. Diffs (updated) - src/master/flags.hpp 996cf38c88f9718e55e88d6e906b5e3d1989478a src/master/flags.cpp 5798989d3f135978ec3d5f714b1cd8d84180fc90 src/master/main.cpp 18f8c3178459da0cbf23a1817ec49cd9d3998bfd Diff: https://reviews.apache.org/r/33514/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Checked the whole pipeline works with a modularized allocator. The allocator is HierarchicalDRF packed into a library: https://github.com/rukletsov/mesos-modules/tree/master/external-allocator Master and slave have been pointed to an allocator module: ``` ./bin/mesos-master.sh --work_dir=m/work --modules=file://path/external-allocator.json --allocator=ExternalAllocatorModule ./bin/mesos-slave.sh --master=master-ip --work_dir=s/work ``` Slave successfully registers, its resources are offered to a simple task: ``` ./src/mesos-execute --command=sleep 1 --master=master-ip --name=alloc-test ``` Thanks, Alexander Rukletsov
Re: Review Request 33514: Wired up --allocator flag in master.
On May 8, 2015, 8:58 p.m., Joerg Schad wrote: Please add to documentation After that Ship it! - Joerg --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33514/#review83085 --- On May 8, 2015, 8:44 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33514/ --- (Updated May 8, 2015, 8:44 p.m.) Review request for mesos, Kapil Arya, Niklas Nielsen, and Till Toenshoff. Bugs: MESOS-2597 https://issues.apache.org/jira/browse/MESOS-2597 Repository: mesos Description --- See summary. Diffs - src/master/flags.hpp 996cf38c88f9718e55e88d6e906b5e3d1989478a src/master/flags.cpp 5798989d3f135978ec3d5f714b1cd8d84180fc90 src/master/main.cpp 18f8c3178459da0cbf23a1817ec49cd9d3998bfd Diff: https://reviews.apache.org/r/33514/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Checked the whole pipeline works with a modularized allocator. The allocator is HierarchicalDRF packed into a library: https://github.com/rukletsov/mesos-modules/tree/master/external-allocator Master and slave have been pointed to an allocator module: ``` ./bin/mesos-master.sh --work_dir=m/work --modules=file://path/external-allocator.json --allocator=ExternalAllocatorModule ./bin/mesos-slave.sh --master=master-ip --work_dir=s/work ``` Slave successfully registers, its resources are offered to a simple task: ``` ./src/mesos-execute --command=sleep 1 --master=master-ip --name=alloc-test ``` Thanks, Alexander Rukletsov
Re: Review Request 33514: Wired up --allocator flag in master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33514/#review83095 --- Patch looks great! Reviews applied: [33513, 33514] All tests passed. - Mesos ReviewBot On May 8, 2015, 8:44 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33514/ --- (Updated May 8, 2015, 8:44 p.m.) Review request for mesos, Kapil Arya, Niklas Nielsen, and Till Toenshoff. Bugs: MESOS-2597 https://issues.apache.org/jira/browse/MESOS-2597 Repository: mesos Description --- See summary. Diffs - src/master/flags.hpp 996cf38c88f9718e55e88d6e906b5e3d1989478a src/master/flags.cpp 5798989d3f135978ec3d5f714b1cd8d84180fc90 src/master/main.cpp 18f8c3178459da0cbf23a1817ec49cd9d3998bfd Diff: https://reviews.apache.org/r/33514/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Checked the whole pipeline works with a modularized allocator. The allocator is HierarchicalDRF packed into a library: https://github.com/rukletsov/mesos-modules/tree/master/external-allocator Master and slave have been pointed to an allocator module: ``` ./bin/mesos-master.sh --work_dir=m/work --modules=file://path/external-allocator.json --allocator=ExternalAllocatorModule ./bin/mesos-slave.sh --master=master-ip --work_dir=s/work ``` Slave successfully registers, its resources are offered to a simple task: ``` ./src/mesos-execute --command=sleep 1 --master=master-ip --name=alloc-test ``` Thanks, Alexander Rukletsov
Re: Review Request 33490: MESOS-2633 Avoid adding duplicate tasks
On May 7, 2015, 8:37 a.m., Adam B wrote: Thanks for attacking one of my TODOs. I've got a couple of questions, but it looks pretty straightforward to me. I'm confused about when `==` would be too strict. What fields would you expect to differ? Maybe if you got a TASK_LOST followed by a TASK_FAILED for the same taskId, as is supposedly possible in some weird race? https://github.com/apache/mesos/blob/master/src/master/master.cpp#L3738 Testing: It's probably unfair to link to a private mesosphere-only build result in your Testing section. Did the buildbot pass `make distcheck` as well as just `make check`? Is there a unit test to verify your change? Do we need one? I have yet to learn what our recommended coding style is for lambdas now, so I'll defer to @jvanremoortere for his approval there. This was more a question I was pondering myself: the current `==` does a bitwise comparison of two `Task` objects; my question was around the semantic meaning of equality for two `Task`s: are they *equal* if and only if they are *identical*, or is there a narrower definition of *equality* (eg, at one extreme, one could just use the `Task::task_id` and ignore all other fields). In fact, in this case, should we just do that? (it would certainly address somewhat, if not entirely, your other concern about performance impact) - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33490/#review82809 --- On May 7, 2015, 8:26 a.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33490/ --- (Updated May 7, 2015, 8:26 a.m.) Review request for mesos, Adam B and Joris Van Remoortere. Bugs: MESOS-2633 https://issues.apache.org/jira/browse/MESOS-2633 Repository: mesos Description --- In Framework::addCompletedTask(const Task task) we did not check for duplicated tasks, so they could be added more than once. A simple check has now been added (there still is the issue of whether the `operator ==` on `Task` is too strict, so as never to cause a match). Diffs - src/master/framework.cpp PRE-CREATION Diff: https://reviews.apache.org/r/33490/diff/ Testing --- buildbot make check pass http://build.mesosphere.com:/builders/dev_test/builds/13 Thanks, Marco Massenzio
Re: Review Request 33718: Extended documentation on Mesos hooks.
On May 7, 2015, 9:06 p.m., Niklas Nielsen wrote: docs/modules.md, line 149 https://reviews.apache.org/r/33718/diff/2/?file=952571#file952571line149 s/config/configuration file/? It does not have to be a file, hence I would suggest configuration. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33718/#review82908 --- On May 7, 2015, 8:40 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33718/ --- (Updated May 7, 2015, 8:40 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Till Toenshoff. Bugs: MESOS-2680 https://issues.apache.org/jira/browse/MESOS-2680 Repository: mesos Description --- Mentions necessary flags and adds a usage example. Diffs - docs/modules.md a8b471541cdfa584eeb89fbe96643f93c712cfd4 Diff: https://reviews.apache.org/r/33718/diff/ Testing --- none: docs update. @Adam: as a native speaker, do you mind checking the language? Thanks, Alexander Rukletsov
Re: Review Request 33718: Extended documentation on Mesos hooks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33718/#review83074 --- docs/modules.md https://reviews.apache.org/r/33718/#comment133941 I would like to propose using two terms for making clear the specific roles of those flags. `--modules` is there for __introducing__ any module (name, location and parameters) to mesos. `--hooks` is there for __selecting__ a specific module as a hooks module. Hence my clumpsy attempt to reword: For introducing any module to mesos, you need to specify its name via the `--modules` flag configuration. For selecting that module as a hook module, you will have to additionally specify it via the `--hooks` flag. docs/modules.md https://reviews.apache.org/r/33718/#comment133942 s/5050/PORT/ s/--work_dir=s\/work// - Till Toenshoff On May 7, 2015, 8:40 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33718/ --- (Updated May 7, 2015, 8:40 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Till Toenshoff. Bugs: MESOS-2680 https://issues.apache.org/jira/browse/MESOS-2680 Repository: mesos Description --- Mentions necessary flags and adds a usage example. Diffs - docs/modules.md a8b471541cdfa584eeb89fbe96643f93c712cfd4 Diff: https://reviews.apache.org/r/33718/diff/ Testing --- none: docs update. @Adam: as a native speaker, do you mind checking the language? Thanks, Alexander Rukletsov
Re: Review Request 33718: Extended documentation on Mesos hooks.
On May 7, 2015, 9:06 p.m., Niklas Nielsen wrote: Have you rendered this in a markdown viewer? As far as I know, the code block won't render if you don't have a preceeding newline I have and it was OK, but you're right and it's not consistent with the rest of the doc, I'll change that. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33718/#review82908 --- On May 8, 2015, 11:19 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33718/ --- (Updated May 8, 2015, 11:19 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Till Toenshoff. Bugs: MESOS-2680 https://issues.apache.org/jira/browse/MESOS-2680 Repository: mesos Description --- Mentions necessary flags and adds a usage example. Diffs - docs/modules.md a8b471541cdfa584eeb89fbe96643f93c712cfd4 Diff: https://reviews.apache.org/r/33718/diff/ Testing --- none: docs update. @Adam: as a native speaker, do you mind checking the language? Thanks, Alexander Rukletsov
Re: Review Request 33823: Protocol file with the HTTP API messages between executor and slave
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33823/#review83128 --- include/mesos/executor/executor.hpp https://reviews.apache.org/r/33823/#comment133995 not sure whether this is common practice in Mesos - if so, please feel free to ignore this comment. I find it confusing that this include file is named exactly as /mesos/executor.hpp - there is nothing (for the uninitiated) to indicate that this is just a 'shell' around the Protobuf generated include: not the location, not the name, not nothing - and it has the same name as a completely different include. Again, if this is the way it is, so be it - if not, please consider renaming it something more meaningful? - Marco Massenzio On May 4, 2015, 10:21 p.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33823/ --- (Updated May 4, 2015, 10:21 p.m.) Review request for mesos, Isabel Jimenez, Marco Massenzio, and Vinod Kone. Repository: mesos Description --- See summary. Diffs - include/mesos/executor/executor.hpp PRE-CREATION include/mesos/executor/executor.proto PRE-CREATION src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b Diff: https://reviews.apache.org/r/33823/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 33513: Added a modules-aware factory for allocators.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33513/ --- (Updated May 8, 2015, 11:47 p.m.) Review request for mesos, Kapil Arya, Niklas Nielsen, and Till Toenshoff. Changes --- Updated the comment. Bugs: MESOS-2597 https://issues.apache.org/jira/browse/MESOS-2597 Repository: mesos Description --- With the support of allocator modules, it should be possible to select and instantiate an allocator based on its name. Diffs (updated) - include/mesos/master/allocator.hpp e821bf33536ab70cc2be45b9239c089e251f1f24 src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b src/master/allocator/allocator.cpp PRE-CREATION src/master/allocator/mesos/allocator.hpp 2681af5ff265b84c5174de3ad459292e006dcf97 src/master/allocator/mesos/hierarchical.hpp 09adced9d8712b3eeda885d598443791186890db src/master/constants.hpp c386eab3973363e64c113f7e84452456fdd3393c src/master/constants.cpp 9ee17e9ad884da98cabfdfe316cb4a831de193b3 src/master/main.cpp 18f8c3178459da0cbf23a1817ec49cd9d3998bfd Diff: https://reviews.apache.org/r/33513/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 33513: Added a modules-aware factory for allocators.
On May 6, 2015, 10:39 p.m., Vinod Kone wrote: include/mesos/master/allocator.hpp, lines 54-55 https://reviews.apache.org/r/33513/diff/4/?file=951247#file951247line54 Instead of empty string, make the argument Optionstring? Alexander Rukletsov wrote: I think a common pattern in such cases is to use a constant like `DEFAULT_XXX` and not `Optionstring`, e.g. `MasterDetector`, `Authenticator`, `Authenticatee`. if there is a DEFAULT_* constant, isn't that always be passed to the create() to get the default module? Why would one need to pass an empty string in that case? - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33513/#review82748 --- On May 8, 2015, 11:47 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33513/ --- (Updated May 8, 2015, 11:47 p.m.) Review request for mesos, Kapil Arya, Niklas Nielsen, and Till Toenshoff. Bugs: MESOS-2597 https://issues.apache.org/jira/browse/MESOS-2597 Repository: mesos Description --- With the support of allocator modules, it should be possible to select and instantiate an allocator based on its name. Diffs - include/mesos/master/allocator.hpp e821bf33536ab70cc2be45b9239c089e251f1f24 src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b src/master/allocator/allocator.cpp PRE-CREATION src/master/allocator/mesos/allocator.hpp 2681af5ff265b84c5174de3ad459292e006dcf97 src/master/allocator/mesos/hierarchical.hpp 09adced9d8712b3eeda885d598443791186890db src/master/constants.hpp c386eab3973363e64c113f7e84452456fdd3393c src/master/constants.cpp 9ee17e9ad884da98cabfdfe316cb4a831de193b3 src/master/main.cpp 18f8c3178459da0cbf23a1817ec49cd9d3998bfd Diff: https://reviews.apache.org/r/33513/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
