Re: Review Request 29089: Fixed the DRFSorter share calculation to not assume flattened resources.

[Ben Mahler]

> On Dec. 16, 2014, 9:55 p.m., Jie Yu wrote:
> > src/tests/sorter_tests.cpp, lines 182-183
> > 
> >
> > Consider using ```createDiskInfo``` in src/test/mesos.hpp?

Good idea, although given mesos.hpp is largely a header leveraged by the 
integration tests, could we move it to src/test/utils.hpp?


- Ben


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29089/#review65246
---


On Dec. 16, 2014, 6:28 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29089/
> ---
> 
> (Updated Dec. 16, 2014, 6:28 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-2099
> https://issues.apache.org/jira/browse/MESOS-2099
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> We broke the share calculation as soon as we allowed resources to be 
> non-flattened.
> 
> 
> Diffs
> -
> 
>   src/master/drf_sorter.cpp 0ad6c52759e1ceec4b43f99d8fa1e75d7e2e1c31 
>   src/tests/sorter_tests.cpp 0516ab573d9f4f2af249978e15ebf52c2afb5359 
> 
> Diff: https://reviews.apache.org/r/29089/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 29134: Added an overload of Resources::contains for Resource object.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29134/#review65278
---


Patch looks great!

Reviews applied: [29134]

All tests passed.

- Mesos ReviewBot


On Dec. 17, 2014, 1:36 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29134/
> ---
> 
> (Updated Dec. 17, 2014, 1:36 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 296553a 
>   src/common/resources.cpp 9bf7ae9 
> 
> Diff: https://reviews.apache.org/r/29134/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 29088: Removed an unnecessary helper in the sorter tests.

[Ben Mahler]

> On Dec. 16, 2014, 6:16 p.m., Jie Yu wrote:
> > src/tests/sorter_tests.cpp, line 71
> > 
> >
> > Curious if the following will compile or not:
> > 
> > EXPECT_EQ(sorter.sort(), {"a", "b"});

Since the expected value should be on the left, I had tried the following 
unsuccessfully:

```
EXPECT_EQ({"a", "b"}, sorter.sort());
```

Also, likely don't want to risk another 4.4.7 issue with implicit construction 
from initializer lists, given the issue benh reported.


- Ben


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29088/#review65217
---


On Dec. 16, 2014, 6:28 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29088/
> ---
> 
> (Updated Dec. 16, 2014, 6:28 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> This helper is no longer necessary, and the EXPECT statments inside a method 
> loses line number information when the tests fail.
> 
> 
> Diffs
> -
> 
>   src/tests/sorter_tests.cpp 0516ab573d9f4f2af249978e15ebf52c2afb5359 
> 
> Diff: https://reviews.apache.org/r/29088/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 28698: [WIP] Modified Resources to account for reservation type.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28698/#review65274
---


Patch looks great!

Reviews applied: [28697, 28698]

All tests passed.

- Mesos ReviewBot


On Dec. 17, 2014, 12:56 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28698/
> ---
> 
> (Updated Dec. 17, 2014, 12:56 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, and 
> Vinod Kone.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Modified Resources to account for reservation type.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc 
>   src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92 
> 
> Diff: https://reviews.apache.org/r/28698/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 29134: Added an overload of Resources::contains for Resource object.

[Ben Mahler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29134/#review65272
---



include/mesos/resources.hpp


Hm.. I must be missing something subtle here. Why is this necessary to 
point out and differentiate with _contains?

Is it possible for a Resources object to contain an invalid Resource?


- Ben Mahler


On Dec. 17, 2014, 1:36 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29134/
> ---
> 
> (Updated Dec. 17, 2014, 1:36 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 296553a 
>   src/common/resources.cpp 9bf7ae9 
> 
> Diff: https://reviews.apache.org/r/29134/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 29134: Added an overload of Resources::contains for Resource object.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29134/
---

Review request for mesos and Ben Mahler.


Repository: mesos-git


Description
---

See summary.


Diffs
-

  include/mesos/resources.hpp 296553a 
  src/common/resources.cpp 9bf7ae9 

Diff: https://reviews.apache.org/r/29134/diff/


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 28815: Converted an initial DRF integration test to a unit test.

[Ben Mahler]

> On Dec. 16, 2014, 10:14 p.m., Benjamin Hindman wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 173
> > 
> >
> > This breaks on my Linux box with gcc 4.4.7.
> > 
> > $ uname -a
> > Linux centos6-build 2.6.32-431.23.3.el6.x86_64 #1 SMP Thu Jul 31 
> > 17:20:51 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
> > 
> > $ gcc --version
> > gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-11)
> > Copyright (C) 2010 Free Software Foundation, Inc.
> > This is free software; see the source for copying conditions.  There is 
> > NO
> > warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR 
> > PURPOSE.

Fixed, thanks for sending me the patch!


- Ben


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28815/#review65250
---


On Dec. 11, 2014, 2:48 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28815/
> ---
> 
> (Updated Dec. 11, 2014, 2:48 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Now that the allocator can be unit tested (r/28814), this adds the unit test 
> base calss, and moves over the DRF test to a unit test.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8 
>   src/tests/allocator_tests.cpp 8626362e5d45304a4800a807d5171178e457ff53 
>   src/tests/hierarchical_allocator_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/28815/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 29128: Added a Resources transformation for acquiring persistent disk.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29128/#review65270
---


Patch looks great!

Reviews applied: [29128]

All tests passed.

- Mesos ReviewBot


On Dec. 17, 2014, 1:10 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29128/
> ---
> 
> (Updated Dec. 17, 2014, 1:10 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> See summary. Seprated out from https://reviews.apache.org/r/28720/
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 296553a 
>   src/common/resources.cpp 9bf7ae9 
>   src/tests/resources_tests.cpp bac092e 
> 
> Diff: https://reviews.apache.org/r/29128/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 29128: Added a Resources transformation for acquiring persistent disk.

[Ben Mahler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29128/#review65269
---

Ship it!



src/tests/resources_tests.cpp


Looks like you can use EXPECT_SOME_EQ to avoid the separate statements?


- Ben Mahler


On Dec. 17, 2014, 1:10 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29128/
> ---
> 
> (Updated Dec. 17, 2014, 1:10 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> See summary. Seprated out from https://reviews.apache.org/r/28720/
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 296553a 
>   src/common/resources.cpp 9bf7ae9 
>   src/tests/resources_tests.cpp bac092e 
> 
> Diff: https://reviews.apache.org/r/29128/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 29128: Added a Resources transformation for acquiring persistent disk.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29128/
---

(Updated Dec. 17, 2014, 1:10 a.m.)


Review request for mesos and Ben Mahler.


Changes
---

Review comments.


Bugs: MESOS-2030
https://issues.apache.org/jira/browse/MESOS-2030


Repository: mesos-git


Description
---

See summary. Seprated out from https://reviews.apache.org/r/28720/


Diffs (updated)
-

  include/mesos/resources.hpp 296553a 
  src/common/resources.cpp 9bf7ae9 
  src/tests/resources_tests.cpp bac092e 

Diff: https://reviews.apache.org/r/29128/diff/


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 29128: Added a Resources transformation for acquiring persistent disk.

[Jie Yu]

> On Dec. 17, 2014, 12:53 a.m., Ben Mahler wrote:
> > include/mesos/resources.hpp, line 241
> > 
> >
> > It might not be clear what "no transformation will be applied" means, 
> > error?
> > 
> > How about:
> > s/no transformation will be applied/no change occurs/

Invalid after the discussion.


> On Dec. 17, 2014, 12:53 a.m., Ben Mahler wrote:
> > src/common/resources.cpp, lines 918-922
> > 
> >
> > How about:
> > 
> > ```
> > if (resources.contains(disk)) {
> >   return resources; // No-op: already acquired!
> > }

Invalid after the discussion.


> On Dec. 17, 2014, 12:53 a.m., Ben Mahler wrote:
> > src/common/resources.cpp, line 920
> > 
> >
> > Is Resources() here necessary? It looks like there is an operator for 
> > just doing:
> > 
> > ```
> > if (resources.contains(disk)) {
> >   ...
> > }
> > ```
> > 
> > Via this:
> > ```
> > bool Resources::contains(const Resource& that) const;
> > ```

We dont' have 'contains' for Resource object. I'll add one in a following 
patch. Drop a TODO here.


> On Dec. 17, 2014, 12:53 a.m., Ben Mahler wrote:
> > src/common/resources.cpp, line 932
> > 
> >
> > Might want to wrap the ID in single quotes, unless we're heavily 
> > sanitizing it (e.g. do we allow whitespace?)

No longer valid.


> On Dec. 17, 2014, 12:53 a.m., Ben Mahler wrote:
> > src/tests/resources_tests.cpp, line 952
> > 
> >
> > Just curious, where will we catch a duplicate container path under the 
> > same executor?
> > 
> > Do you have an integration test for this at least? Let's keep track of 
> > this extra case.

Will check that in master.


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29128/#review65262
---


On Dec. 16, 2014, 11:56 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29128/
> ---
> 
> (Updated Dec. 16, 2014, 11:56 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> See summary. Seprated out from https://reviews.apache.org/r/28720/
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 296553a 
>   src/common/resources.cpp 9bf7ae9 
>   src/tests/resources_tests.cpp bac092e 
> 
> Diff: https://reviews.apache.org/r/29128/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 29083: Updated Sorter to allow transforming allocated resources.

[Ben Mahler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29083/#review65267
---



src/master/drf_sorter.hpp


Jie and I were chatting and realized that since the sorter aggregates 
resources across slaves (which breaks for non-scalar things like ranges and 
sets), the disk transformation would have issues with duplicated IDs here.

Jie is updating the transformation to be simpler and non-idempotent which 
will fix the issue here (can have duplicate persistence IDs in the disk 
resources), but ideally we stop merging resources across slaves.

At the very least a TODO is needed here!


- Ben Mahler


On Dec. 16, 2014, 6:28 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29083/
> ---
> 
> (Updated Dec. 16, 2014, 6:28 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-2099
> https://issues.apache.org/jira/browse/MESOS-2099
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> In order for frameworks to perform resource transformations on their 
> allocations, the allocator must be able to update the allocation in the 
> sorter objects.
> 
> Note that the sorters cannot simply take a Transformation, as done in 
> r/29084/. This is because sorters may not store the complete allocation for a 
> client (e.g. role sorter doesn't track reserved resources).
> 
> This means that taking a Transformation is problematic for the role sorter in 
> the allocator.
> 
> 
> Diffs
> -
> 
>   src/master/drf_sorter.hpp c47b56afdb8f9f3ff0793f0d10a4e4c656e6a212 
>   src/master/drf_sorter.cpp 0ad6c52759e1ceec4b43f99d8fa1e75d7e2e1c31 
>   src/master/sorter.hpp 0150f90b9aab2ecb4edd07dca7f0d58997e422aa 
>   src/tests/sorter_tests.cpp 0516ab573d9f4f2af249978e15ebf52c2afb5359 
> 
> Diff: https://reviews.apache.org/r/29083/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 28698: [WIP] Modified Resources to account for reservation type.

[Michael Park]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28698/
---

(Updated Dec. 17, 2014, 12:56 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, and 
Vinod Kone.


Repository: mesos-git


Description
---

Modified Resources to account for reservation type.


Diffs
-

  include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc 
  src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92 

Diff: https://reviews.apache.org/r/28698/diff/


Testing (updated)
---

make check


Thanks,

Michael Park

Re: Review Request 28697: [WIP] Add ReservationType and ReservationInfo for dynamic reservations.

[Michael Park]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28697/
---

(Updated Dec. 17, 2014, 12:56 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, and 
Vinod Kone.


Bugs: MESOS-2137
https://issues.apache.org/jira/browse/MESOS-2137


Repository: mesos-git


Description
---

Adding new protobuf messages necessary to support dynamic reservations.


Diffs
-

  include/mesos/mesos.proto 540071db64961466eb75c779b3ea6863f4594437 

Diff: https://reviews.apache.org/r/28697/diff/


Testing (updated)
---

make check


Thanks,

Michael Park

Re: Review Request 29128: Added a Resources transformation for acquiring persistent disk.

[Ben Mahler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29128/#review65262
---


Thanks for splitting!

Can you simplify the Transformation per your suggestion when we chatted? 
(Simple transformation that is non-idempotent, master contains the validation 
around persistence IDs). Some of my comments will become invalid or will move 
to the master code.

Then we can get this shipped, I'll need to update the sorters to not aggregate 
resources across slaves (or at least drop a TODO). I'll make a note on my 
review.


include/mesos/resources.hpp


It might not be clear what "no transformation will be applied" means, error?

How about:
s/no transformation will be applied/no change occurs/



src/common/resources.cpp


How about:

```
if (resources.contains(disk)) {
  return resources; // No-op: already acquired!
}



src/common/resources.cpp


Is Resources() here necessary? It looks like there is an operator for just 
doing:

```
if (resources.contains(disk)) {
  ...
}
```

Via this:
```
bool Resources::contains(const Resource& that) const;
```



src/common/resources.cpp


Might want to wrap the ID in single quotes, unless we're heavily sanitizing 
it (e.g. do we allow whitespace?)



src/tests/resources_tests.cpp


Just curious, where will we catch a duplicate container path under the same 
executor?

Do you have an integration test for this at least? Let's keep track of this 
extra case.


- Ben Mahler


On Dec. 16, 2014, 11:56 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29128/
> ---
> 
> (Updated Dec. 16, 2014, 11:56 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> See summary. Seprated out from https://reviews.apache.org/r/28720/
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 296553a 
>   src/common/resources.cpp 9bf7ae9 
>   src/tests/resources_tests.cpp bac092e 
> 
> Diff: https://reviews.apache.org/r/29128/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 28698: [WIP] Modified Resources to account for reservation type.

[Michael Park]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28698/
---

(Updated Dec. 17, 2014, 12:48 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, and 
Vinod Kone.


Repository: mesos-git


Description
---

Modified Resources to account for reservation type.


Diffs
-

  include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc 
  src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92 

Diff: https://reviews.apache.org/r/28698/diff/


Testing
---


Thanks,

Michael Park

Re: Review Request 28697: [WIP] Add ReservationType and ReservationInfo for dynamic reservations.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28697/#review65264
---


Patch looks great!

Reviews applied: [28697]

All tests passed.

- Mesos ReviewBot


On Dec. 16, 2014, 11:01 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28697/
> ---
> 
> (Updated Dec. 16, 2014, 11:01 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-2137
> https://issues.apache.org/jira/browse/MESOS-2137
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Adding new protobuf messages necessary to support dynamic reservations.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 540071db64961466eb75c779b3ea6863f4594437 
> 
> Diff: https://reviews.apache.org/r/28697/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: using std::unique_ptr

[Benjamin Hindman]

>
> The style guide has the following to say (long quote, sorry):
>

To be clear to other people on the dev list, this is from Google's style
guide, which we use in Mesos except for explicit places where we diverge. I
say this because std::unique_ptr might be one such place, and we shouldn't
eliminate that as an option if it makes sense.

Now I'm a big fan of explicit ownership and moving ownership rather than
> sharing non-smart pointers, but I recognise that in our code-base,
> ownership is difficult to reason about in many cases. However, we do have
> quite a few cases where we manage lifetime scope with explicit delete calls
> and I'd like to start by eliminating those. Ie, using std::unique_ptr to
> manage lifetime not necessarily ownership.
>

I'm also a big fan explicit ownership and move semantics, more generally of
introducing abstractions that can help us write more robust and reliable
code. But I do want to point out that memory leaks, the problem it sounds
like you're addressing with using std::unique_ptr to manage lifetime, has
been a very minor source of bugs in our code base for the last 5 years.

Moreover, in places where we've used raw pointers (especially in
libprocess), the safety that has come from forcing a programmer to think
about what's happening when they write 'delete pointer;' has been hugely
valuable in eliminating bugs that could now become more common.


> This is difficult though as we may pass these pointers to other methods or
> even other libprocess processes. In cases that we can reason about the
> lifetime of the various pointers, that should be fine, but we have to be
> careful.
>

This is where I'd love to see some examples of how we'd use
std::unique_ptr. In the Master, for example, we pass things like Framework*
and Task* around, how would we do the same with std::unique_ptr? What would
be the new convention? These are the questions I'd love to see asked and
examples shown that everyone can discuss before making a decision we might
later regret.

One option is to start by replacing these pointers with process::Owned. The
> downside to this approach is that it introduces more non-standard types
> and, because Owned is implemented using std::shared_ptr, doesn't move us
> closer to defining clear ownership.
>

I don't think we should be afraid of introducing non-standard types like
Owned. If a non-standard type makes the code more readable, or can provide
value that the standard doesn't provide, it's a no brainer. This has
happened countless times in Mesos, and we've almost always decided to go
for something non-standard to make code easier to maintain going forward.

I do understand the value of being able to expose a public API that doesn't
require our non-standard types, but I don't think that's a hugely pressing
issue right now.

Also, it would be great to see Owned reimplemented in terms of
std::unique_ptr along with any necessary cleanups.

​So the floor is open. Do we:
>

To answer this question I'd love to see more examples from within our
codebase that can drive how we introduce std::unique_ptr.

Ben.

Re: Review Request 28720: Adjusted the calculation of unused resources in _launchTasks by considering persistent disk acquisition.

[Jie Yu]

> On Dec. 16, 2014, 9:59 p.m., Ben Mahler wrote:
> > src/common/resources.cpp, lines 922-930
> > 
> >
> > Duplicate persistence IDs are allowed across roles? Is that handled 
> > everywhere else in the code?

Yes. It's handled. Also, see include/mesos/mesos.proto


> On Dec. 16, 2014, 9:59 p.m., Ben Mahler wrote:
> > src/common/resources.cpp, line 928
> > 
> >
> > Can we include the ID?

Done.


> On Dec. 16, 2014, 9:59 p.m., Ben Mahler wrote:
> > src/common/resources.cpp, line 909
> > 
> >
> > How about splitting these checks?
> > 
> > ```
> > CHECK(disk.has_disk());
> > CHECK(disk.disk().has_persistence());
> > ```

Done.


> On Dec. 16, 2014, 9:59 p.m., Ben Mahler wrote:
> > src/common/resources.cpp, lines 946-947
> > 
> >
> > How about `stripped` or `strippedDisk`? Might be a bit easier to read.

Done.


> On Dec. 16, 2014, 9:59 p.m., Ben Mahler wrote:
> > src/common/resources.cpp, line 958
> > 
> >
> > How about:
> > 
> > ```
> > return Error("Insufficient disk resources");
> > ```

Done.


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28720/#review65237
---


On Dec. 16, 2014, 9:11 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28720/
> ---
> 
> (Updated Dec. 16, 2014, 9:11 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Introduced an abstraction for mutating resources. Acquiring persistent disk 
> is one type of resources mutation.
> 
> Infer persistent disk acquisitions from resources and check resource usage 
> against adjusted total resources.
> 
> Adjusted the calculation of unused resources in _launchTasks by considering 
> persistent disk acquisition.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc 
>   src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92 
>   src/master/master.cpp 0f55a5cc2d6845cbaace718a48f771d80aad0e6e 
>   src/tests/resource_offers_tests.cpp 
> e13b6c5460d9e6729843c40bed9e4d4e3f76d5d3 
> 
> Diff: https://reviews.apache.org/r/28720/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 29128: Added a Resources transformation for acquiring persistent disk.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29128/
---

Review request for mesos and Ben Mahler.


Bugs: MESOS-2030
https://issues.apache.org/jira/browse/MESOS-2030


Repository: mesos-git


Description
---

See summary. Seprated out from https://reviews.apache.org/r/28720/


Diffs
-

  include/mesos/resources.hpp 296553a 
  src/common/resources.cpp 9bf7ae9 
  src/tests/resources_tests.cpp bac092e 

Diff: https://reviews.apache.org/r/29128/diff/


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 28698: [WIP] Modified Resources to account for reservation type.

[Michael Park]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28698/#review65258
---



src/common/resources.cpp


What should be the default behaviour for functions like `reserved(role)`?
  1. Return resources with the same role, ignoring reservations, or
  2. Return resources with the same role and set reservation to be STATIC 
by default.

I've implemented 1 here because I believe it's a better API but I'm 
wondering if it has backwards-compatibility issues.


- Michael Park


On Dec. 16, 2014, 11:04 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28698/
> ---
> 
> (Updated Dec. 16, 2014, 11:04 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, and 
> Vinod Kone.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Modified Resources to account for reservation type.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc 
>   src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92 
> 
> Diff: https://reviews.apache.org/r/28698/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 28698: [WIP] Modified Resources to account for reservation type.

[Michael Park]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28698/
---

(Updated Dec. 16, 2014, 11:53 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, and 
Vinod Kone.


Repository: mesos-git


Description
---

Modified Resources to account for reservation type.


Diffs
-

  include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc 
  src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92 

Diff: https://reviews.apache.org/r/28698/diff/


Testing
---


Thanks,

Michael Park

Re: Review Request 29071: added webui_log option

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29071/#review65259
---


Patch looks great!

Reviews applied: [29071]

All tests passed.

- Mesos ReviewBot


On Dec. 16, 2014, 10:37 p.m., David Robinson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29071/
> ---
> 
> (Updated Dec. 16, 2014, 10:37 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-2193
> https://issues.apache.org/jira/browse/MESOS-2193
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> added webui_log option
> 
> 
> Diffs
> -
> 
>   src/master/flags.hpp 1cea50c02f3ad7de1e1ae91d65d1accdb9af7b03 
>   src/master/http.cpp 46890bed05d7c4b63e1f7be5bb35217173e0ade8 
>   src/master/master.cpp 0f55a5cc2d6845cbaace718a48f771d80aad0e6e 
>   src/slave/flags.hpp 670997dc3a702cd5edf33f2e5824c5e4dfe4ecef 
>   src/slave/http.cpp d1cf8a68fab9a2df44f6c753683ad37fd4b1a1f9 
>   src/slave/slave.cpp 50b57819b55bdcdb9f49f20648199badc4d3f37b 
>   src/webui/master/static/js/controllers.js 
> 41a70a80442501a2bf7b217939dbe504662941d2 
> 
> Diff: https://reviews.apache.org/r/29071/diff/
> 
> 
> Testing
> ---
> 
> Ran locally.
> 
> 
> Thanks,
> 
> David Robinson
> 
>

Re: Review Request 28698: [WIP] Modified Resources to account for reservation type.

[Michael Park]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28698/
---

(Updated Dec. 16, 2014, 11:04 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, and 
Vinod Kone.


Repository: mesos-git


Description (updated)
---

Modified Resources to account for reservation type.


Diffs (updated)
-

  include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc 
  src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92 

Diff: https://reviews.apache.org/r/28698/diff/


Testing
---


Thanks,

Michael Park

Re: Review Request 28697: [WIP] Add ReservationType and ReservationInfo for dynamic reservations.

[Michael Park]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28697/
---

(Updated Dec. 16, 2014, 11:01 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, and 
Vinod Kone.


Bugs: MESOS-2137
https://issues.apache.org/jira/browse/MESOS-2137


Repository: mesos-git


Description (updated)
---

Adding new protobuf messages necessary to support dynamic reservations.


Diffs (updated)
-

  include/mesos/mesos.proto 540071db64961466eb75c779b3ea6863f4594437 

Diff: https://reviews.apache.org/r/28697/diff/


Testing
---


Thanks,

Michael Park

Re: Review Request 29121: NetworkIsolatorFilter: Make error message explicit instead of skipping sliently.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29121/#review65253
---


Patch looks great!

Reviews applied: [29121]

All tests passed.

- Mesos ReviewBot


On Dec. 16, 2014, 10:14 p.m., Chi Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29121/
> ---
> 
> (Updated Dec. 16, 2014, 10:14 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jie Yu, and Timothy Chen.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> NetworkIsolatorFilter: Make error message explicit instead of skipping 
> sliently.
> 
> 
> Diffs
> -
> 
>   src/tests/environment.cpp bc2fc90fe956ac3c075cb7c850eb9481820380fd 
> 
> Diff: https://reviews.apache.org/r/29121/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 28815: Converted an initial DRF integration test to a unit test.

[Jie Yu]

Ben,

Thanks for reporting. We will fix that.

BTW: when can we start to assume 4.6?

- Jie

On Tue, Dec 16, 2014 at 2:14 PM, Benjamin Hindman  wrote:
>
>This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28815/
>src/tests/hierarchical_allocator_tests.cpp
>  
> (Diff
> revision 2)
>
> 173
>
>   initialize({"role1", "role2"});
>
>   This breaks on my Linux box with gcc 4.4.7.
>
> $ uname -a
>
> Linux centos6-build 2.6.32-431.23.3.el6.x86_64 #1 SMP Thu Jul 31 17:20:51 UTC 
> 2014 x86_64 x86_64 x86_64 GNU/Linux
>
> $ gcc --version
>
> gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-11)
>
> Copyright (C) 2010 Free Software Foundation, Inc.
>
> This is free software; see the source for copying conditions.  There is NO
>
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>
>
> - Benjamin Hindman
>
> On December 11th, 2014, 2:48 a.m. UTC, Ben Mahler wrote:
>   Review request for mesos and Jie Yu.
> By Ben Mahler.
>
> *Updated Dec. 11, 2014, 2:48 a.m.*
>  *Repository: * mesos-git
> Description
>
> Now that the allocator can be unit tested (r/28814), this adds the unit test 
> base calss, and moves over the DRF test to a unit test.
>
>   Testing
>
> make check
>
>   Diffs
>
>- src/Makefile.am (86161fe7a8bdd86958d24adb74d434cd92d7dfb8)
>- src/tests/allocator_tests.cpp
>(8626362e5d45304a4800a807d5171178e457ff53)
>- src/tests/hierarchical_allocator_tests.cpp (PRE-CREATION)
>
> View Diff 
>

Re: Review Request 29018: Added abstraction for resources transformation.

[Jie Yu]

> On Dec. 16, 2014, 9:42 p.m., Ben Mahler wrote:
> > include/mesos/resources.hpp, lines 190-198
> > 
> >
> > Maybe this could be a bit more succinct:
> > 
> > ```
> > // This is an abstraction for describing a transformation that can
> > // be applied to Resources. Transformations cannot not alter the
> > // quantity, or the static role of the resources.
> > //
> > // TODO(jieyu): Enforce the transformation invariants fully!
> > class Transformation
> > {
> > public:
> >   ...
> >   
> >   // Returns the result of the transformation, applied to 'resources'.
> >   // Returns an Error if the transformation cannot be applied.
> >   Try operator () (const Resources& resources) const;
> > };
> > ```

Done.


> On Dec. 16, 2014, 9:42 p.m., Ben Mahler wrote:
> > include/mesos/resources.hpp, lines 210-212
> > 
> >
> > Maybe mention the all-or-nothing nature:
> > 
> > ```
> > // Represents a sequence of transformations, the transformations
> > // are applied in an all-or-nothing manner.
> > class CompositeTransformation : public Transformation
> > {
> > };
> > ```

Done.


> On Dec. 16, 2014, 9:42 p.m., Ben Mahler wrote:
> > include/mesos/resources.hpp, line 236
> > 
> >
> > You need an include for vector now?

DOne.


> On Dec. 16, 2014, 9:42 p.m., Ben Mahler wrote:
> > src/common/resources.cpp, line 33
> > 
> >
> > Unused?

Removed.


> On Dec. 16, 2014, 9:42 p.m., Ben Mahler wrote:
> > src/common/resources.cpp, line 860
> > 
> >
> > "transformations"?

Fixed.


> On Dec. 16, 2014, 9:42 p.m., Ben Mahler wrote:
> > src/common/resources.cpp, lines 864-885
> > 
> >
> > Thanks, could this be:
> > 
> > ```
> > Try applied = apply(resources);
> > 
> > if (applied.isSome()) {
> >   // Additional checks.
> > }
> > 
> > return applied;
> > ```

Done.


> On Dec. 16, 2014, 9:42 p.m., Ben Mahler wrote:
> > src/common/resources.cpp, line 895
> > 
> >
> > newline here?

Done.


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29018/#review65241
---


On Dec. 16, 2014, 9:10 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29018/
> ---
> 
> (Updated Dec. 16, 2014, 9:10 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added abstraction for resources transformation. Split from 
> https://reviews.apache.org/r/28720/.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc 
>   src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92 
> 
> Diff: https://reviews.apache.org/r/29018/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 29071: added webui_log option

[David Robinson]

> On Dec. 16, 2014, 1:44 a.m., Mesos ReviewBot wrote:
> > Bad patch!
> > 
> > Reviews applied: [29071]
> > 
> > Failed command: ./support/apply-review.sh -n -r 29071
> > 
> > Error:
> >  2014-12-16 01:44:48 URL:https://reviews.apache.org/r/29071/diff/raw/ 
> > [5226/5226] -> "29071.patch" [1]
> > error: master/flags.hpp: does not exist in index
> > error: master/http.cpp: does not exist in index
> > error: master/master.cpp: does not exist in index
> > error: slave/flags.hpp: does not exist in index
> > error: slave/http.cpp: does not exist in index
> > error: slave/slave.cpp: does not exist in index
> > error: webui/master/static/js/controllers.js: does not exist in index
> > Failed to apply patch

Guess you don't like 'noprefix = true'?!


- David


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29071/#review65165
---


On Dec. 16, 2014, 10:37 p.m., David Robinson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29071/
> ---
> 
> (Updated Dec. 16, 2014, 10:37 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-2193
> https://issues.apache.org/jira/browse/MESOS-2193
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> added webui_log option
> 
> 
> Diffs
> -
> 
>   src/master/flags.hpp 1cea50c02f3ad7de1e1ae91d65d1accdb9af7b03 
>   src/master/http.cpp 46890bed05d7c4b63e1f7be5bb35217173e0ade8 
>   src/master/master.cpp 0f55a5cc2d6845cbaace718a48f771d80aad0e6e 
>   src/slave/flags.hpp 670997dc3a702cd5edf33f2e5824c5e4dfe4ecef 
>   src/slave/http.cpp d1cf8a68fab9a2df44f6c753683ad37fd4b1a1f9 
>   src/slave/slave.cpp 50b57819b55bdcdb9f49f20648199badc4d3f37b 
>   src/webui/master/static/js/controllers.js 
> 41a70a80442501a2bf7b217939dbe504662941d2 
> 
> Diff: https://reviews.apache.org/r/29071/diff/
> 
> 
> Testing
> ---
> 
> Ran locally.
> 
> 
> Thanks,
> 
> David Robinson
> 
>

Re: Review Request 29071: added webui_log option

[David Robinson]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29071/
---

(Updated Dec. 16, 2014, 10:37 p.m.)


Review request for mesos.


Changes
---

Dominic's feedback.


Bugs: MESOS-2193
https://issues.apache.org/jira/browse/MESOS-2193


Repository: mesos-git


Description
---

added webui_log option


Diffs (updated)
-

  src/master/flags.hpp 1cea50c02f3ad7de1e1ae91d65d1accdb9af7b03 
  src/master/http.cpp 46890bed05d7c4b63e1f7be5bb35217173e0ade8 
  src/master/master.cpp 0f55a5cc2d6845cbaace718a48f771d80aad0e6e 
  src/slave/flags.hpp 670997dc3a702cd5edf33f2e5824c5e4dfe4ecef 
  src/slave/http.cpp d1cf8a68fab9a2df44f6c753683ad37fd4b1a1f9 
  src/slave/slave.cpp 50b57819b55bdcdb9f49f20648199badc4d3f37b 
  src/webui/master/static/js/controllers.js 
41a70a80442501a2bf7b217939dbe504662941d2 

Diff: https://reviews.apache.org/r/29071/diff/


Testing
---

Ran locally.


Thanks,

David Robinson

Re: Review Request 29071: added webui_log option

[David Robinson]

> On Dec. 16, 2014, 12:58 a.m., Dominic Hamon wrote:
> > src/slave/slave.cpp, line 459
> > 
> >
> > might want to check that the file exists, as per the log_dir stanza 
> > above.

The stanza above just returns a filename (created from log_dir, log level etc), 
it doesn't check that the file exists. FilesProcess::attach is where file 
existance check occurs:


Future FilesProcess::attach(const string& path, const string& name)
{
  Result result = os::realpath(path);

  if (!result.isSome()) {
return Failure(
"Failed to get realpath of '" + path + "': " +
(result.isError()
 ? result.error()
 : "No such file or directory"));
  }

  // Make sure we have permissions to read the file/dir.
  ...


> On Dec. 16, 2014, 12:58 a.m., Dominic Hamon wrote:
> > src/slave/slave.cpp, line 446
> > 
> >
> > so webui_log overrides log_dir? should check if both are set and error.

Done.


> On Dec. 16, 2014, 12:58 a.m., Dominic Hamon wrote:
> > src/webui/master/static/js/controllers.js, line 405
> > 
> >
> > or 'webui_log'

Done.


- David


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29071/#review65162
---


On Dec. 16, 2014, 12:51 a.m., David Robinson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29071/
> ---
> 
> (Updated Dec. 16, 2014, 12:51 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-2193
> https://issues.apache.org/jira/browse/MESOS-2193
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> added webui_log option
> 
> 
> Diffs
> -
> 
>   src/master/flags.hpp 1cea50c02f3ad7de1e1ae91d65d1accdb9af7b03 
>   src/master/http.cpp 46890bed05d7c4b63e1f7be5bb35217173e0ade8 
>   src/master/master.cpp 0f55a5cc2d6845cbaace718a48f771d80aad0e6e 
>   src/slave/flags.hpp 670997dc3a702cd5edf33f2e5824c5e4dfe4ecef 
>   src/slave/http.cpp d1cf8a68fab9a2df44f6c753683ad37fd4b1a1f9 
>   src/slave/slave.cpp 50b57819b55bdcdb9f49f20648199badc4d3f37b 
>   src/webui/master/static/js/controllers.js 
> 41a70a80442501a2bf7b217939dbe504662941d2 
> 
> Diff: https://reviews.apache.org/r/29071/diff/
> 
> 
> Testing
> ---
> 
> Ran locally.
> 
> 
> Thanks,
> 
> David Robinson
> 
>

Re: Review Request 28815: Converted an initial DRF integration test to a unit test.

[Benjamin Hindman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28815/#review65250
---



src/tests/hierarchical_allocator_tests.cpp


This breaks on my Linux box with gcc 4.4.7.

$ uname -a
Linux centos6-build 2.6.32-431.23.3.el6.x86_64 #1 SMP Thu Jul 31 17:20:51 
UTC 2014 x86_64 x86_64 x86_64 GNU/Linux

$ gcc --version
gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-11)
Copyright (C) 2010 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.


- Benjamin Hindman


On Dec. 11, 2014, 2:48 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28815/
> ---
> 
> (Updated Dec. 11, 2014, 2:48 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Now that the allocator can be unit tested (r/28814), this adds the unit test 
> base calss, and moves over the DRF test to a unit test.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8 
>   src/tests/allocator_tests.cpp 8626362e5d45304a4800a807d5171178e457ff53 
>   src/tests/hierarchical_allocator_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/28815/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Review Request 29121: NetworkIsolatorFilter: Make error message explicit instead of skipping sliently.

[Chi Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29121/
---

Review request for mesos, Ben Mahler, Jie Yu, and Timothy Chen.


Repository: mesos-git


Description
---

NetworkIsolatorFilter: Make error message explicit instead of skipping sliently.


Diffs
-

  src/tests/environment.cpp bc2fc90fe956ac3c075cb7c850eb9481820380fd 

Diff: https://reviews.apache.org/r/29121/diff/


Testing
---


Thanks,

Chi Zhang

Re: Review Request 29083: Updated Sorter to allow transforming allocated resources.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29083/#review65248
---



src/master/drf_sorter.cpp


Do you need to update total resources as well?


- Jie Yu


On Dec. 16, 2014, 6:28 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29083/
> ---
> 
> (Updated Dec. 16, 2014, 6:28 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-2099
> https://issues.apache.org/jira/browse/MESOS-2099
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> In order for frameworks to perform resource transformations on their 
> allocations, the allocator must be able to update the allocation in the 
> sorter objects.
> 
> Note that the sorters cannot simply take a Transformation, as done in 
> r/29084/. This is because sorters may not store the complete allocation for a 
> client (e.g. role sorter doesn't track reserved resources).
> 
> This means that taking a Transformation is problematic for the role sorter in 
> the allocator.
> 
> 
> Diffs
> -
> 
>   src/master/drf_sorter.hpp c47b56afdb8f9f3ff0793f0d10a4e4c656e6a212 
>   src/master/drf_sorter.cpp 0ad6c52759e1ceec4b43f99d8fa1e75d7e2e1c31 
>   src/master/sorter.hpp 0150f90b9aab2ecb4edd07dca7f0d58997e422aa 
>   src/tests/sorter_tests.cpp 0516ab573d9f4f2af249978e15ebf52c2afb5359 
> 
> Diff: https://reviews.apache.org/r/29083/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 28720: Adjusted the calculation of unused resources in _launchTasks by considering persistent disk acquisition.

[Ben Mahler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28720/#review65237
---


First pass, only looked at the disk transformation for now, which would be 
great to pull out into a separate patch with tests! :)

I'm still thinking about the idempotent nature of transformations more 
generally, but we can punt on it for now until we have more of them to deal 
with. :)


src/master/master.cpp


Why is a disk with no persistence info and no volume considered valid? What 
does that mean?



src/master/master.cpp


There is a bit of non-local reasoning needed here, since this assumes that 
has_volume() implies has_persistence(), otherwise we'd be missing 
transformations. That is, I need to know about the semantics of the validation 
code here.

It would be nice if has_disk() was the only signal for transformation, 
because that allows the validation to change (allow volumes w/o persistence 
ids) while keeping this code correct:

```
if (resource.has_disk()) {
  transformation.add(DiskTransformation(resource));
}
```

But since we have the very specific `AcquirePersistentDisk` currently, 
maybe we just need to show our assumptions here with a NOTE and a CHECK:

```
if (resource.has_disk()) {
  // NOTE: Validation currently ensures that disk metadata
  // is only provided when persistence is desired.
  CHECK(resource.disk().has_persistence());
  transformation.add(AcquirePersistentDisk(resource));
}
```

Thoughts?



src/master/master.cpp


How about we rename totalResources to offeredResources, then we can call 
this transformedOfferedResources?

All these "adjusted" namings make me think that we should have called it 
Adjustment instead of Transformation ;)



include/mesos/resources.hpp


Any chance you could split this one out into a separate patch with some 
tests? We can land it quicker that way :)



src/common/resources.cpp


How about splitting these checks?

```
CHECK(disk.has_disk());
CHECK(disk.disk().has_persistence());
```



src/common/resources.cpp


Duplicate persistence IDs are allowed across roles? Is that handled 
everywhere else in the code?



src/common/resources.cpp


Can we include the ID?



src/common/resources.cpp


How about `stripped` or `strippedDisk`? Might be a bit easier to read.



src/common/resources.cpp


How about:

```
return Error("Insufficient disk resources");
```


- Ben Mahler


On Dec. 16, 2014, 9:11 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28720/
> ---
> 
> (Updated Dec. 16, 2014, 9:11 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Introduced an abstraction for mutating resources. Acquiring persistent disk 
> is one type of resources mutation.
> 
> Infer persistent disk acquisitions from resources and check resource usage 
> against adjusted total resources.
> 
> Adjusted the calculation of unused resources in _launchTasks by considering 
> persistent disk acquisition.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc 
>   src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92 
>   src/master/master.cpp 0f55a5cc2d6845cbaace718a48f771d80aad0e6e 
>   src/tests/resource_offers_tests.cpp 
> e13b6c5460d9e6729843c40bed9e4d4e3f76d5d3 
> 
> Diff: https://reviews.apache.org/r/28720/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 29089: Fixed the DRFSorter share calculation to not assume flattened resources.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29089/#review65246
---

Ship it!



src/tests/sorter_tests.cpp


Can you move this line down and include disk1 and disk2 so that you test 
the calculation of hte 'total' part as well.

```
sorter.add(Resources::parse("...").get() + disk1 + disk2);
```



src/tests/sorter_tests.cpp


Consider using ```createDiskInfo``` in src/test/mesos.hpp?


- Jie Yu


On Dec. 16, 2014, 6:28 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29089/
> ---
> 
> (Updated Dec. 16, 2014, 6:28 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-2099
> https://issues.apache.org/jira/browse/MESOS-2099
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> We broke the share calculation as soon as we allowed resources to be 
> non-flattened.
> 
> 
> Diffs
> -
> 
>   src/master/drf_sorter.cpp 0ad6c52759e1ceec4b43f99d8fa1e75d7e2e1c31 
>   src/tests/sorter_tests.cpp 0516ab573d9f4f2af249978e15ebf52c2afb5359 
> 
> Diff: https://reviews.apache.org/r/29089/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 29018: Added abstraction for resources transformation.

[Ben Mahler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29018/#review65241
---

Ship it!



include/mesos/resources.hpp


Maybe this could be a bit more succinct:

```
// This is an abstraction for describing a transformation that can
// be applied to Resources. Transformations cannot not alter the
// quantity, or the static role of the resources.
//
// TODO(jieyu): Enforce the transformation invariants fully!
class Transformation
{
public:
  ...
  
  // Returns the result of the transformation, applied to 'resources'.
  // Returns an Error if the transformation cannot be applied.
  Try operator () (const Resources& resources) const;
};
```



include/mesos/resources.hpp


Maybe mention the all-or-nothing nature:

```
// Represents a sequence of transformations, the transformations
// are applied in an all-or-nothing manner.
class CompositeTransformation : public Transformation
{
};
```



include/mesos/resources.hpp


You need an include for vector now?



src/common/resources.cpp


Unused?



src/common/resources.cpp


"transformations"?



src/common/resources.cpp


Thanks, could this be:

```
Try applied = apply(resources);

if (applied.isSome()) {
  // Additional checks.
}

return applied;
```



src/common/resources.cpp


newline here?


- Ben Mahler


On Dec. 16, 2014, 9:10 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29018/
> ---
> 
> (Updated Dec. 16, 2014, 9:10 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added abstraction for resources transformation. Split from 
> https://reviews.apache.org/r/28720/.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc 
>   src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92 
> 
> Diff: https://reviews.apache.org/r/29018/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 29018: Added abstraction for resources transformation.

[Ben Mahler]

> On Dec. 15, 2014, 9:29 p.m., Ben Mahler wrote:
> > include/mesos/resources.hpp, lines 211-214
> > 
> >
> > To keep consistent with the rest of the code base, couldn't you have 
> > done the following?
> > 
> > ```
> > template 
> > void add(const T& t)
> > {
> >   transformations.push_back(new T(t));
> > }
> > ```
> > 
> > With unique_ptr, we're requiring that ownership be transferred if the 
> > caller is holding the transformation, which seems a bit odd for our library 
> > to impose on the caller:
> > 
> > ```
> > // The caller cannot keep ownership of t.
> > std::unique_ptr t(new DiskTransformation(...));
> > composite.add(std::move(t));
> > 
> > // Ok.
> > composite.add(std::unique_ptr(new 
> > DiskTransformation(...));
> > ```
> > 
> > Could we keep the const argument semantics for now for consistency? 
> > Then the caller doesn't have to use 'new' as well. Thoughts?
> 
> Dominic Hamon wrote:
> That's exactly the point of std::unique_ptr - a library is finally able 
> to inform the caller that they're taking responsibility for the object and 
> the memory associated with it. I don't see that as odd.
> 
> Can you describe a use case where a caller might want to retain a 
> reference to the transformation?
> 
> Jie Yu wrote:
> OK, since we haven't finalized the unique_ptr usage in the style guide, 
> I'll use your suggestion and add a TODO here.

Thanks Jie and Dominic! This could very well be a great place to leverage 
unique_ptr, but as jie said let's take this example to the thread :)

The point that we'll want to consider there is that function calls with value 
semantics are generally easier to reason about:

```cpp
CompositeTransformation composite;
composite.add(DiskTransformation(disk));

FooBarTransformation transformation; ...;
composite.add(transformation);
```

The caller here only has to reason about values. The callee is responsible for 
taking a copy if it's needed.

Here we have to reason about memory ownership:

```cpp
CompositeTransformation composite;
composite.add(unique_ptr(new DiskTransformation(disk)));

unique_ptr transformation(new FooBarTransformation()); ...;
composite.add(std::move(transformation));

// Also consider:
transformation->...; // Yikes! As far as I can tell, the compiler won't warn 
about this?
```

Another point to bring up in the summary from that thread (as it's getting a 
bit long), is whether compilers warn about using moved objects, doesn't seem 
like it?
http://stackoverflow.com/questions/14552690/warn-if-accessing-moved-object-in-c11


- Ben


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29018/#review65121
---


On Dec. 16, 2014, 9:10 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29018/
> ---
> 
> (Updated Dec. 16, 2014, 9:10 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added abstraction for resources transformation. Split from 
> https://reviews.apache.org/r/28720/.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc 
>   src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92 
> 
> Diff: https://reviews.apache.org/r/29018/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 28698: [WIP] Modified Resources to account for reservation type.

[Michael Park]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28698/
---

(Updated Dec. 16, 2014, 9:33 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, and 
Vinod Kone.


Summary (updated)
-

[WIP] Modified Resources to account for reservation type.


Repository: mesos-git


Description
---

Modified Resources arithmetic logic to account for reservation type. Work in 
progress.


Diffs
-

  include/mesos/resources.hpp 10777a62492e4ad764e0f75c064694e054d1 
  src/common/resources.cpp 535a0eab6377b9ae63c960cdb05978647f667d5e 

Diff: https://reviews.apache.org/r/28698/diff/


Testing
---


Thanks,

Michael Park

Re: Review Request 28697: [WIP] Add ReservationType and ReservationInfo for dynamic reservations.

[Michael Park]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28697/
---

(Updated Dec. 16, 2014, 9:33 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, and 
Vinod Kone.


Summary (updated)
-

[WIP] Add ReservationType and ReservationInfo for dynamic reservations.


Bugs: MESOS-2137
https://issues.apache.org/jira/browse/MESOS-2137


Repository: mesos-git


Description
---

Adding new protobuf messages necessary to support dynamic reservations. Work in 
progress.


Diffs
-

  include/mesos/mesos.proto e0b6375579ca7d3af03e4b59044810b8b7d2df86 

Diff: https://reviews.apache.org/r/28697/diff/


Testing
---


Thanks,

Michael Park

Re: Review Request 28720: Adjusted the calculation of unused resources in _launchTasks by considering persistent disk acquisition.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28720/
---

(Updated Dec. 16, 2014, 9:11 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Review comments.


Bugs: MESOS-2030
https://issues.apache.org/jira/browse/MESOS-2030


Repository: mesos-git


Description
---

Introduced an abstraction for mutating resources. Acquiring persistent disk is 
one type of resources mutation.

Infer persistent disk acquisitions from resources and check resource usage 
against adjusted total resources.

Adjusted the calculation of unused resources in _launchTasks by considering 
persistent disk acquisition.


Diffs (updated)
-

  include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc 
  src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92 
  src/master/master.cpp 0f55a5cc2d6845cbaace718a48f771d80aad0e6e 
  src/tests/resource_offers_tests.cpp e13b6c5460d9e6729843c40bed9e4d4e3f76d5d3 

Diff: https://reviews.apache.org/r/28720/diff/


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 29018: Added abstraction for resources transformation.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29018/
---

(Updated Dec. 16, 2014, 9:10 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Review comments.


Bugs: MESOS-2030
https://issues.apache.org/jira/browse/MESOS-2030


Repository: mesos-git


Description
---

Added abstraction for resources transformation. Split from 
https://reviews.apache.org/r/28720/.


Diffs (updated)
-

  include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc 
  src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92 

Diff: https://reviews.apache.org/r/29018/diff/


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 28966: PortMappingIsolator: added a metric to expose the number of TC IP filters on eth0.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28966/#review65232
---


Patch looks great!

Reviews applied: [28966]

All tests passed.

- Mesos ReviewBot


On Dec. 16, 2014, 6:50 p.m., Chi Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28966/
> ---
> 
> (Updated Dec. 16, 2014, 6:50 p.m.)
> 
> 
> Review request for mesos, Dominic Hamon, Ian Downes, and Jie Yu.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> also added a test.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336 
>   src/tests/port_mapping_tests.cpp eb82993 
> 
> Diff: https://reviews.apache.org/r/28966/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 29018: Added abstraction for resources transformation.

[Jie Yu]

> On Dec. 15, 2014, 9:29 p.m., Ben Mahler wrote:
> > include/mesos/resources.hpp, lines 191-202
> > 
> >
> > Great, do we want to specify what a valid transformation is? In 
> > particular, it seems like the invariant of a Transformation is that the 
> > amount of a resource ("cpus", "mem", "disk", "ports", ...) remains 
> > unchanged and only the "metadata" of the resources can be manipulated. 
> > Let's document that this is what a Transformation is?
> > 
> > For invariants, maybe just a NOTE for now.. but I'd love to have 
> > invariants enforced in the allocator (if not provided by Transformation the 
> > allocator will have to manually check them).
> > 
> > One suggestion is, similary to the Registrar's Operation, we can keep a 
> > top level `apply` or function operator as non-virtual, which performs the 
> > transformation and validates any of the invariants (cpus, mem, disk, ports 
> > remain the same) and rely on a virtual method to perform the actual 
> > transformation? That way, Transformation can enforce the invariants, and a 
> > CHECK fails or an Error results if they are broken.

Added a non virtual operator which calls the virtual apply. Enforced the 
invariants in the operator.


> On Dec. 15, 2014, 9:29 p.m., Ben Mahler wrote:
> > include/mesos/resources.hpp, lines 211-214
> > 
> >
> > To keep consistent with the rest of the code base, couldn't you have 
> > done the following?
> > 
> > ```
> > template 
> > void add(const T& t)
> > {
> >   transformations.push_back(new T(t));
> > }
> > ```
> > 
> > With unique_ptr, we're requiring that ownership be transferred if the 
> > caller is holding the transformation, which seems a bit odd for our library 
> > to impose on the caller:
> > 
> > ```
> > // The caller cannot keep ownership of t.
> > std::unique_ptr t(new DiskTransformation(...));
> > composite.add(std::move(t));
> > 
> > // Ok.
> > composite.add(std::unique_ptr(new 
> > DiskTransformation(...));
> > ```
> > 
> > Could we keep the const argument semantics for now for consistency? 
> > Then the caller doesn't have to use 'new' as well. Thoughts?
> 
> Dominic Hamon wrote:
> That's exactly the point of std::unique_ptr - a library is finally able 
> to inform the caller that they're taking responsibility for the object and 
> the memory associated with it. I don't see that as odd.
> 
> Can you describe a use case where a caller might want to retain a 
> reference to the transformation?

OK, since we haven't finalized the unique_ptr usage in the style guide, I'll 
use your suggestion and add a TODO here.


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29018/#review65121
---


On Dec. 13, 2014, 5:57 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29018/
> ---
> 
> (Updated Dec. 13, 2014, 5:57 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added abstraction for resources transformation. Split from 
> https://reviews.apache.org/r/28720/.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc 
>   src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92 
> 
> Diff: https://reviews.apache.org/r/29018/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 28966: PortMappingIsolator: added a metric to expose the number of TC IP filters on eth0.

[Chi Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28966/
---

(Updated Dec. 16, 2014, 6:50 p.m.)


Review request for mesos, Dominic Hamon, Ian Downes, and Jie Yu.


Changes
---

addressed comments.


Repository: mesos-git


Description
---

also added a test.


Diffs (updated)
-

  src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352 
  src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336 
  src/tests/port_mapping_tests.cpp eb82993 

Diff: https://reviews.apache.org/r/28966/diff/


Testing
---

make check


Thanks,

Chi Zhang

Re: Review Request 28966: PortMappingIsolator: added a metric to expose the number of TC IP filters on eth0.

[Chi Zhang]

> On Dec. 16, 2014, 6:21 p.m., Jie Yu wrote:
> > What's the purpose of exposing this metric?

I think it's not a bad thing to have a way to monitor the number of tc filters 
we have put on the system, even just for our internal knowledge.


- Chi


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28966/#review65218
---


On Dec. 16, 2014, 6:50 p.m., Chi Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28966/
> ---
> 
> (Updated Dec. 16, 2014, 6:50 p.m.)
> 
> 
> Review request for mesos, Dominic Hamon, Ian Downes, and Jie Yu.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> also added a test.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336 
>   src/tests/port_mapping_tests.cpp eb82993 
> 
> Diff: https://reviews.apache.org/r/28966/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 28966: PortMappingIsolator: added a metric to expose the number of TC IP filters on eth0.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28966/#review65218
---


What's the purpose of exposing this metric?

- Jie Yu


On Dec. 16, 2014, 1:54 a.m., Chi Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28966/
> ---
> 
> (Updated Dec. 16, 2014, 1:54 a.m.)
> 
> 
> Review request for mesos, Dominic Hamon, Ian Downes, and Jie Yu.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> also added a test.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336 
>   src/tests/port_mapping_tests.cpp eb82993 
> 
> Diff: https://reviews.apache.org/r/28966/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 29088: Removed an unnecessary helper in the sorter tests.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29088/#review65217
---

Ship it!



src/tests/sorter_tests.cpp


Curious if the following will compile or not:

EXPECT_EQ(sorter.sort(), {"a", "b"});


- Jie Yu


On Dec. 16, 2014, 6:28 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29088/
> ---
> 
> (Updated Dec. 16, 2014, 6:28 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> This helper is no longer necessary, and the EXPECT statments inside a method 
> loses line number information when the tests fail.
> 
> 
> Diffs
> -
> 
>   src/tests/sorter_tests.cpp 0516ab573d9f4f2af249978e15ebf52c2afb5359 
> 
> Diff: https://reviews.apache.org/r/29088/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 29069: Added an integration test for testing the persistence primitives.

[Dominic Hamon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29069/#review65214
---



src/tests/persistent_volume_tests.cpp


s/size/sizeMb/ and lose the comment



src/tests/persistent_volume_tests.cpp


EXPECT_FALSE(offers1.get().empty());



src/tests/persistent_volume_tests.cpp


it's odd to me that this path isn't absolute. is this just a test thing or 
is it going to be like this for end users?



src/tests/persistent_volume_tests.cpp


for my edification: what does this loop do that the AWAIT_READY doesn't?



src/tests/persistent_volume_tests.cpp


EXPECT_FALSE(offers2.get().empty());



src/tests/persistent_volume_tests.cpp


this is called in the base class TearDown.


- Dominic Hamon


On Dec. 15, 2014, 5:05 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29069/
> ---
> 
> (Updated Dec. 15, 2014, 5:05 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added an integration test for testing the persistence primitives.
> 
> This tests the basic persistence feature. See comments in the code.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 6f132b5a66e117c2a907763217bfafe1fce1b7a0 
>   src/tests/persistent_volume_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29069/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 28966: PortMappingIsolator: added a metric to expose the number of TC IP filters on eth0.

[Dominic Hamon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28966/#review65213
---



src/tests/port_mapping_tests.cpp


this can be a const method as you don't change any members.


- Dominic Hamon


On Dec. 15, 2014, 5:54 p.m., Chi Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28966/
> ---
> 
> (Updated Dec. 15, 2014, 5:54 p.m.)
> 
> 
> Review request for mesos, Dominic Hamon, Ian Downes, and Jie Yu.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> also added a test.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/network/port_mapping.hpp f1e2352 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 2d81336 
>   src/tests/port_mapping_tests.cpp eb82993 
> 
> Diff: https://reviews.apache.org/r/28966/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 29018: Added abstraction for resources transformation.

[Dominic Hamon]

> On Dec. 15, 2014, 1:47 p.m., Dominic Hamon wrote:
> > include/mesos/resources.hpp, line 213
> > 
> >
> > not quite. emplace_back with a unique_ptr works because unique_ptr 
> > doesn't have a copy constructor. however, as it turns out, both 
> > push_back(std::move(..)) and emplace_back(..) may leak if the vector grow 
> > fails and an exception is thrown.
> > 
> > ideally, then, this should be push_back(make_unique<>(..))
> > 
> > but we don't have make_unique ;)
> 
> Michael Park wrote:
> Hey Dominic, was this supposed to be a reply to my comment?

yes, sorry. i forgot that it doesn't thread correctly from the diff.


- Dominic


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29018/#review65125
---


On Dec. 12, 2014, 9:57 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29018/
> ---
> 
> (Updated Dec. 12, 2014, 9:57 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2030
> https://issues.apache.org/jira/browse/MESOS-2030
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added abstraction for resources transformation. Split from 
> https://reviews.apache.org/r/28720/.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc 
>   src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92 
> 
> Diff: https://reviews.apache.org/r/29018/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 29084: Updated Allocator to allow transforming allocated resources.

[Dominic Hamon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29084/#review65211
---



src/master/allocator.hpp


if this is a Shared, why are you passing by reference? this means the 
reference count won't be updated and the lifetime of the Shared may end before 
this method returns.

Unless I'm missing something.


- Dominic Hamon


On Dec. 15, 2014, 10:28 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29084/
> ---
> 
> (Updated Dec. 15, 2014, 10:28 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-2099
> https://issues.apache.org/jira/browse/MESOS-2099
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> The idea here is that we want the framework to be able to transform the 
> resources allocated to it, and these Transformations need to be reflected in 
> the allocator.
> 
> Note that Transformations should not be updating the quantity, or the static 
> roles of the resources. I've left this as a TODO and hopefully we can enforce 
> this in the Transformation abstraction itself.
> 
> 
> Diffs
> -
> 
>   src/master/allocator.hpp f4068aad42a7df38544718dcd171ff239dc1e39a 
>   src/master/hierarchical_allocator_process.hpp 
> 95fa520a3947474472cd36987c2342e1bfb51cfe 
>   src/tests/hierarchical_allocator_tests.cpp 
> c43d3520a06dda6b48e74edfd692b3431fb639d2 
>   src/tests/mesos.hpp bb24222c20cb5458b5c627d2001fc3cb1e542cce 
> 
> Diff: https://reviews.apache.org/r/29084/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 29083: Updated Sorter to allow transforming allocated resources.

[Dominic Hamon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29083/#review65210
---



src/master/drf_sorter.cpp


const_iterator


- Dominic Hamon


On Dec. 15, 2014, 10:28 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29083/
> ---
> 
> (Updated Dec. 15, 2014, 10:28 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-2099
> https://issues.apache.org/jira/browse/MESOS-2099
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> In order for frameworks to perform resource transformations on their 
> allocations, the allocator must be able to update the allocation in the 
> sorter objects.
> 
> Note that the sorters cannot simply take a Transformation, as done in 
> r/29084/. This is because sorters may not store the complete allocation for a 
> client (e.g. role sorter doesn't track reserved resources).
> 
> This means that taking a Transformation is problematic for the role sorter in 
> the allocator.
> 
> 
> Diffs
> -
> 
>   src/master/drf_sorter.hpp c47b56afdb8f9f3ff0793f0d10a4e4c656e6a212 
>   src/master/drf_sorter.cpp 0ad6c52759e1ceec4b43f99d8fa1e75d7e2e1c31 
>   src/master/sorter.hpp 0150f90b9aab2ecb4edd07dca7f0d58997e422aa 
>   src/tests/sorter_tests.cpp 0516ab573d9f4f2af249978e15ebf52c2afb5359 
> 
> Diff: https://reviews.apache.org/r/29083/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>