Re: Review Request 41791: Updated allocation slack when dynamic reserve new resources (1/3).

[Guangya Liu]

> On 一月 20, 2016, 10:54 p.m., Joseph Wu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 662
> > 
> >
> > Have you considered modifying `Resources::apply` to "create" allocation 
> > slack upon a reservation?  (And to "destroy" allocation slack upon 
> > unreserve?)

I do not want to update `Resources::apply` as it will be called when `reserve` 
and `unreserve` resources, when `reserve` resources, we can simply add some 
`allocation slack`; when `unreserve` resources, life become complex, we cannot 
simply decrease the `allocation slack` as if the `allocation slack` decreased, 
there will be new `unreserved` resources which can be send as offer and launch 
task because the `allocation slack` still running task and there might be 
resource confilict for this, that's why I have another following two patches 
handlding `unreserve` separately.

Take a case:
1) agent: cpus(r1):100;cpus(*){ALLOCATION_SLACK}:100
2) framework use up all cpus(*){ALLOCATION_SLACK}:100 resources.
3) unreserve 30 cpus.
4) The resources would become:
Total: cpus(*):30;cpus(r1):70;cpus(*){ALLOCATION_SLACK}:100 NOTE: We do not 
decrease the total allocation slack here.
Free: cpus(*):30;cpus(r1):70
Used: cpus(*){ALLOCATION_SLACK}:100
5) At this time, we cannot send cpus(*):30 out as offer because there are still 
30 over committed allocation slack in use.
6) recover 20 allocaiton slack, we can update the total resources as this:
Total: cpus(*):30;cpus(r1):70;cpus(*){ALLOCATION_SLACK}:80 NOTE: decrease the 
total allocation slack when recover resource but not when `unreserve` resources 
in such case.
Used: cpus(*){ALLOCATION_SLACK}:80
Free: cpus(*):30;cpus(r1):70
The allocate can send out offer with cpus(*):20 but not the whole 30 resources.
7) recover another 10 allocation slack.
Total: cpus(*):30;cpus(r1):70;cpus(*){ALLOCATION_SLACK}:70 NOTE: from now on, 
the allocatioin slack is same as reserved resources.
Used: cpus(*)20;cpus(*){ALLOCATION_SLACK}:70
Free: cpus(*):10;cpus(r1):70
The allocater can send out offer with cpus(*):10.


- Guangya


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


On 一月 20, 2016, 6:38 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41791/
> ---
> 
> (Updated 一月 20, 2016, 6:38 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, Joseph Wu, Klaus Ma, and Jian Qiu.
> 
> 
> Bugs: MESOS-4145
> https://issues.apache.org/jira/browse/MESOS-4145
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update allocation slack resources if reserve some new
> stateless reserved resources. For such case, the allocation
> slack resources will be increased.
> 
> This patch updates both `updateAvailable` and `updateAllocation`
> when reserving some new resources.
> 
> There are three patches handling dynamic reservation and oo phase 1.
> 1) https://reviews.apache.org/r/41791 Reserve new resources via 
> `updateAllocation` and `updateAvailable`. (1/3).
> 2) https://reviews.apache.org/r/42113 Unreserve resources via 
> `updateAllocation` (2/3).
> 3) https://reviews.apache.org/r/42194 Unreserve resources via 
> `updateAvailable` (3/3).
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> d541bfa3f4190865c65d35c9d1ffdb8a3f194056 
>   src/tests/hierarchical_allocator_tests.cpp 
> e044f832c2c16e53e663c6ced5452649bb0dcb59 
> 
> Diff: https://reviews.apache.org/r/41791/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 42535: Made allocator's pause/resume idempotent.

[Joris Van Remoortere]

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

Ship it!


I see the test is in the works in a separate review.

- Joris Van Remoortere


On Jan. 20, 2016, 2:09 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42535/
> ---
> 
> (Updated Jan. 20, 2016, 2:09 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-4417
> https://issues.apache.org/jira/browse/MESOS-4417
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 101482156ffc5a4fe3cd60be222bfe609330ec3c 
>   src/master/allocator/mesos/hierarchical.cpp 
> e32ee4aa3ed9793bb5a99233e699e5cc2bdd796b 
> 
> Diff: https://reviews.apache.org/r/42535/diff/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 42506: Multiple Disk: Parameterized persistent volume tests on disk source.

[Joris Van Remoortere]

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

(Updated Jan. 21, 2016, 7:41 a.m.)


Review request for mesos, Jie Yu and Michael Park.


Changes
---

Rebased.


Repository: mesos


Description
---

This allows us to leverage the existing persistent volume tests while
also testing support for multiple disks.


Diffs (updated)
-

  src/tests/persistent_volume_tests.cpp 
7acf7ab29d64d891f3288f8042d267dcc82a32e9 

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


Testing
---

make check


Thanks,

Joris Van Remoortere

Re: Review Request 42505: Multiple Disk: Adjusted DiskInfo validation.

[Joris Van Remoortere]

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

(Updated Jan. 21, 2016, 7:40 a.m.)


Review request for mesos, Jie Yu and Michael Park.


Changes
---

Added support for `Mount` type.
Addressed issues.


Repository: mesos


Description (updated)
---

Multiple Disk: Adjusted DiskInfo validation.


Diffs (updated)
-

  src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
  src/master/validation.cpp 98360b690382ed1912a868ac93b058cb28003a12 
  src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 42504: Multiple Disk: Modified 'DiskInfo' stripping logic.

[Joris Van Remoortere]

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

(Updated Jan. 21, 2016, 7:39 a.m.)


Review request for mesos, Jie Yu and Michael Park.


Changes
---

Re-added volume stripping.
Added `createPersistentVolume` overload.


Repository: mesos


Description
---

Refactored persistent volume tests to use a helper to generate disk
resources.


Diffs (updated)
-

  src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
  src/common/resources_utils.cpp d64db54a175299bf7c32ecbeeb1ce9a7afb1c88a 
  src/tests/mesos.hpp 3d9ebc6c9dc3cd1be02dc3771fbd847386907fac 
  src/tests/persistent_volume_tests.cpp 
7acf7ab29d64d891f3288f8042d267dcc82a32e9 
  src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666 
  src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 

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


Testing
---


Thanks,

Joris Van Remoortere

Review Request 42593: Multiple Disk: Fixed invalid 'DiskResourcesTest's.

[Joris Van Remoortere]

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

Review request for mesos.


Repository: mesos


Description
---

These were invalid as a resource should not have a 'DiskInfo' if it
sets none of: persistence, volume, or source.


Diffs
-

  src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 42503: Multiple Disk: Added Resource arithmetic tests for type 'PATH'.

[Joris Van Remoortere]

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

(Updated Jan. 21, 2016, 7:39 a.m.)


Review request for mesos, Jie Yu and Michael Park.


Summary (updated)
-

Multiple Disk: Added Resource arithmetic tests for type 'PATH'.


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


Repository: mesos


Description (updated)
---

Multiple Disk: Added Resource arithmetic tests for type 'PATH'.


Diffs (updated)
-

  src/tests/mesos.hpp 3d9ebc6c9dc3cd1be02dc3771fbd847386907fac 
  src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 42474: Multiple Disk: Updated Slave initialize to create DiskInfo paths.

[Joris Van Remoortere]

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

(Updated Jan. 21, 2016, 7:38 a.m.)


Review request for mesos, Jie Yu and Michael Park.


Changes
---

Added support for `Mount` type, and validation against the `/proc/mounts` table.


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


Repository: mesos


Description (updated)
---

Create disk paths based on 'DiskInfo.Source' if the type is 'PATH'
and the directory does not yet exist.
Validate 'MOUNT' sources against the /proc/mounts table in linux.


Diffs (updated)
-

  src/slave/slave.cpp bd7fe03f8a8b07c6201db2f876f4f9cd7dc337cf 

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


Testing
---

make check


Thanks,

Joris Van Remoortere

Re: Review Request 42473: Multiple Disk: Updated filesystem isolators to use 'DiskInfo.Source'.

[Joris Van Remoortere]

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

(Updated Jan. 21, 2016, 7:37 a.m.)


Review request for mesos, Jie Yu and Michael Park.


Changes
---

Support for `Mount` type.


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


Repository: mesos


Description
---

Multiple Disk: Updated filesystem isolators to use 'DiskInfo.Source'.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
ace9e305c24a9841f1716c9bf40cd13b16ef0cec 
  src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
d914587eedc9c66bc14b4088ec211b7f0eea63ea 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 42472: Multiple Disk: Checkpoint persistent volume based on 'DiskInfo.Source'.

[Joris Van Remoortere]

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

(Updated Jan. 21, 2016, 7:37 a.m.)


Review request for mesos, Jie Yu and Michael Park.


Changes
---

Support for `Mount` type.


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


Repository: mesos


Description
---

We now create persistent volume directories based on 'DiskInfo.Source'
if it is present.


Diffs (updated)
-

  src/slave/slave.cpp bd7fe03f8a8b07c6201db2f876f4f9cd7dc337cf 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 42471: Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.

[Joris Van Remoortere]

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

(Updated Jan. 21, 2016, 7:36 a.m.)


Review request for mesos, Jie Yu and Michael Park.


Changes
---

Added logic for `Mount` type.


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


Repository: mesos


Description
---

Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.


Diffs (updated)
-

  include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a 
  include/mesos/v1/mesos.hpp 961042d8e4944a475076b829966020d62175d726 
  src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
  src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 42470: Multiple Disk: Added 'Source' to 'Resource.DiskInfo'.

[Joris Van Remoortere]

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

(Updated Jan. 21, 2016, 7:35 a.m.)


Review request for mesos, Jie Yu and Michael Park.


Changes
---

Added `Mount` type.


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


Repository: mesos


Description
---

Multiple Disk: Added 'Source' to 'Resource.DiskInfo'.


Diffs (updated)
-

  include/mesos/mesos.proto 0be4bed336e86a5c377e87ac6212c70ac3b4c66b 
  include/mesos/v1/mesos.proto c3244e87f9351c71312d2eace7a49bcac926fafd 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 42589: Added test case for allocator recover with Quota.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [42535, 42589]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 21, 2016, 6:27 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42589/
> ---
> 
> (Updated Jan. 21, 2016, 6:27 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Joris Van 
> Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test case for allocator recover with Quota.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 953712149bd951789beb29c72779c4ac65aa48dc 
> 
> Diff: https://reviews.apache.org/r/42589/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER="HierarchicalAllocatorTest.RecoverWithQuota" (CHECK() 
> failed without fix)
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 41847: Updated allocation slack when slave was updated.

[Guangya Liu]

> On 一月 16, 2016, 2:21 a.m., Joseph Wu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 529-535
> > 
> >
> > What if you did this?
> > ```
> > slaves[slaveId]total = slaves[slaveId].total.nonUsageSlack() + 
> > oversubscribed;
> > ```
> > 
> > Where `nonUsageSlack` is a helper that filters out usage slack.  (You 
> > could just use a Resource filter directly here.)
> > 
> > ---
> > 
> > This would have fewer operations too (1 filter operation instead of 2).
> 
> Guangya Liu wrote:
> I think that the current logic is that when 
> enableReservationOversubscription is true, we only need to add the allocation 
> slack to the total resources, there is no other change except this.
> 
> The `allocationSlack()` also has only 1 filter `isAllocationSlack` , 
> comments?
> 
> Joseph Wu wrote:
> I'm talking about the `.nonRevocable()` and the `allocationSlack()` 
> filters.
> 
> The current patch is a pretty roundabout way of writing this:
> ```
> slaves[slaveId].total = oversubscribed + 
>   slaves[slaveId].total.filter([](const Resource& resource) { return 
> !isUsageSlack(resource); });
> ```
> 
> ^ This makes it explicit that we're just resetting the `USAGE_SLACK` 
> component and leaving everything else untouched.
> 
> Guangya Liu wrote:
> My logic here is that for update `updateSlave`, do not touch other logic 
> but only add the `allocationSlack` when `enableReservationOversubscription` 
> is enabled, this may be cleaner? As I was only adding `allocationSlack` when 
> `enableReservationOversubscription` is enabled.
> 
> Joseph Wu wrote:
> The `enableReservationOversubscription` flag shouldn't even matter here.  
> If you really want to be strict, you can add this:
> ```
> if (enableReservationOversubscription) {
>   CHECK_EQ(Resources(), slaves[slaveId].total.allocationSlack());
> }
> ```

@Joseph, so the logic you proposed is as this?

if (enableReservationOversubscription) {
  slaves[slaveId]total = slaves[slaveId].total.nonUsageSlack() + oversubscribed;
} else {
  slaves[slaveId].total = total.nonRevocable() + oversubscribed;
}

Yes, the above logic can make allocator call only 1 filter. If this is your 
proposal, I will update the patch and move the `nonUsage` before this patch.


- Guangya


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


On 一月 13, 2016, 12:55 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41847/
> ---
> 
> (Updated 一月 13, 2016, 12:55 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, Joseph Wu, Klaus Ma, and Jian Qiu.
> 
> 
> Bugs: MESOS-4145
> https://issues.apache.org/jira/browse/MESOS-4145
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated allocation slack when slave was updated.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> d541bfa3f4190865c65d35c9d1ffdb8a3f194056 
>   src/tests/hierarchical_allocator_tests.cpp 
> e044f832c2c16e53e663c6ced5452649bb0dcb59 
> 
> Diff: https://reviews.apache.org/r/41847/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> GLOG_v=2 ./bin/mesos-tests.sh  --gtest_filter="HierarchicalAllocatorTest.*" 
> --verbose
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 42355: Removed the timeout from the filter.

[Ben Mahler]

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


Thanks Alex, code change looks great. Feel free to split the fix and the tests 
into different patches if you like.

Is there also an existing test for an offer filter being larger than the 
allocation timeout?


src/master/allocator/mesos/hierarchical.cpp (line 75)


It would be great to isolate this fix, any reason you've included the 
private addition here rather than in a separate patch?



src/master/allocator/mesos/hierarchical.cpp (line 938)


// see MESOS-4302 for more information.



src/master/allocator/mesos/hierarchical.cpp (line 942)


Don't change it right now, but seems that 'seconds' was an unfortunate 
name, the following would have been clearer?

```
timeout = std::max(allocationInterval, timeout);
```

Let's keep track of this for later so that we can get to it during the 
allocator cleanups.



src/tests/hierarchical_allocator_tests.cpp (lines 449 - 451)


Why mention expiration between two consecutive allocations here? The way I 
had been thinking about a test here was that we ensure that a filter always 
expires after the next batch allocation. 

So, is the fact that there are two batch allocations here is just an 
implementation detail as far as testing is concerned?

Would you mind referencing MESOS-4302 so that the next person to look at 
this test can follow the history a little more easily?



src/tests/hierarchical_allocator_tests.cpp (line 452)


but the test may need adjustment as far as timing is concerned, yes?



src/tests/hierarchical_allocator_tests.cpp (lines 480 - 481)


Took me some time to figure out why this note is here :)

How about placing the addSlave call before we add the frameworks? Will that 
avoid the need for omitting the allocation here and hence the need for the NOTE?



src/tests/hierarchical_allocator_tests.cpp (lines 527 - 528)


Why not explcitly set the allocation interval by passing the flags into 
initialize()? It seems a bit fragile to assume 100ms is less than the implicit 
default, which may change.



src/tests/hierarchical_allocator_tests.cpp (lines 544 - 560)


In the bottom section of this test, I'm not sure folks without our context 
will understand what is being done and what is expected to occur.

Maybe we just write something as simple as the following, for example:

The allocator will ensure that offer filters are removed after at least one 
batch allocation has occurred. Therefore, we expect that after the timeout 
elapses, the filter remains active for the next allocation and the resources 
are sent to framework1.

Then, I'm curious why you don't have a subsequent allocation to verify that 
the offer filter was really removed, is there a test for that?



src/tests/hierarchical_allocator_tests.cpp (line 546)


Can you use offerFilter.refuse_seconds() here? Or create a 'timeout' 
variable which is used to both set the offerFitlter.refuse_seconds and is 
passed to this advance() call?



src/tests/hierarchical_allocator_tests.cpp (line 1303)


Would you mind omitting this change here, so that this patch is focused 
solely on the fix?



src/tests/hierarchical_allocator_tests.cpp (lines 1408 - 1409)


As far as terminology goes, it would be great to consistently refer to 
"batch allocation", otherwise readers may be confused as to whether there is a 
distinction between a "periodic allocation" and a "batch allocation".


- Ben Mahler


On Jan. 19, 2016, 11:32 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42355/
> ---
> 
> (Updated Jan. 19, 2016, 11:32 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-4302
> https://issues.apache.org/jira/browse/MESOS-4302
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Without the timeout, we rely on filter expiration only. This guarantees
> that filter removal is scheduled after `allocate()` if the allocator is
> backlogged given default parameters are

Re: Review Request 42239: Added tests for the Docker URI fetcher plugin.

[Timothy Chen]

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

Ship it!


Ship It!

- Timothy Chen


On Jan. 21, 2016, 12:41 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42239/
> ---
> 
> (Updated Jan. 21, 2016, 12:41 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-4296
> https://issues.apache.org/jira/browse/MESOS-4296
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for the Docker URI fetcher plugin.
> 
> 
> Diffs
> -
> 
>   src/tests/uri_fetcher_tests.cpp c3bc4eaa26edf2c4e827341794b230d022a91283 
> 
> Diff: https://reviews.apache.org/r/42239/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 40632: Enabled oversubscribed resources for reservations in allocator.

[Guangya Liu]

> On 一月 16, 2016, 2:11 a.m., Joseph Wu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1291-1319
> > 
> >
> > None of this should be necessary:
> > 
> > 1) You should have all the allocation slack inside `available`.
> > 2) We don't have allocation slack ACLs in the MVP, so all allocation 
> > slack should fall under `available.unreserved`.  If we do have ACLs, some 
> > of the allocation slack will be accounted for in `available.reserved(role)`.
> > 3) You shouldn't need to recalculate this during `::allocate`.
> > 
> > If any of the above are not true, you probably have a problem in the 
> > `Resource` helpers or in `::addSlave`.
> 
> Guangya Liu wrote:
> We can take a look at the following cases:
> 1) total resources cpus(r1):100
> 2) register a framework f1, cycle 1: allocated 
> cpus(r1):100;cpu(*){ALLOCATION_SLACK}:100 to f1
> 3) f1 recovered cpu(*){ALLOCATION_SLACK}:100 back
> 4) register another framwork f2, f2 get nothing here as f1 used up all 
> reserved resoures and there is no allocation slack now.
> 
> The above logic make sure that step 4) can always get the latest 
> remaining allocation slack.
> 
> Joseph Wu wrote:
> The step 3) in your case is presumably because you haven't implemented 
> all the allocator methods yet (as of this review in the chain), like 
> `recoverResources`, `updateAllocation`, and `updateAvailable`.
> 
> If so, you should consider moving the test additions/changes before/after 
> (this lets us know what your intended behavior is) the allocator changes.
> 
> Guangya Liu wrote:
> I was updating the comments here to make it more clear.
> 
> // Calculate the `remainingAllocationSlack` if the framework can
> // use revocable resources and reservation oversubscription also
> // enabled. The `remainingAllocationSlack` need to exclude the
> // stateless reserved resources allocated in previous allocation
> // cycle.
> 
> Joseph Wu wrote:
> I would still **highly recommend** that you don't recalculate the 
> allocation slack inside a doubly-nested for-loop.
> 
> And I'll reinterate: if you can't find the allocation slack inside the 
> `available` Resources object, then you're doing something wrong.  You should 
> probably double-check if this is the case.

The reason I need to recalcuate this is because for a role, let's say `r1`, the 
`r1`'s resources including both `reserved` and `allocation slack` would be send 
out as an offer for `r1` if a framework is using `r1`.

A case is as this:
1) agent1 with cpus(r1):100
2) add agent1, then agent1 reosurces will become 
cpus(r1):100;cpus(*){ALLOCATION_SLACK}:100
3) Register a framework with r1
4) The allocator will send out resources as 
cpus(r1):100;cpus(*){ALLOCATION_SLACK}:100. For here, as we want thr framework 
can also run revocable tasks, so here, we are sending both reserved and 
revocable resources to framework and let the framework select the resources.
5) The allcator recover the allocation slack
6) When allocate again, the allocator need to recalculate the allocation slack 
and found it was 0

But if we do not send out both one role's `reserved` and `allocation slack`, we 
can simply use the `allocation slack` in `available` resources.

This is a question that we need to discuss today: Do we need to send out the 
offer in one role include both `reserved` and `allocation slack` ?


- Guangya


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


On 一月 20, 2016, 6:25 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40632/
> ---
> 
> (Updated 一月 20, 2016, 6:25 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, Joseph Wu, Klaus Ma, and Jian Qiu.
> 
> 
> Bugs: MESOS-4145
> https://issues.apache.org/jira/browse/MESOS-4145
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled oversubscribed resources for reservations in allocator.
> The allocator part including 5 patches:
> 1) https://reviews.apache.org/r/40632 Enabled oversubscribed resources for 
> reservations in allocator
> 2) https://reviews.apache.org/r/41847 Updated allocation slack when slave was 
> updated.
> 3) https://reviews.apache.org/r/41791 Updated allocation slack for dynamic 
> reserve (1/3).
> 4) https://reviews.apache.org/r/42113 Handle unreserve logic for dynamic 
> reservation (2/3).
> 5) https://reviews.apache.org/r/42194 Handle unreserve logic for dynamic 
> reservation (3/3).
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos

Re: Review Request 42591: Added Framework protobufs to registry.

[Yongqiao Wang]

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

(Updated Jan. 21, 2016, 6:44 a.m.)


Review request for mesos, Ben Mahler and Vinod Kone.


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


Repository: mesos


Description (updated)
---

Add the protobufs in registry to persist some framework informations which do 
not allow to change when framework re-register.


Diffs
-

  src/master/registry.proto 9958f9c2bdb785390fca2f292b65d5a9310434d5 

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


Testing
---

Make && Make check successfully


Thanks,

Yongqiao Wang

Review Request 42591: Added Framework protobufs to registry.

[Yongqiao Wang]

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

Review request for mesos, Ben Mahler and Vinod Kone.


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


Repository: mesos


Description
---

Define the protobufs in registry to persist some framework infoamation which do 
not allowed to changed when framework re-register.


Diffs
-

  src/master/registry.proto 9958f9c2bdb785390fca2f292b65d5a9310434d5 

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


Testing
---

Make && Make check successfully


Thanks,

Yongqiao Wang

Re: Review Request 41772: Added helper function to flatten resources.

[Guangya Liu]

> On 一月 19, 2016, 10:18 p.m., Joseph Wu wrote:
> > src/common/resources.cpp, lines 874-877
> > 
> >
> > This is pretty much a copy of `Resources::reserved` now.  You can 
> > remove it.
> 
> Guangya Liu wrote:
> The reason that I keep this is because the current `Resources::reserved` 
> returns a hashmap but what I need here is a flatten string for resources in 
> different roles. e.g. cpus(r1):100;cpus(r2):200
> 
> I updated the comments here to clarify why this is needed.
> 
> Klaus Ma wrote:
> I think it's more general to add a `Resources` constructor to accept 
> hashmap, so it'll be `Resources(res.reserved()).flatten()`.
> 
> Joseph Wu wrote:
> It doesn't look like `reserved()` is used very heavily.  And the places 
> that do use it could easily use a normal `Resources` object.  I'd recommend 
> changing `reserved()` or just using `reserved("*")`.

I created a new patch here https://reviews.apache.org/r/42590/ , can you help 
review? I think that the `reserved()` API can be removed.


- Guangya


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


On 一月 20, 2016, 3:40 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> ---
> 
> (Updated 一月 20, 2016, 3:40 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, Joseph Wu, and Klaus Ma.
> 
> 
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper function to flatten resources.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa 
>   include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c 
>   src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
>   src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666 
>   src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 
> 
> Diff: https://reviews.apache.org/r/41772/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> GLOG_v=2 ./bin/mesos-tests.sh  --gtest_filter="ResourcesOperationTest.*" 
> --verbose
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Review Request 42590: Removed reserved() API.

[Guangya Liu]

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

Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, 
Joseph Wu, and Klaus Ma.


Bugs: ESOS-4447
https://issues.apache.org/jira/browse/ESOS-4447


Repository: mesos


Description
---

Removed reserved() API.


Diffs
-

  include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa 
  include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c 
  src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
  src/master/http.cpp 12c1fe5a514903f657911302e8770e9b245fdbb7 
  src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666 
  src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 42589: Added test case for allocator recover with Quota.

[Klaus Ma]

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

(Updated Jan. 21, 2016, 2:27 p.m.)


Review request for mesos, Alexander Rukletsov, Ben Mahler, and Joris Van 
Remoortere.


Changes
---

Add description on testing.


Repository: mesos


Description
---

Added test case for allocator recover with Quota.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
953712149bd951789beb29c72779c4ac65aa48dc 

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


Testing (updated)
---

make
make check GTEST_FILTER="HierarchicalAllocatorTest.RecoverWithQuota" (CHECK() 
failed without fix)


Thanks,

Klaus Ma

Re: Review Request 42535: Made allocator's pause/resume idempotent.

[Klaus Ma]

> On Jan. 20, 2016, 3:11 p.m., Klaus Ma wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1048
> > 
> >
> > Suggest to add some tests for this case.
> 
> Alexander Rukletsov wrote:
> I totally agree we need more tests for allocator recovery (we have none 
> right now). But could you please describe the particular one you have in mind?
> 
> Klaus Ma wrote:
> Sorry for short comments :). That's the case I'd suggest to add:
> 
> 1. initialize()
> 2. create one Quota; one Quota is enough to avoid return before 
> `delay(..., resume())` in `::recover()`;
> 3. `allocator->recover(2, quotas)`;
> 4. add two slaves by `addSlave()`, the latest `addSlave` will trigger 
> `::resume()`
> 5. `advance(10mins)` to trigger resume() again which is delay in 
> `::recover()`
>  
> Without this fix, `CHECK` should be failed because `::resume()` twice. 
> 
> BTW, should be a JIRA to export `ALLOCATION_HOLD_OFF_RECOVERY_TIMEOUT` & 
> `AGENT_RECOVERY_FACTOR` as flags?

Not sure whether you have time on this test case, draft a path for that; would 
you help to review?


- Klaus


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


On Jan. 20, 2016, 10:09 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42535/
> ---
> 
> (Updated Jan. 20, 2016, 10:09 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-4417
> https://issues.apache.org/jira/browse/MESOS-4417
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 101482156ffc5a4fe3cd60be222bfe609330ec3c 
>   src/master/allocator/mesos/hierarchical.cpp 
> e32ee4aa3ed9793bb5a99233e699e5cc2bdd796b 
> 
> Diff: https://reviews.apache.org/r/42535/diff/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 42589: Added test case for allocator recover with Quota.

[Klaus Ma]

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

Review request for mesos, Alexander Rukletsov, Ben Mahler, and Joris Van 
Remoortere.


Repository: mesos


Description
---

Added test case for allocator recover with Quota.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
953712149bd951789beb29c72779c4ac65aa48dc 

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


Testing
---

make
make check GTEST_FILTER="HierarchicalAllocatorTest.RecoverWithQuota"


Thanks,

Klaus Ma

Re: Review Request 42288: Add timestamp to DockerContainerizer's ResourceStatistics.

[Andy Pang]

> On 一月 21, 2016, 6:12 a.m., Mesos ReviewBot wrote:
> > Bad patch!
> > 
> > Reviews applied: [42288]
> > 
> > Failed command: ./support/apply-review.sh -n -r 42288
> > 
> > Error:
> >  2016-01-21 06:12:19 URL:https://reviews.apache.org/r/42288/diff/raw/ 
> > [612/612] -> "42288.patch" [1]
> > Total errors found: 0
> > Checking 1 files
> > Error: Commit message summary (the first line) must end in a period.

Thanks,I have fix it


- Andy


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


On 一月 21, 2016, 6:18 a.m., Andy Pang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42288/
> ---
> 
> (Updated 一月 21, 2016, 6:18 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-4328
> https://issues.apache.org/jira/browse/MESOS-4328
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Docker container REST API /monitor/statistics.json output have no timestamp 
> field, in docker.cpp function cgroupsStatistics add the timestamp value set.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 40f6f0b 
> 
> Diff: https://reviews.apache.org/r/42288/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Andy Pang
> 
>

Re: Review Request 42288: Add timestamp to DockerContainerizer's ResourceStatistics.

[Andy Pang]

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

(Updated 一月 21, 2016, 6:18 a.m.)


Review request for mesos, Jie Yu and Timothy Chen.


Summary (updated)
-

Add timestamp to DockerContainerizer's ResourceStatistics.


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


Repository: mesos


Description (updated)
---

Docker container REST API /monitor/statistics.json output have no timestamp 
field, in docker.cpp function cgroupsStatistics add the timestamp value set.


Diffs
-

  src/slave/containerizer/docker.cpp 40f6f0b 

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


Testing
---

make check


Thanks,

Andy Pang

Re: Review Request 42288: Add timestamp to DockerContainerizer's ResourceStatistics

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [42288]

Failed command: ./support/apply-review.sh -n -r 42288

Error:
 2016-01-21 06:12:19 URL:https://reviews.apache.org/r/42288/diff/raw/ [612/612] 
-> "42288.patch" [1]
Total errors found: 0
Checking 1 files
Error: Commit message summary (the first line) must end in a period.

- Mesos ReviewBot


On Jan. 21, 2016, 4:12 a.m., Andy Pang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42288/
> ---
> 
> (Updated Jan. 21, 2016, 4:12 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-4328
> https://issues.apache.org/jira/browse/MESOS-4328
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Docker container REST API /monitor/statistics.json output have no timestamp 
> field
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 40f6f0b 
> 
> Diff: https://reviews.apache.org/r/42288/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Andy Pang
> 
>

Re: Review Request 41790: Add tests for /weights endpoint.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [41597, 41681, 41789]

Failed command: ./support/apply-review.sh -n -r 41789

Error:
 2016-01-21 05:51:10 URL:https://reviews.apache.org/r/41789/diff/raw/ 
[8436/8436] -> "41789.patch" [1]
error: patch failed: 3rdparty/libprocess/src/tests/http_tests.cpp:92
error: 3rdparty/libprocess/src/tests/http_tests.cpp: patch does not apply

- Mesos ReviewBot


On Jan. 21, 2016, 3:08 a.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41790/
> ---
> 
> (Updated Jan. 21, 2016, 3:08 a.m.)
> 
> 
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
> 
> 
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add tests for /weights endpoint.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 4fabe600d4ba38c95a777d622b0b675dd5811a53 
>   src/tests/dynamic_weights_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41790/diff/
> 
> 
> Testing
> ---
> 
> Make and Make check successfully!
> 
> ./src/mesos-tests --gtest_filter=DynamicWeightsTest.*
> [==] Running 11 tests from 1 test case.
> [--] Global test environment set-up.
> [--] 11 tests from DynamicWeightsTest
> [ RUN  ] DynamicWeightsTest.PutInvalidRequest
> [   OK ] DynamicWeightsTest.PutInvalidRequest (102 ms)
> [ RUN  ] DynamicWeightsTest.ZeroWeight
> [   OK ] DynamicWeightsTest.ZeroWeight (38 ms)
> [ RUN  ] DynamicWeightsTest.NegativeWeight
> [   OK ] DynamicWeightsTest.NegativeWeight (38 ms)
> [ RUN  ] DynamicWeightsTest.MissingRole
> [   OK ] DynamicWeightsTest.MissingRole (43 ms)
> [ RUN  ] DynamicWeightsTest.UnknownRole
> [   OK ] DynamicWeightsTest.UnknownRole (36 ms)
> [ RUN  ] DynamicWeightsTest.UpdateWeightsWithExplictRoles
> [   OK ] DynamicWeightsTest.UpdateWeightsWithExplictRoles (44 ms)
> [ RUN  ] DynamicWeightsTest.UpdateWeightsWithImplicitRoles
> [   OK ] DynamicWeightsTest.UpdateWeightsWithImplicitRoles (52 ms)
> [ RUN  ] DynamicWeightsTest.UnauthenticatedUpdateWeightRequest
> [   OK ] DynamicWeightsTest.UnauthenticatedUpdateWeightRequest (40 ms)
> [ RUN  ] DynamicWeightsTest.AuthorizedWeightUpdateRequest
> [   OK ] DynamicWeightsTest.AuthorizedWeightUpdateRequest (32 ms)
> [ RUN  ] DynamicWeightsTest.AuthorizedUpdateWeightRequestWithoutPrincipal
> [   OK ] DynamicWeightsTest.AuthorizedUpdateWeightRequestWithoutPrincipal 
> (37 ms)
> [ RUN  ] DynamicWeightsTest.UnauthorizedWeightUpdateRequest
> [   OK ] DynamicWeightsTest.UnauthorizedWeightUpdateRequest (30 ms)
> [--] 11 tests from DynamicWeightsTest (492 ms total)
> 
> [--] Global test environment tear-down
> [==] 11 tests from 1 test case ran. (503 ms total)
> [  PASSED  ] 11 tests.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 41772: Added helper function to flatten resources.

[Guangya Liu]

> On 一月 19, 2016, 10:18 p.m., Joseph Wu wrote:
> > src/common/resources.cpp, line 880
> > 
> >
> > It should be fine to just name this `flatten`.
> > 
> > You should also consider changing the parameter type from 
> > `RevocableInfo::Type` to just `RevocableInfo`.  Supplying the whole 
> > `RevocableInfo` will save us another tweak to the helpers if we change the 
> > revocable protobufs again.
> 
> Guangya Liu wrote:
> I think that we cannot rename it to `flatten` as the compiler will not 
> know what does `flatten()` mean.
> 
> Klaus Ma wrote:
> Regarding from `RevocableInfo::Type` to `RevocableInfo`, do you mean 
> change `RevocableInfo` from `message` to `enum`? I think it's better to 
> define const in `Resources` instead of changing `RevocableInfo`'s type, e.g. 
> `static const Resource::RevocableInfo::Type Resources::ALLOCATION_SLACK = 
> Resource::RevocableInfo::Type::ALLOCATION_SLACK`.
> 
> Regarding `flatten`, there're two options to me:
> 
> * Option 1: add `RevocableInfo::Type` to as 3rd parameter in current 
> `flatten` with `None()` as default vaule, and a new helper function to accept 
> `RevocableInfo::Type` only without default value. The sample code is:
> 
> ```
> Resources flatten(string role = "*", Option 
> resveration = None(), Option revocable = None()) {
>   ...
>   if (revocable.isSome()) {
> flatten(revocable.get());
>   } else {
> // clear revocable info.
>   }
> }
> 
> Resources flatten(RevocableInfo::Type revocable) {
>   foreach resources {
> resource.mutable_revocable()->set_type(revocable);
>   }
> }
> ```
> 
> * Option 2: add new `flatten` functions but did not provide default 
> value; `None()` means clear the `RevocableInfo::Type`.
> 
> I perfer to Option 1 :). Any comments?
> 
> Guangya Liu wrote:
> option 1 will cause all caller who want to flatten allocation slack need 
> to input all of the four parameter, it is really not convenient for the 
> caller.
> Option 2 need to add a new `flatten` with a `requested` but not `option` 
> parameter, I think that maybe 2 is better.
> 
> Klaus Ma wrote:
> regarding 1, if want to clear the flags (role, reservation and 
> revocable), call `flatten()`; if want to set `ALLOCATION_SLACK`, call 
> `flatten(Resources::ALLOCATION_SLACK)`.
> 
> Guangya Liu wrote:
> Just clarify 1) here, I have some mis-understanding for it.
> 
> Option 1) has involved two APIs, one is update current `Resources 
> flatten(string role = "*", Option resveration = None(), , 
> Option revocable = None() )`, the other is add a new 
> `flatten(RevocableInfo::Type revocable)`. The `first flatten` is used to 
> clear `allocation slack` and the `second flatten` is used to set `allocation 
> slack` flag.
> 
> Seems 1) is better as it faciliates the caller.
> 
> Joseph Wu wrote:
> Given how you're using `flatten`, it would be better to have two calls:
> `Resources flatten(string role = "*", Option resveration 
> = None())`
> and 
> `Resources flatten(Option revocable)`  // No default so 
> there's no ambiguity.
> 
> ---
> 
> Note: If you wanted a single `flatten`, you'd need to have 
> `Option>` so that you have the option of setting, 
> un-setting, or not-changing each field.

I think that adding a new `flatten` without default value might be better and 
clear.


- Guangya


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


On 一月 20, 2016, 3:40 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> ---
> 
> (Updated 一月 20, 2016, 3:40 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, Joseph Wu, and Klaus Ma.
> 
> 
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper function to flatten resources.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa 
>   include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c 
>   src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
>   src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666 
>   src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 
> 
> Diff: https://reviews.apache.org/r/41772/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> GLOG_v=2 ./bin/mesos-tests.sh  --gtest_filter="Resources

Re: Review Request 42517: Added discussion about allowing multiple frameworks in a role.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [42517]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 20, 2016, 9:57 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42517/
> ---
> 
> (Updated Jan. 20, 2016, 9:57 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added discussion about allowing multiple frameworks in a role.
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md 4af7d6e8dc648fb630f56db5fbad1b7b438ebcfb 
>   docs/roles.md 609a63cbff2d9c652af45ba16152ce3caf48 
> 
> Diff: https://reviews.apache.org/r/42517/diff/
> 
> 
> Testing
> ---
> 
> Previewed on github.
> 
> Note that the link to `roles.md` doesn't work at the moment, but I believe it 
> should work once Joerg's fix for the `Rakefile` is merged.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 42288: Add timestamp to DockerContainerizer's ResourceStatistics

[Andy Pang]

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

(Updated 一月 21, 2016, 4:12 a.m.)


Review request for mesos, Jie Yu and Timothy Chen.


Changes
---

merge the newest git branch


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


Repository: mesos


Description
---

Docker container REST API /monitor/statistics.json output have no timestamp 
field


Diffs (updated)
-

  src/slave/containerizer/docker.cpp 40f6f0b 

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


Testing
---

make check


Thanks,

Andy Pang

Re: Review Request 42580: Updated changelog :renaming of 'authenticate' to 'authenticate_frameworks' flag

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [42580]

Failed command: ./support/apply-review.sh -n -r 42580

Error:
 2016-01-21 03:54:18 URL:https://reviews.apache.org/r/42580/diff/raw/ [467/467] 
-> "42580.patch" [1]
fatal: corrupt patch at line 14

- Mesos ReviewBot


On Jan. 21, 2016, 1:11 a.m., Disha  Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42580/
> ---
> 
> (Updated Jan. 21, 2016, 1:11 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4386
> https://issues.apache.org/jira/browse/MESOS-4386
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated changelog :renaming of 'authenticate' to 'authenticate_frameworks' 
> flag
> 
> 
> Diffs
> -
> 
>   CHANGELOG 912a402 
> 
> Diff: https://reviews.apache.org/r/42580/diff/
> 
> 
> Testing
> ---
> 
> Not-required.Not a functional change.
> 
> 
> Thanks,
> 
> Disha  Singh
> 
>

Re: Review Request 42355: Removed the timeout from the filter.

[Qian Zhang]

> On Jan. 20, 2016, 9:49 a.m., Qian Zhang wrote:
> > One question: Say allocation interval is 10s, at the time 5s, framework 
> > sets a filter with 3s, so with this patch, we will expire the filter 10s 
> > (max(10, 3)) later, i.e., at the time 15s. Then at the time of 10s (the 
> > next allocation cycle), allocator will not allocate any resources to the 
> > framework due to the 10s filter which is good and is the issue that we 
> > intend to fix. And then in the time 12, a new slave joins, at this moment, 
> > allocator will not allocate any resources to the framework too due to the 
> > 10s filter, but maybe the new slave has the resources needed by the 
> > framework. So my question is whether this is a reasonable behavior, do we 
> > filter too much for the framework in this case?
> 
> Qian Zhang wrote:
> In this case, do we need to cancel the filter once it has taken effect 
> for one time and last for long enough time?
> 
> Alexander Rukletsov wrote:
> Your concern is valid and we indeed may filter too much. I wonder how 
> probable is your scenario in real-world setups.
> 
> Our intention is "filter for X seconds but at least for one allocation 
> touching filtered agent". What we have here is more of a hack and I'd rather 
> remove `std::max()` in favor of a proper fix, which is allocating on resource 
> recovery (MESOS-3078). Does a TODO I left in the code explan it?
> 
> Alexander Rukletsov wrote:
> To clarify Qian's concern and my answer: filters are set per-agent basis, 
> so a new agent joining the cluster won't be filtered by any existing filters. 
> However, we indeed may filter longer than asked by a framework, but I think 
> being precise about the filter duration is less important than making the 
> refused resources available for other frameworks.

Yes, I agree it does make sense, thanks for the clarification!


- Qian


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


On Jan. 20, 2016, 7:32 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42355/
> ---
> 
> (Updated Jan. 20, 2016, 7:32 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-4302
> https://issues.apache.org/jira/browse/MESOS-4302
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Without the timeout, we rely on filter expiration only. This guarantees
> that filter removal is scheduled after `allocate()` if the allocator is
> backlogged given default parameters are used. Additionally we ensure the
> filter timeout is at least as big as the allocation interval.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 48acde69b1a2f305b568a7e322a58708063dd30a 
>   src/tests/hierarchical_allocator_tests.cpp 
> 9362dd306497ba01e0f387c3862456cdcac6f863 
> 
> Diff: https://reviews.apache.org/r/42355/diff/
> 
> 
> Testing
> ---
> 
> On Mac OS 10.10.4:
> 
> `make check`
> 
> `GTEST_FILTER="HierarchicalAllocatorTest.FilterTimeout" ./bin/mesos-tests.sh 
> --gtest_repeat=100 --gtest_break_on_failure` passes with the patch and fails 
> without.
> 
> `GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh 
> --gtest_repeat=100 --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 42288: Add timestamp to DockerContainerizer's ResourceStatistics

[Andy Pang]

> On 一月 14, 2016, 9:19 a.m., Mesos ReviewBot wrote:
> > Bad patch!
> > 
> > Reviews applied: [42288]
> > 
> > Failed command: ./support/apply-review.sh -n -r 42288
> > 
> > Error:
> >  2016-01-14 09:19:38 URL:https://reviews.apache.org/r/42288/diff/raw/ 
> > [612/612] -> "42288.patch" [1]
> > Total errors found: 0
> > Checking 1 files
> > Error: Commit message summary (the first line) must not exceed 72 
> > characters.

Thanks?I have fix it, modify the summary as "Add timestamp to 
DockerContainerizer's ResourceStatistics"


- Andy


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


On 一月 21, 2016, 3:46 a.m., Andy Pang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42288/
> ---
> 
> (Updated 一月 21, 2016, 3:46 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-4328
> https://issues.apache.org/jira/browse/MESOS-4328
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Docker container REST API /monitor/statistics.json output have no timestamp 
> field
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp da19975 
> 
> Diff: https://reviews.apache.org/r/42288/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Andy Pang
> 
>

Re: Review Request 42288: Add timestamp to DockerContainerizer's ResourceStatistics

[Andy Pang]

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

(Updated 一月 21, 2016, 3:46 a.m.)


Review request for mesos, Jie Yu and Timothy Chen.


Changes
---

modify the summary,length less than 72 chars


Summary (updated)
-

Add timestamp to DockerContainerizer's ResourceStatistics


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


Repository: mesos


Description
---

Docker container REST API /monitor/statistics.json output have no timestamp 
field


Diffs
-

  src/slave/containerizer/docker.cpp da19975 

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


Testing
---

make check


Thanks,

Andy Pang

Re: Review Request 42239: Added tests for the Docker URI fetcher plugin.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [42235, 41961, 42236, 42237, 42275, 42238, 42442, 42443, 
42444, 42239]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 21, 2016, 12:41 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42239/
> ---
> 
> (Updated Jan. 21, 2016, 12:41 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-4296
> https://issues.apache.org/jira/browse/MESOS-4296
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for the Docker URI fetcher plugin.
> 
> 
> Diffs
> -
> 
>   src/tests/uri_fetcher_tests.cpp c3bc4eaa26edf2c4e827341794b230d022a91283 
> 
> Diff: https://reviews.apache.org/r/42239/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 41790: Add tests for /weights endpoint.

[Yongqiao Wang]

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

(Updated Jan. 21, 2016, 3:08 a.m.)


Review request for mesos, Adam B, Neil Conway, and Qian Zhang.


Changes
---

Add the denpendy to fix the build error.


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


Repository: mesos


Description
---

Add tests for /weights endpoint.


Diffs
-

  src/Makefile.am 4fabe600d4ba38c95a777d622b0b675dd5811a53 
  src/tests/dynamic_weights_tests.cpp PRE-CREATION 

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


Testing
---

Make and Make check successfully!

./src/mesos-tests --gtest_filter=DynamicWeightsTest.*
[==] Running 11 tests from 1 test case.
[--] Global test environment set-up.
[--] 11 tests from DynamicWeightsTest
[ RUN  ] DynamicWeightsTest.PutInvalidRequest
[   OK ] DynamicWeightsTest.PutInvalidRequest (102 ms)
[ RUN  ] DynamicWeightsTest.ZeroWeight
[   OK ] DynamicWeightsTest.ZeroWeight (38 ms)
[ RUN  ] DynamicWeightsTest.NegativeWeight
[   OK ] DynamicWeightsTest.NegativeWeight (38 ms)
[ RUN  ] DynamicWeightsTest.MissingRole
[   OK ] DynamicWeightsTest.MissingRole (43 ms)
[ RUN  ] DynamicWeightsTest.UnknownRole
[   OK ] DynamicWeightsTest.UnknownRole (36 ms)
[ RUN  ] DynamicWeightsTest.UpdateWeightsWithExplictRoles
[   OK ] DynamicWeightsTest.UpdateWeightsWithExplictRoles (44 ms)
[ RUN  ] DynamicWeightsTest.UpdateWeightsWithImplicitRoles
[   OK ] DynamicWeightsTest.UpdateWeightsWithImplicitRoles (52 ms)
[ RUN  ] DynamicWeightsTest.UnauthenticatedUpdateWeightRequest
[   OK ] DynamicWeightsTest.UnauthenticatedUpdateWeightRequest (40 ms)
[ RUN  ] DynamicWeightsTest.AuthorizedWeightUpdateRequest
[   OK ] DynamicWeightsTest.AuthorizedWeightUpdateRequest (32 ms)
[ RUN  ] DynamicWeightsTest.AuthorizedUpdateWeightRequestWithoutPrincipal
[   OK ] DynamicWeightsTest.AuthorizedUpdateWeightRequestWithoutPrincipal 
(37 ms)
[ RUN  ] DynamicWeightsTest.UnauthorizedWeightUpdateRequest
[   OK ] DynamicWeightsTest.UnauthorizedWeightUpdateRequest (30 ms)
[--] 11 tests from DynamicWeightsTest (492 ms total)

[--] Global test environment tear-down
[==] 11 tests from 1 test case ran. (503 ms total)
[  PASSED  ] 11 tests.


Thanks,

Yongqiao Wang

Re: Review Request 42559: Allocated resources as revocable beyond quota.

[Klaus Ma]

> On Jan. 21, 2016, 1:20 a.m., Klaus Ma wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1313
> > 
> >
> > Who will kill this `revocable` resources? So we'll have a patch for 
> > evicting?
> 
> Alexander Rukletsov wrote:
> Someday, someway... : )
> 
> Actually, I thought you guys are working on this (oversubscription for 
> reservations, incl. quota)!
> 
> Klaus Ma wrote:
> In "oversubscription for reservations", only `reserved & 
> !persistentVolume` resources are considered to be revocable to other 
> framework; the un-reserved resources are not handled for now.
> 
> To me, the key of eviction is which resources shoulbe be evicted for its 
> owner: 1.) in "oversubscription for reservations", it's `reserved & 
> !persistentVolume`, 2.) in "oversubscription" (Estimator & QoSController), it 
> dependent on modules;
> 
> When I polishing the design doc of Phase 2 (Simultaneous offer), I'm 
> thinking to do fair-share un-reserved resources by Quota/Role's Weight; for 
> example, 100 CPUS in cluster and two Quota: 30 CPUS (role1) & 20 CPUS 
> (role2); when only framework1 in role1 registered, it will get 30 CPUS 
> (reserved) + 30 CPUS (unreserved) + 20 CPUS (revocable), this offer is Quota 
> + (total - Quota1 - Quota2) * (Quota1/(Quota1 + Quota2)) + (total - Quota1 - 
> Quota2) * (Quota2/(Quota1 + Quota2)); when framework2 in role2 registered, 
> the revocable 20CPUS of framework1 will be evicted by master/allocator.
> 
> I logged MESOS-4303 to trace this idea; but did not get enough time on 
> the detail for now :). Maybe we can draft a doc on this, any comments?

typo: the offer should be "30 CPUS (quota) + 30 CPUS (unreserved) + 20 CPUS 
(revocable)".


- Klaus


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


On Jan. 21, 2016, 1:01 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42559/
> ---
> 
> (Updated Jan. 21, 2016, 1:01 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, Joseph Wu, 
> and Qian Zhang.
> 
> 
> Bugs: MESOS-4441
> https://issues.apache.org/jira/browse/MESOS-4441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6 
>   src/master/allocator/mesos/hierarchical.cpp 
> e32ee4aa3ed9793bb5a99233e699e5cc2bdd796b 
>   src/tests/hierarchical_allocator_tests.cpp 
> 953712149bd951789beb29c72779c4ac65aa48dc 
> 
> Diff: https://reviews.apache.org/r/42559/diff/
> 
> 
> Testing
> ---
> 
> On Mac OS 10.10.4:
> 
> `GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh 
> --gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle`
> `make check`
> 
> Updated `HierarchicalAllocatorTest.DRFWithQuota` succeeds with the change and 
> fails without.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 42559: Allocated resources as revocable beyond quota.

[Klaus Ma]

> On Jan. 21, 2016, 1:20 a.m., Klaus Ma wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1313
> > 
> >
> > Who will kill this `revocable` resources? So we'll have a patch for 
> > evicting?
> 
> Alexander Rukletsov wrote:
> Someday, someway... : )
> 
> Actually, I thought you guys are working on this (oversubscription for 
> reservations, incl. quota)!

In "oversubscription for reservations", only `reserved & !persistentVolume` 
resources are considered to be revocable to other framework; the un-reserved 
resources are not handled for now.

To me, the key of eviction is which resources shoulbe be evicted for its owner: 
1.) in "oversubscription for reservations", it's `reserved & 
!persistentVolume`, 2.) in "oversubscription" (Estimator & QoSController), it 
dependent on modules;

When I polishing the design doc of Phase 2 (Simultaneous offer), I'm thinking 
to do fair-share un-reserved resources by Quota/Role's Weight; for example, 100 
CPUS in cluster and two Quota: 30 CPUS (role1) & 20 CPUS (role2); when only 
framework1 in role1 registered, it will get 30 CPUS (reserved) + 30 CPUS 
(unreserved) + 20 CPUS (revocable), this offer is Quota + (total - Quota1 - 
Quota2) * (Quota1/(Quota1 + Quota2)) + (total - Quota1 - Quota2) * 
(Quota2/(Quota1 + Quota2)); when framework2 in role2 registered, the revocable 
20CPUS of framework1 will be evicted by master/allocator.

I logged MESOS-4303 to trace this idea; but did not get enough time on the 
detail for now :). Maybe we can draft a doc on this, any comments?


- Klaus


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


On Jan. 21, 2016, 1:01 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42559/
> ---
> 
> (Updated Jan. 21, 2016, 1:01 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, Joseph Wu, 
> and Qian Zhang.
> 
> 
> Bugs: MESOS-4441
> https://issues.apache.org/jira/browse/MESOS-4441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6 
>   src/master/allocator/mesos/hierarchical.cpp 
> e32ee4aa3ed9793bb5a99233e699e5cc2bdd796b 
>   src/tests/hierarchical_allocator_tests.cpp 
> 953712149bd951789beb29c72779c4ac65aa48dc 
> 
> Diff: https://reviews.apache.org/r/42559/diff/
> 
> 
> Testing
> ---
> 
> On Mac OS 10.10.4:
> 
> `GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh 
> --gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle`
> `make check`
> 
> Updated `HierarchicalAllocatorTest.DRFWithQuota` succeeds with the change and 
> fails without.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 42559: Allocated resources as revocable beyond quota.

[Guangya Liu]

> On 一月 21, 2016, 1:48 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1310
> > 
> >
> > The `resource` will include some `reserved` resources, here even if you 
> > call `resource.mutable_revocable();`, the `reserved` resources will still 
> > be considered as `reserved`.
> > 
> > Take a look at isReserved()
> > 
> > bool Resources::isReserved(
> > const Resource& resource,
> > const Option& role)
> > {
> >   if (role.isSome()) {
> > return !isUnreserved(resource) && role.get() == resource.role();
> >   } else {
> > return !isUnreserved(resource);
> >   }
> > }
> > 
> > 
> > bool Resources::isUnreserved(const Resource& resource)
> > {
> >   return resource.role() == "*" && !resource.has_reservation();
> > }
> > 
> > For a resources such as `cpus(r1):100`, after call 
> > `resource.mutable_revocable();`, it will be `cpus(r1){REV}:100`, and the 
> > helper function `isReserved(resources, role)` still treate this as 
> > `reserved` resources but not `revocable` resources.
> > 
> > This has highly dependency and interaction with MESOS-1607,the 
> > optimisitic offer phase 1, in phase 1, we are planning to treate the 
> > `reserved but not used` as `allocation slack`, it is not right if you 
> > simply translating all `resources` to `revocable`, you may want at least 
> > filter out the `reserved` resources when translating to `revocalbe` 
> > resources.
> > 
> > I would suggest you also invite bmahler as review as he may be the 
> > shephard for MESOS-1607

Correct one typo: `cpus(r1){REV}:100` will be treated as both `reserved` and 
`revocable` resources.

diff --git a/src/tests/resources_tests.cpp b/src/tests/resources_tests.cpp
index b42610f..517c70d 100644
--- a/src/tests/resources_tests.cpp
+++ b/src/tests/resources_tests.cpp
@@ -1968,6 +1968,10 @@ TEST(RevocableResourceTest, Filter)
   EXPECT_EQ(r2, r2.nonRevocable());
   EXPECT_TRUE(r2.revocable().empty());
 
  Resources r3 = createRevocableResource("cpus", "1", "r1", true);  
  EXPECT_EQ(r3, r3.revocable()); 
  EXPECT_EQ(r3, r3.reserved("r1")); 

   EXPECT_EQ(r1, (r1 + r2).revocable());
   EXPECT_EQ(r2, (r1 + r2).nonRevocable());
 }


- Guangya


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


On 一月 20, 2016, 5:01 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42559/
> ---
> 
> (Updated 一月 20, 2016, 5:01 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, Joseph Wu, 
> and Qian Zhang.
> 
> 
> Bugs: MESOS-4441
> https://issues.apache.org/jira/browse/MESOS-4441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6 
>   src/master/allocator/mesos/hierarchical.cpp 
> e32ee4aa3ed9793bb5a99233e699e5cc2bdd796b 
>   src/tests/hierarchical_allocator_tests.cpp 
> 953712149bd951789beb29c72779c4ac65aa48dc 
> 
> Diff: https://reviews.apache.org/r/42559/diff/
> 
> 
> Testing
> ---
> 
> On Mac OS 10.10.4:
> 
> `GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh 
> --gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle`
> `make check`
> 
> Updated `HierarchicalAllocatorTest.DRFWithQuota` succeeds with the change and 
> fails without.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 42516: Add support for user-defined networks.

[Qian Zhang]

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



src/docker/docker.cpp (line 517)


Do we need to check if `dockerInfo.network_name()` is empty? What will 
happend if we pass `--net ` into `docker run"


- Qian Zhang


On Jan. 20, 2016, 8:25 p.m., Ezra Silvera wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42516/
> ---
> 
> (Updated Jan. 20, 2016, 8:25 p.m.)
> 
> 
> Review request for mesos and TimothyIL TimothyIL.
> 
> 
> Bugs: MESOS-4369
> https://issues.apache.org/jira/browse/MESOS-4369
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Signed-off-by: Ezra Silvera 
> 
> Review: https://reviews.apache.org/r/42549
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 0be4bed336e86a5c377e87ac6212c70ac3b4c66b 
>   include/mesos/v1/mesos.proto c3244e87f9351c71312d2eace7a49bcac926fafd 
>   src/docker/docker.cpp 4d2f1fa14f4450b8fa3401081bf52807d2e79a7e 
> 
> Diff: https://reviews.apache.org/r/42516/diff/
> 
> 
> Testing
> ---
> 
> Using Swarm running on Mesos create a network with "docker  network create 
> --driver=bridge myNetwork"   and then create a container on that network:  
> "docker run --net=myNetwork"
> 
> 
> Thanks,
> 
> Ezra Silvera
> 
>

Re: Review Request 42559: Allocated resources as revocable beyond quota.

[Guangya Liu]

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


Another problem is with Qos Controller, the slave may found that the resource 
estimator report 10 revocable cpus but the slave is using 20 revocable cpus, if 
the qos controller has logic of killing executors in such condition, then even 
if you offer the resources to those tasks, it will still be killed by qos 
controller.


src/master/allocator/mesos/hierarchical.cpp (line 408)


When slave recover, the `revocable used` resources will also be reported 
back as `allocated`, this also cause some problem, as when `allocate()`, you 
are adding the resources which not coverted to `revocable`.


- Guangya Liu


On 一月 20, 2016, 5:01 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42559/
> ---
> 
> (Updated 一月 20, 2016, 5:01 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, Joseph Wu, 
> and Qian Zhang.
> 
> 
> Bugs: MESOS-4441
> https://issues.apache.org/jira/browse/MESOS-4441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6 
>   src/master/allocator/mesos/hierarchical.cpp 
> e32ee4aa3ed9793bb5a99233e699e5cc2bdd796b 
>   src/tests/hierarchical_allocator_tests.cpp 
> 953712149bd951789beb29c72779c4ac65aa48dc 
> 
> Diff: https://reviews.apache.org/r/42559/diff/
> 
> 
> Testing
> ---
> 
> On Mac OS 10.10.4:
> 
> `GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh 
> --gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle`
> `make check`
> 
> Updated `HierarchicalAllocatorTest.DRFWithQuota` succeeds with the change and 
> fails without.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 42575: Removed superfluous word from quota.md.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [42575]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 20, 2016, 11:17 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42575/
> ---
> 
> (Updated Jan. 20, 2016, 11:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and Neil 
> Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed superfluous word from quota.md.
> 
> 
> Diffs
> -
> 
>   docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6 
> 
> Diff: https://reviews.apache.org/r/42575/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 42559: Allocated resources as revocable beyond quota.

[Guangya Liu]

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


There maybe risk if we want to catch up with 0.27.0 for this.


src/master/allocator/mesos/hierarchical.cpp (lines 1311 - 1314)


I would suggest adding a new revocable type here, according to the design 
of MESOS-1607, this API will set resources as `USAGE SLACK` which means 
`resources allocated but under-utilized`, but here giving `USAGE SLACK` another 
meaning: resources beyond quota which does not fit into the `USAGE SLACK` well.



src/master/allocator/mesos/hierarchical.cpp (line 1313)


I would suggest adding a new revocable type here, according to the design 
of MESOS-1607, this API will set resources as `USAGE SLACK` which means 
`resources allocated but under-utilized`, but here giving `USAGE SLACK` another 
meaning: resources beyond quota which does not fit into the `USAGE SLACK` well.



src/master/allocator/mesos/hierarchical.cpp (line 1329)


The filter will not work after you set the resources as revocable.



src/tests/hierarchical_allocator_tests.cpp (line 1621)


You may encounter some problem if you add some static reserved resources.


- Guangya Liu


On 一月 20, 2016, 5:01 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42559/
> ---
> 
> (Updated 一月 20, 2016, 5:01 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, Joseph Wu, 
> and Qian Zhang.
> 
> 
> Bugs: MESOS-4441
> https://issues.apache.org/jira/browse/MESOS-4441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6 
>   src/master/allocator/mesos/hierarchical.cpp 
> e32ee4aa3ed9793bb5a99233e699e5cc2bdd796b 
>   src/tests/hierarchical_allocator_tests.cpp 
> 953712149bd951789beb29c72779c4ac65aa48dc 
> 
> Diff: https://reviews.apache.org/r/42559/diff/
> 
> 
> Testing
> ---
> 
> On Mac OS 10.10.4:
> 
> `GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh 
> --gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle`
> `make check`
> 
> Updated `HierarchicalAllocatorTest.DRFWithQuota` succeeds with the change and 
> fails without.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 42517: Added discussion about allowing multiple frameworks in a role.

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On 一月 20, 2016, 9:57 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42517/
> ---
> 
> (Updated 一月 20, 2016, 9:57 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added discussion about allowing multiple frameworks in a role.
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md 4af7d6e8dc648fb630f56db5fbad1b7b438ebcfb 
>   docs/roles.md 609a63cbff2d9c652af45ba16152ce3caf48 
> 
> Diff: https://reviews.apache.org/r/42517/diff/
> 
> 
> Testing
> ---
> 
> Previewed on github.
> 
> Note that the link to `roles.md` doesn't work at the moment, but I believe it 
> should work once Joerg's fix for the `Rakefile` is merged.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 42559: Allocated resources as revocable beyond quota.

[Guangya Liu]

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



src/master/allocator/mesos/hierarchical.cpp (line 1310)


The `resource` will include some `reserved` resources, here even if you 
call `resource.mutable_revocable();`, the `reserved` resources will still be 
considered as `reserved`.

Take a look at isReserved()

bool Resources::isReserved(
const Resource& resource,
const Option& role)
{
  if (role.isSome()) {
return !isUnreserved(resource) && role.get() == resource.role();
  } else {
return !isUnreserved(resource);
  }
}

bool Resources::isUnreserved(const Resource& resource)
{
  return resource.role() == "*" && !resource.has_reservation();
}

For a resources such as `cpus(r1):100`, after call 
`resource.mutable_revocable();`, it will be `cpus(r1){REV}:100`, and the helper 
function `isReserved(resources, role)` still treate this as `reserved` 
resources but not `revocable` resources.

This has highly dependency and interaction with MESOS-1607,the optimisitic 
offer phase 1, in phase 1, we are planning to treate the `reserved but not 
used` as `allocation slack`, it is not right if you simply translating all 
`resources` to `revocable`, you may want at least filter out the `reserved` 
resources when translating to `revocalbe` resources.

I would suggest you also invite bmahler as review as he may be the shephard 
for MESOS-1607


- Guangya Liu


On 一月 20, 2016, 5:01 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42559/
> ---
> 
> (Updated 一月 20, 2016, 5:01 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, Joseph Wu, 
> and Qian Zhang.
> 
> 
> Bugs: MESOS-4441
> https://issues.apache.org/jira/browse/MESOS-4441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6 
>   src/master/allocator/mesos/hierarchical.cpp 
> e32ee4aa3ed9793bb5a99233e699e5cc2bdd796b 
>   src/tests/hierarchical_allocator_tests.cpp 
> 953712149bd951789beb29c72779c4ac65aa48dc 
> 
> Diff: https://reviews.apache.org/r/42559/diff/
> 
> 
> Testing
> ---
> 
> On Mac OS 10.10.4:
> 
> `GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh 
> --gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle`
> `make check`
> 
> Updated `HierarchicalAllocatorTest.DRFWithQuota` succeeds with the change and 
> fails without.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41855: Trace pending executors.

[Joseph Wu]

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



src/slave/slave.cpp (lines 1384 - 1388)


You probably don't need this additional tracking.  

It looks like you're inserting the executor eviction logic inside the code 
path for running executors.  i.e.:
`::runTask` -> (evict executors logic) ->
`::_runTask` -> `::launchExecutor`

All of these things happen inside the Agent actor and the eviction logic 
can be mostly synchronous.  So you can just pass the executor-to-be-launched 
around.

(The less state you save == less error prone the function is.)


- Joseph Wu


On Jan. 9, 2016, 1:38 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41855/
> ---
> 
> (Updated Jan. 9, 2016, 1:38 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Guangya Liu, Artem Harutyunyan, Joris 
> Van Remoortere, Joseph Wu, and Jian Qiu.
> 
> 
> Bugs: MESOS-3892
> https://issues.apache.org/jira/browse/MESOS-3892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Trace the executor that are not launched; after launching, it's removed from 
> pendingExecutor list.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp b7586ce42bfac9d9885a3eb8d82deb94680c236c 
>   src/slave/slave.cpp 90d0fecd2d83fd174134870a577ac59d79c0006f 
> 
> Diff: https://reviews.apache.org/r/41855/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 42546: Updated `Master::Http::stateSummary` to use `jsonify`.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [42543, 42546]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 20, 2016, 6:44 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42546/
> ---
> 
> (Updated Jan. 20, 2016, 6:44 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 34a70ee50553492fc8c3947497ab5922f4379d72 
> 
> Diff: https://reviews.apache.org/r/42546/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Review Request 42580: Updated changelog :renaming of 'authenticate' to 'authenticate_frameworks' flag

[Disha Singh]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Updated changelog :renaming of 'authenticate' to 'authenticate_frameworks' flag


Diffs
-

  CHANGELOG 912a402 

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


Testing
---

Not-required.Not a functional change.


Thanks,

Disha  Singh

Re: Review Request 42389: Fixed unmount order in linux filesystem isolator cleanup.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Jan. 19, 2016, 12:10 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42389/
> ---
> 
> (Updated Jan. 19, 2016, 12:10 a.m.)
> 
> 
> Review request for mesos, haosdent huang and Jie Yu.
> 
> 
> Bugs: MESOS-3379
> https://issues.apache.org/jira/browse/MESOS-3379
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed unmount order in linux filesystem isolator cleanup.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> ace9e305c24a9841f1716c9bf40cd13b16ef0cec 
> 
> Diff: https://reviews.apache.org/r/42389/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 42239: Added tests for the Docker URI fetcher plugin.

[Jie Yu]

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

(Updated Jan. 21, 2016, 12:41 a.m.)


Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and Timothy 
Chen.


Changes
---

Review comments.


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


Repository: mesos


Description
---

Added tests for the Docker URI fetcher plugin.


Diffs (updated)
-

  src/tests/uri_fetcher_tests.cpp c3bc4eaa26edf2c4e827341794b230d022a91283 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 42239: Added tests for the Docker URI fetcher plugin.

[Jie Yu]

> On Jan. 18, 2016, 11:02 p.m., Timothy Chen wrote:
> > src/tests/uri_fetcher_tests.cpp, line 216
> > 
> >
> > Should we create a constant for docker registry server url? We're 
> > referring to it in multiple places and I think we could then update it if 
> > Docker decides to change it.

Created a constant, and added a TODO to expose it.


- Jie


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


On Jan. 21, 2016, 12:41 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42239/
> ---
> 
> (Updated Jan. 21, 2016, 12:41 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-4296
> https://issues.apache.org/jira/browse/MESOS-4296
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for the Docker URI fetcher plugin.
> 
> 
> Diffs
> -
> 
>   src/tests/uri_fetcher_tests.cpp c3bc4eaa26edf2c4e827341794b230d022a91283 
> 
> Diff: https://reviews.apache.org/r/42239/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 42517: Added discussion about allowing multiple frameworks in a role.

[Alexander Rukletsov]

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

Ship it!


Ship It!

- Alexander Rukletsov


On Jan. 20, 2016, 9:57 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42517/
> ---
> 
> (Updated Jan. 20, 2016, 9:57 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added discussion about allowing multiple frameworks in a role.
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md 4af7d6e8dc648fb630f56db5fbad1b7b438ebcfb 
>   docs/roles.md 609a63cbff2d9c652af45ba16152ce3caf48 
> 
> Diff: https://reviews.apache.org/r/42517/diff/
> 
> 
> Testing
> ---
> 
> Previewed on github.
> 
> Note that the link to `roles.md` doesn't work at the moment, but I believe it 
> should work once Joerg's fix for the `Rakefile` is merged.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 42575: Removed superfluous word from quota.md.

[Alexander Rukletsov]

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

Ship it!


Ship It!

- Alexander Rukletsov


On Jan. 20, 2016, 11:17 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42575/
> ---
> 
> (Updated Jan. 20, 2016, 11:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and Neil 
> Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed superfluous word from quota.md.
> 
> 
> Diffs
> -
> 
>   docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6 
> 
> Diff: https://reviews.apache.org/r/42575/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 42516: Add support for user-defined networks.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [42516]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 20, 2016, 12:25 p.m., Ezra Silvera wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42516/
> ---
> 
> (Updated Jan. 20, 2016, 12:25 p.m.)
> 
> 
> Review request for mesos and TimothyIL TimothyIL.
> 
> 
> Bugs: MESOS-4369
> https://issues.apache.org/jira/browse/MESOS-4369
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Signed-off-by: Ezra Silvera 
> 
> Review: https://reviews.apache.org/r/42549
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 0be4bed336e86a5c377e87ac6212c70ac3b4c66b 
>   include/mesos/v1/mesos.proto c3244e87f9351c71312d2eace7a49bcac926fafd 
>   src/docker/docker.cpp 4d2f1fa14f4450b8fa3401081bf52807d2e79a7e 
> 
> Diff: https://reviews.apache.org/r/42516/diff/
> 
> 
> Testing
> ---
> 
> Using Swarm running on Mesos create a network with "docker  network create 
> --driver=bridge myNetwork"   and then create a container on that network:  
> "docker run --net=myNetwork"
> 
> 
> Thanks,
> 
> Ezra Silvera
> 
>

Review Request 42575: Removed superfluous word from quota.md.

[Joerg Schad]

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

Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and Neil 
Conway.


Repository: mesos


Description
---

Removed superfluous word from quota.md.


Diffs
-

  docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6 

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


Testing
---


Thanks,

Joerg Schad

Re: Review Request 41850: Add map to trace evictable executors.

[Joseph Wu]

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



src/slave/slave.cpp (line 1599)


The way you're using this helper suggests that all executors are evictable 
(which they are not).

If you only use the helpers once, it may be better to put the logic in here 
directly.
Similar for `removeEvictableExecutor`.


- Joseph Wu


On Jan. 7, 2016, 10:53 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41850/
> ---
> 
> (Updated Jan. 7, 2016, 10:53 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Guangya Liu, Artem Harutyunyan, Joris 
> Van Remoortere, Joseph Wu, and Jian Qiu.
> 
> 
> Bugs: MESOS-3892
> https://issues.apache.org/jira/browse/MESOS-3892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Before getting evictable executors for evicting, it need to trace which 
> execuotor can be evicted.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp b7586ce42bfac9d9885a3eb8d82deb94680c236c 
>   src/slave/slave.cpp 9d80c96d8e28085c7fa47ce21b9b055c0926d12c 
> 
> Diff: https://reviews.apache.org/r/41850/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 41791: Updated allocation slack when dynamic reserve new resources (1/3).

[Joseph Wu]

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



src/master/allocator/mesos/hierarchical.cpp (line 662)


Have you considered modifying `Resources::apply` to "create" allocation 
slack upon a reservation?  (And to "destroy" allocation slack upon unreserve?)


- Joseph Wu


On Jan. 19, 2016, 10:38 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41791/
> ---
> 
> (Updated Jan. 19, 2016, 10:38 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, Joseph Wu, Klaus Ma, and Jian Qiu.
> 
> 
> Bugs: MESOS-4145
> https://issues.apache.org/jira/browse/MESOS-4145
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update allocation slack resources if reserve some new
> stateless reserved resources. For such case, the allocation
> slack resources will be increased.
> 
> This patch updates both `updateAvailable` and `updateAllocation`
> when reserving some new resources.
> 
> There are three patches handling dynamic reservation and oo phase 1.
> 1) https://reviews.apache.org/r/41791 Reserve new resources via 
> `updateAllocation` and `updateAvailable`. (1/3).
> 2) https://reviews.apache.org/r/42113 Unreserve resources via 
> `updateAllocation` (2/3).
> 3) https://reviews.apache.org/r/42194 Unreserve resources via 
> `updateAvailable` (3/3).
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> d541bfa3f4190865c65d35c9d1ffdb8a3f194056 
>   src/tests/hierarchical_allocator_tests.cpp 
> e044f832c2c16e53e663c6ced5452649bb0dcb59 
> 
> Diff: https://reviews.apache.org/r/41791/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 41791: Updated allocation slack when dynamic reserve new resources (1/3).

[Joseph Wu]

> On Jan. 19, 2016, 3:34 p.m., Joseph Wu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 676
> > 
> >
> > I see several places where you overwrite changes from this review in 
> > the subsequent two.  Did you mean to have three reviews for the changes to 
> > `::updateAllocation`, `::updateAvailable`, and `::recoverResources`?
> 
> Guangya Liu wrote:
> There are three patches handling dynamic reservation and 
> 1) https://reviews.apache.org/r/41791 Reserve new resources via 
> `updateAllocation` and `updateAvailable`. (1/3).
> 2) https://reviews.apache.org/r/42113 Unreserve resources via 
> `updateAllocation` (2/3).
> 3) https://reviews.apache.org/r/42194 Unreserve resources via 
> `updateAvailable` (3/3).
> 
> I have updated the description here.

It doesn't seem like you need 3 patches.

I'd recommend a couple of cleanups:

* Add a helper for all the duplicate logic you have.  Both `updateAllocation` 
and `updateAvailable` deal with reservations, in essentially the same way (per 
optimistic offers).
* Consolidate the overlapping changes into a single patch.  Just don't 
overwrite your own changes.
* Consider a different approach than subtracting allocation slack on unreserve. 
 You could perhaps, for accounting purposes, elevate a portion of the 
allocation from revocable to non-revocable.


- Joseph


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


On Jan. 19, 2016, 10:38 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41791/
> ---
> 
> (Updated Jan. 19, 2016, 10:38 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, Joseph Wu, Klaus Ma, and Jian Qiu.
> 
> 
> Bugs: MESOS-4145
> https://issues.apache.org/jira/browse/MESOS-4145
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update allocation slack resources if reserve some new
> stateless reserved resources. For such case, the allocation
> slack resources will be increased.
> 
> This patch updates both `updateAvailable` and `updateAllocation`
> when reserving some new resources.
> 
> There are three patches handling dynamic reservation and oo phase 1.
> 1) https://reviews.apache.org/r/41791 Reserve new resources via 
> `updateAllocation` and `updateAvailable`. (1/3).
> 2) https://reviews.apache.org/r/42113 Unreserve resources via 
> `updateAllocation` (2/3).
> 3) https://reviews.apache.org/r/42194 Unreserve resources via 
> `updateAvailable` (3/3).
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> d541bfa3f4190865c65d35c9d1ffdb8a3f194056 
>   src/tests/hierarchical_allocator_tests.cpp 
> e044f832c2c16e53e663c6ced5452649bb0dcb59 
> 
> Diff: https://reviews.apache.org/r/41791/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 41726: Implement os::memory() for FreeBSD.

[Ian Downes]

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

Ship it!


Ship It!

- Ian Downes


On Jan. 7, 2016, 4:52 p.m., David Forsythe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41726/
> ---
> 
> (Updated Jan. 7, 2016, 4:52 p.m.)
> 
> 
> Review request for mesos and Ian Downes.
> 
> 
> Bugs: MESOS-4251
> https://issues.apache.org/jira/browse/MESOS-4251
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement os::memory() for FreeBSD.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> 4cf693fb7e8c6bb3ad1920ebe90d61f0adb5dc99 
> 
> Diff: https://reviews.apache.org/r/41726/diff/
> 
> 
> Testing
> ---
> 
> gmake check
> 
> 
> Thanks,
> 
> David Forsythe
> 
>

Re: Review Request 42559: Allocated resources as revocable beyond quota.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [42559]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 20, 2016, 5:01 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42559/
> ---
> 
> (Updated Jan. 20, 2016, 5:01 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, Joseph Wu, 
> and Qian Zhang.
> 
> 
> Bugs: MESOS-4441
> https://issues.apache.org/jira/browse/MESOS-4441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6 
>   src/master/allocator/mesos/hierarchical.cpp 
> e32ee4aa3ed9793bb5a99233e699e5cc2bdd796b 
>   src/tests/hierarchical_allocator_tests.cpp 
> 953712149bd951789beb29c72779c4ac65aa48dc 
> 
> Diff: https://reviews.apache.org/r/42559/diff/
> 
> 
> Testing
> ---
> 
> On Mac OS 10.10.4:
> 
> `GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh 
> --gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle`
> `make check`
> 
> Updated `HierarchicalAllocatorTest.DRFWithQuota` succeeds with the change and 
> fails without.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41283: Introduced an Executor Library based on the new executor HTTP API.

[Anand Mazumdar]

> On Jan. 14, 2016, 10:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 123
> > 
> >
> > The name of this struct is weird considering it is encapsulating two 
> > connections.
> > 
> > s/HttpConnection/connections/ ?
> > 
> > Also, do we really need a struct?

Dropped this `struct` and instead used two members for the `Connection` object 
called `subscribe`/`nonSubscribe`.


> On Jan. 14, 2016, 10:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, lines 179-183
> > 
> >
> > is mesos_slave_pid the only way an executor is going to know about the 
> > http endpoint to connect? i think we need to have a better env variable 
> > because 'pid' is a relic of the old world, which doesn't make sense in the 
> > http world.
> > 
> > wasn't there an MESOS_AGENT_ENDPOINT?

Yep, there is. But we need `MESOS_SLAVE_PID` for testing since we don't have 
delegates set for `slave(X)` -> root i.e. `/api/v1/executor`. As per our 
discussion, I would file a separate JIRA for this since the scheduler also uses 
the `PID` from the master currently.


> On Jan. 14, 2016, 10:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, lines 282-287
> > 
> >
> > This definitely needs a comment here. IIUC, you are opening up two 
> > connections here one for subscribe and another for everything else.
> > 
> > More importantly, why are you opening both connections here? I would've 
> > expected to only open the subscribe connection if it's a subscribe calla 
> > and open a non-subscribe one if/when it's a nonsubscribe call.

Fixed. Now we create the `Subscribe` connection initially and invoke the 
`connected` callback thereafter. We only create the second connection on a need 
basis and don't invoke the `disconnected` callback if it breaks. The 
`disconnected` callback is only invoked for the `Subscribe` connection.


> On Jan. 14, 2016, 10:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 334
> > 
> >
> > why do we wait for `recoveryTimeout` if the agent is disconnected and 
> > we are not checkpointing?

I was initially relying on `recoveryTimeout` to be `0` initially. Changed it to 
be an `Option`.


> On Jan. 14, 2016, 10:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 382
> > 
> >
> > s/been/seen/ ?

This wasn't clear to me. This comment is similar to the already existing one in 
`src/exec/exec.cpp`. Feel free to reopen.


- Anand


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


On Jan. 20, 2016, 10:11 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41283/
> ---
> 
> (Updated Jan. 20, 2016, 10:11 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-3550
> https://issues.apache.org/jira/browse/MESOS-3550
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces the implementation for the executor library. 
> 
> This uses the new HTTP Connection interface to ensure calls are properly 
> pipelined.
> 
> `connected` -> Callback invoked when a TCP connection is established with the 
> agent for the `Subscribe` call.
> `disconnected` -> When the `Subscribe` TCP connection is interrupted possibly 
> due to an agent restart.
> `received` -> When the executor receives events from the agent upon 
> subscribing.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am d23e35001078a86775bd9b76baa207ecb9dab7e1 
>   src/executor/executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41283/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 41283: Introduced an Executor Library based on the new executor HTTP API.

[Anand Mazumdar]

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

(Updated Jan. 20, 2016, 10:11 p.m.)


Review request for mesos, Ben Mahler and Vinod Kone.


Changes
---

Review comments


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


Repository: mesos


Description (updated)
---

This change introduces the implementation for the executor library. 

This uses the new HTTP Connection interface to ensure calls are properly 
pipelined.

`connected` -> Callback invoked when a TCP connection is established with the 
agent for the `Subscribe` call.
`disconnected` -> When the `Subscribe` TCP connection is interrupted possibly 
due to an agent restart.
`received` -> When the executor receives events from the agent upon subscribing.


Diffs (updated)
-

  src/Makefile.am d23e35001078a86775bd9b76baa207ecb9dab7e1 
  src/executor/executor.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 42517: Added discussion about allowing multiple frameworks in a role.

[Neil Conway]

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

(Updated Jan. 20, 2016, 9:57 p.m.)


Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.


Repository: mesos


Description
---

Added discussion about allowing multiple frameworks in a role.


Diffs
-

  docs/persistent-volume.md 4af7d6e8dc648fb630f56db5fbad1b7b438ebcfb 
  docs/roles.md 609a63cbff2d9c652af45ba16152ce3caf48 

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


Testing (updated)
---

Previewed on github.

Note that the link to `roles.md` doesn't work at the moment, but I believe it 
should work once Joerg's fix for the `Rakefile` is merged.


Thanks,

Neil Conway

Re: Review Request 42517: Added discussion about allowing multiple frameworks in a role.

[Neil Conway]

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

(Updated Jan. 20, 2016, 9:56 p.m.)


Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.


Changes
---

Address review comments.


Repository: mesos


Description
---

Added discussion about allowing multiple frameworks in a role.


Diffs (updated)
-

  docs/persistent-volume.md 4af7d6e8dc648fb630f56db5fbad1b7b438ebcfb 
  docs/roles.md 609a63cbff2d9c652af45ba16152ce3caf48 

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


Testing
---

Previewed on github.


Thanks,

Neil Conway

Re: Review Request 42255: Updated user documentation around HTTP response codes.

[Till Toenshoff]

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

Ship it!


Ship It!

- Till Toenshoff


On Jan. 18, 2016, 11:46 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42255/
> ---
> 
> (Updated Jan. 18, 2016, 11:46 a.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Till Toenshoff.
> 
> 
> Bugs: MESOS-4305
> https://issues.apache.org/jira/browse/MESOS-4305
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated user documentation around HTTP response codes.
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md f969975f242f7fc7a8b7a89e3e211d2201c9114a 
>   docs/reservation.md 57cd92ce5d8d25b3393763614e40155e17e8c06e 
> 
> Diff: https://reviews.apache.org/r/42255/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41847: Updated allocation slack when slave was updated.

[Joseph Wu]

> On Jan. 15, 2016, 6:21 p.m., Joseph Wu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 529-535
> > 
> >
> > What if you did this?
> > ```
> > slaves[slaveId]total = slaves[slaveId].total.nonUsageSlack() + 
> > oversubscribed;
> > ```
> > 
> > Where `nonUsageSlack` is a helper that filters out usage slack.  (You 
> > could just use a Resource filter directly here.)
> > 
> > ---
> > 
> > This would have fewer operations too (1 filter operation instead of 2).
> 
> Guangya Liu wrote:
> I think that the current logic is that when 
> enableReservationOversubscription is true, we only need to add the allocation 
> slack to the total resources, there is no other change except this.
> 
> The `allocationSlack()` also has only 1 filter `isAllocationSlack` , 
> comments?
> 
> Joseph Wu wrote:
> I'm talking about the `.nonRevocable()` and the `allocationSlack()` 
> filters.
> 
> The current patch is a pretty roundabout way of writing this:
> ```
> slaves[slaveId].total = oversubscribed + 
>   slaves[slaveId].total.filter([](const Resource& resource) { return 
> !isUsageSlack(resource); });
> ```
> 
> ^ This makes it explicit that we're just resetting the `USAGE_SLACK` 
> component and leaving everything else untouched.
> 
> Guangya Liu wrote:
> My logic here is that for update `updateSlave`, do not touch other logic 
> but only add the `allocationSlack` when `enableReservationOversubscription` 
> is enabled, this may be cleaner? As I was only adding `allocationSlack` when 
> `enableReservationOversubscription` is enabled.

The `enableReservationOversubscription` flag shouldn't even matter here.  If 
you really want to be strict, you can add this:
```
if (enableReservationOversubscription) {
  CHECK_EQ(Resources(), slaves[slaveId].total.allocationSlack());
}
```


- Joseph


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


On Jan. 13, 2016, 4:55 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41847/
> ---
> 
> (Updated Jan. 13, 2016, 4:55 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, Joseph Wu, Klaus Ma, and Jian Qiu.
> 
> 
> Bugs: MESOS-4145
> https://issues.apache.org/jira/browse/MESOS-4145
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated allocation slack when slave was updated.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> d541bfa3f4190865c65d35c9d1ffdb8a3f194056 
>   src/tests/hierarchical_allocator_tests.cpp 
> e044f832c2c16e53e663c6ced5452649bb0dcb59 
> 
> Diff: https://reviews.apache.org/r/41847/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> GLOG_v=2 ./bin/mesos-tests.sh  --gtest_filter="HierarchicalAllocatorTest.*" 
> --verbose
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 40632: Enabled oversubscribed resources for reservations in allocator.

[Joseph Wu]

> On Jan. 15, 2016, 6:11 p.m., Joseph Wu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1291-1319
> > 
> >
> > None of this should be necessary:
> > 
> > 1) You should have all the allocation slack inside `available`.
> > 2) We don't have allocation slack ACLs in the MVP, so all allocation 
> > slack should fall under `available.unreserved`.  If we do have ACLs, some 
> > of the allocation slack will be accounted for in `available.reserved(role)`.
> > 3) You shouldn't need to recalculate this during `::allocate`.
> > 
> > If any of the above are not true, you probably have a problem in the 
> > `Resource` helpers or in `::addSlave`.
> 
> Guangya Liu wrote:
> We can take a look at the following cases:
> 1) total resources cpus(r1):100
> 2) register a framework f1, cycle 1: allocated 
> cpus(r1):100;cpu(*){ALLOCATION_SLACK}:100 to f1
> 3) f1 recovered cpu(*){ALLOCATION_SLACK}:100 back
> 4) register another framwork f2, f2 get nothing here as f1 used up all 
> reserved resoures and there is no allocation slack now.
> 
> The above logic make sure that step 4) can always get the latest 
> remaining allocation slack.
> 
> Joseph Wu wrote:
> The step 3) in your case is presumably because you haven't implemented 
> all the allocator methods yet (as of this review in the chain), like 
> `recoverResources`, `updateAllocation`, and `updateAvailable`.
> 
> If so, you should consider moving the test additions/changes before/after 
> (this lets us know what your intended behavior is) the allocator changes.
> 
> Guangya Liu wrote:
> I was updating the comments here to make it more clear.
> 
> // Calculate the `remainingAllocationSlack` if the framework can
> // use revocable resources and reservation oversubscription also
> // enabled. The `remainingAllocationSlack` need to exclude the
> // stateless reserved resources allocated in previous allocation
> // cycle.

I would still **highly recommend** that you don't recalculate the allocation 
slack inside a doubly-nested for-loop.

And I'll reinterate: if you can't find the allocation slack inside the 
`available` Resources object, then you're doing something wrong.  You should 
probably double-check if this is the case.


- Joseph


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


On Jan. 19, 2016, 10:25 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40632/
> ---
> 
> (Updated Jan. 19, 2016, 10:25 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, Joseph Wu, Klaus Ma, and Jian Qiu.
> 
> 
> Bugs: MESOS-4145
> https://issues.apache.org/jira/browse/MESOS-4145
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled oversubscribed resources for reservations in allocator.
> The allocator part including 5 patches:
> 1) https://reviews.apache.org/r/40632 Enabled oversubscribed resources for 
> reservations in allocator
> 2) https://reviews.apache.org/r/41847 Updated allocation slack when slave was 
> updated.
> 3) https://reviews.apache.org/r/41791 Updated allocation slack for dynamic 
> reserve (1/3).
> 4) https://reviews.apache.org/r/42113 Handle unreserve logic for dynamic 
> reservation (2/3).
> 5) https://reviews.apache.org/r/42194 Handle unreserve logic for dynamic 
> reservation (3/3).
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> e32ee4aa3ed9793bb5a99233e699e5cc2bdd796b 
>   src/tests/hierarchical_allocator_tests.cpp 
> 953712149bd951789beb29c72779c4ac65aa48dc 
> 
> Diff: https://reviews.apache.org/r/40632/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> GLOG_v=2  ./bin/mesos-tests.sh  --gtest_filter="HierarchicalAllocatorTest.*" 
> --verbose --gtest_repeat=100 --gtest_shuffle
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 41772: Added helper function to flatten resources.

[Joseph Wu]

> On Jan. 19, 2016, 2:18 p.m., Joseph Wu wrote:
> > src/common/resources.cpp, lines 874-877
> > 
> >
> > This is pretty much a copy of `Resources::reserved` now.  You can 
> > remove it.
> 
> Guangya Liu wrote:
> The reason that I keep this is because the current `Resources::reserved` 
> returns a hashmap but what I need here is a flatten string for resources in 
> different roles. e.g. cpus(r1):100;cpus(r2):200
> 
> I updated the comments here to clarify why this is needed.
> 
> Klaus Ma wrote:
> I think it's more general to add a `Resources` constructor to accept 
> hashmap, so it'll be `Resources(res.reserved()).flatten()`.

It doesn't look like `reserved()` is used very heavily.  And the places that do 
use it could easily use a normal `Resources` object.  I'd recommend changing 
`reserved()` or just using `reserved("*")`.


> On Jan. 19, 2016, 2:18 p.m., Joseph Wu wrote:
> > src/common/resources.cpp, line 880
> > 
> >
> > It should be fine to just name this `flatten`.
> > 
> > You should also consider changing the parameter type from 
> > `RevocableInfo::Type` to just `RevocableInfo`.  Supplying the whole 
> > `RevocableInfo` will save us another tweak to the helpers if we change the 
> > revocable protobufs again.
> 
> Guangya Liu wrote:
> I think that we cannot rename it to `flatten` as the compiler will not 
> know what does `flatten()` mean.
> 
> Klaus Ma wrote:
> Regarding from `RevocableInfo::Type` to `RevocableInfo`, do you mean 
> change `RevocableInfo` from `message` to `enum`? I think it's better to 
> define const in `Resources` instead of changing `RevocableInfo`'s type, e.g. 
> `static const Resource::RevocableInfo::Type Resources::ALLOCATION_SLACK = 
> Resource::RevocableInfo::Type::ALLOCATION_SLACK`.
> 
> Regarding `flatten`, there're two options to me:
> 
> * Option 1: add `RevocableInfo::Type` to as 3rd parameter in current 
> `flatten` with `None()` as default vaule, and a new helper function to accept 
> `RevocableInfo::Type` only without default value. The sample code is:
> 
> ```
> Resources flatten(string role = "*", Option 
> resveration = None(), Option revocable = None()) {
>   ...
>   if (revocable.isSome()) {
> flatten(revocable.get());
>   } else {
> // clear revocable info.
>   }
> }
> 
> Resources flatten(RevocableInfo::Type revocable) {
>   foreach resources {
> resource.mutable_revocable()->set_type(revocable);
>   }
> }
> ```
> 
> * Option 2: add new `flatten` functions but did not provide default 
> value; `None()` means clear the `RevocableInfo::Type`.
> 
> I perfer to Option 1 :). Any comments?
> 
> Guangya Liu wrote:
> option 1 will cause all caller who want to flatten allocation slack need 
> to input all of the four parameter, it is really not convenient for the 
> caller.
> Option 2 need to add a new `flatten` with a `requested` but not `option` 
> parameter, I think that maybe 2 is better.
> 
> Klaus Ma wrote:
> regarding 1, if want to clear the flags (role, reservation and 
> revocable), call `flatten()`; if want to set `ALLOCATION_SLACK`, call 
> `flatten(Resources::ALLOCATION_SLACK)`.
> 
> Guangya Liu wrote:
> Just clarify 1) here, I have some mis-understanding for it.
> 
> Option 1) has involved two APIs, one is update current `Resources 
> flatten(string role = "*", Option resveration = None(), , 
> Option revocable = None() )`, the other is add a new 
> `flatten(RevocableInfo::Type revocable)`. The `first flatten` is used to 
> clear `allocation slack` and the `second flatten` is used to set `allocation 
> slack` flag.
> 
> Seems 1) is better as it faciliates the caller.

Given how you're using `flatten`, it would be better to have two calls:
`Resources flatten(string role = "*", Option resveration = 
None())`
and 
`Resources flatten(Option revocable)`  // No default so there's 
no ambiguity.

---

Note: If you wanted a single `flatten`, you'd need to have 
`Option>` so that you have the option of setting, 
un-setting, or not-changing each field.


- Joseph


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


On Jan. 19, 2016, 7:40 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> ---
> 
> (Updated Jan. 19, 2016, 7:40 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 

Re: Review Request 42505: Multiple Disk: Adjusted DiskInfo validation.

[Jie Yu]

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



src/master/validation.cpp (lines 206 - 210)


Also, master valiation is not sufficient because operators can specify it 
through --resources as well. Can we move this check to Resources::validate.


- Jie Yu


On Jan. 19, 2016, 3:53 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42505/
> ---
> 
> (Updated Jan. 19, 2016, 3:53 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 98360b690382ed1912a868ac93b058cb28003a12 
> 
> Diff: https://reviews.apache.org/r/42505/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 42504: Multiple Disk: Modified 'DiskInfo' stripping logic.

[Jie Yu]

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



src/common/resources.cpp (line 968)


Let's strip the volume as well.



src/common/resources.cpp (line 1006)


Ditto. Strip volume as well.



src/common/resources_utils.cpp (line 64)


strip volume as well.



src/tests/mesos.hpp (lines 545 - 546)


Hum, we should not create DiskInfo if it's not persistent volume and no 
source is specifed?



src/tests/persistent_volume_tests.cpp (line 94)


See my comments above. You don't need this.



src/v1/resources.cpp (line 962)


Ditto on stripping volume.



src/v1/resources.cpp (line 1000)


Same here.


- Jie Yu


On Jan. 20, 2016, 7:18 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42504/
> ---
> 
> (Updated Jan. 20, 2016, 7:18 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored persistent volume tests to use a helper to generate disk
> resources.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
>   src/common/resources_utils.cpp d64db54a175299bf7c32ecbeeb1ce9a7afb1c88a 
>   src/tests/mesos.hpp 3d9ebc6c9dc3cd1be02dc3771fbd847386907fac 
>   src/tests/persistent_volume_tests.cpp 
> 7acf7ab29d64d891f3288f8042d267dcc82a32e9 
>   src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666 
>   src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 
> 
> Diff: https://reviews.apache.org/r/42504/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 41491: Exposed docker image manifest to mesos containerizer.

[Gilbert Song]

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

(Updated Jan. 20, 2016, 12:19 p.m.)


Review request for mesos, Artem Harutyunyan, Jie Yu, Qian Zhang, and Timothy 
Chen.


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


Repository: mesos


Description
---

Exposed docker image manifest to mesos containerizer.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
03425daaf015cf231920b7eda1d5424895a41286 
  src/slave/containerizer/mesos/provisioner/provisioner.hpp 
4202f5552174ad3ab595ab3f16ab91d0854951f0 
  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
023f7363f33c4a927fae11037a408f6747fc3c39 
  src/slave/containerizer/mesos/provisioner/store.hpp 
aec725f789f7aeb92abfcc6718c2e6e2f1f37981 
  src/tests/containerizer/provisioner_docker_tests.cpp 
f81f0039dc12d09d85fda0be345d1509b4c6a664 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 42503: Multiple Disk: Added Resource arithmetic tests.

[Jie Yu]

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

Ship it!



src/tests/resources_tests.cpp (line 1846)


Maybe add a check:
```
EXPECT_EQ(r3, r3 - r1);
```


- Jie Yu


On Jan. 20, 2016, 7:16 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42503/
> ---
> 
> (Updated Jan. 20, 2016, 7:16 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Michael Park.
> 
> 
> Bugs: MESOS-4380
> https://issues.apache.org/jira/browse/MESOS-4380
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Multiple Disk: Added Resource arithmetic tests.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 3d9ebc6c9dc3cd1be02dc3771fbd847386907fac 
>   src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666 
> 
> Diff: https://reviews.apache.org/r/42503/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 42474: Multiple Disk: Updated Slave initialize to create DiskInfo paths.

[Jie Yu]

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

Ship it!



src/slave/slave.cpp (lines 369 - 372)


You may want to print the error message here (will be useful for triaging).

```
Try mkdir = os::mkdir(...);
if (mkdir.isError()) {
  EXIT(1) << "...: " << mkdir.error();
}
```


- Jie Yu


On Jan. 20, 2016, 7:15 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42474/
> ---
> 
> (Updated Jan. 20, 2016, 7:15 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Michael Park.
> 
> 
> Bugs: MESOS-4403
> https://issues.apache.org/jira/browse/MESOS-4403
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Create disk paths based on 'DiskInfo.Source' if the type is 'PATH'
> and the directory does not yet exist.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp bd7fe03f8a8b07c6201db2f876f4f9cd7dc337cf 
> 
> Diff: https://reviews.apache.org/r/42474/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 42473: Multiple Disk: Updated filesystem isolators to use 'DiskInfo.Source'.

[Jie Yu]

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

Ship it!



src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (lines 618 - 621)


Can we use CHECK here instead. I think we already validate this master.



src/slave/containerizer/mesos/isolators/filesystem/posix.cpp (lines 192 - 195)


Ditto.


- Jie Yu


On Jan. 20, 2016, 7:15 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42473/
> ---
> 
> (Updated Jan. 20, 2016, 7:15 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Michael Park.
> 
> 
> Bugs: MESOS-4402
> https://issues.apache.org/jira/browse/MESOS-4402
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Multiple Disk: Updated filesystem isolators to use 'DiskInfo.Source'.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> ace9e305c24a9841f1716c9bf40cd13b16ef0cec 
>   src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
> d914587eedc9c66bc14b4088ec211b7f0eea63ea 
> 
> Diff: https://reviews.apache.org/r/42473/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 42472: Multiple Disk: Checkpoint persistent volume based on 'DiskInfo.Source'.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Jan. 20, 2016, 7:12 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42472/
> ---
> 
> (Updated Jan. 20, 2016, 7:12 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Michael Park.
> 
> 
> Bugs: MESOS-4400
> https://issues.apache.org/jira/browse/MESOS-4400
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We now create persistent volume directories based on 'DiskInfo.Source'
> if it is present.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp bd7fe03f8a8b07c6201db2f876f4f9cd7dc337cf 
> 
> Diff: https://reviews.apache.org/r/42472/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 42557: Moved CachedImage to a separate file.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [41958, 41959, 42156, 42157]

Failed command: ./support/apply-review.sh -n -r 42157

Error:
 2016-01-20 19:44:16 URL:https://reviews.apache.org/r/42157/diff/raw/ 
[3978/3978] -> "42157.patch" [1]
error: patch failed: src/tests/containerizer/provisioner_appc_tests.cpp:253
error: src/tests/containerizer/provisioner_appc_tests.cpp: patch does not apply

- Mesos ReviewBot


On Jan. 20, 2016, 4:16 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42557/
> ---
> 
> (Updated Jan. 20, 2016, 4:16 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently CachedImage is private to the appc store. This will change as we
> implement image fetchers (eg. simple discovery).
> 
> This change will enable image fetcher to use the same structure.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 39a23df3227a4f524ea0d408dc894fa5bbab7d10 
>   src/Makefile.am 8cbfb1ba5fa49f2d3cc26ea325838a1c68a79660 
>   src/slave/containerizer/mesos/provisioner/appc/image_cache.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/appc/image_cache.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/appc/store.cpp 
> 73c4df858a70da3d4cc4a1cb15092165f6ff8fe4 
> 
> Diff: https://reviews.apache.org/r/42557/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 42027: Changed HTTP responses from Unauthorized (401) to Forbidden (403).

[Till Toenshoff]

> On Jan. 20, 2016, 7:37 p.m., Till Toenshoff wrote:
> > src/master/http.cpp, lines 549-553
> > 
> >
> > To prosterity; this means that we currently do not support mixed usage 
> > of old API and HTTP API for frameworks as soon as the master has enabled 
> > mandatory framework authentication.

Sorry, did not mean to make this an issue - dropping it to make it a comment.


- Till


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


On Jan. 20, 2016, 2:03 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42027/
> ---
> 
> (Updated Jan. 20, 2016, 2:03 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, Joerg Schad, Jan 
> Schlicht, and Till Toenshoff.
> 
> 
> Bugs: MESOS-4305
> https://issues.apache.org/jira/browse/MESOS-4305
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It is a common pattern within Mesos to return an HTTP 401 (Unauthorized) 
> response whenever the request is invalid for whatever reason. However, 
> according to the [RFC-2617 Section 
> 1.2](https://tools.ietf.org/html/rfc2617#section-1.2):
> > The 401 (Unauthorized) response message is used by an origin server  to 
> > challenge the authorization of a user agent. This response MUST include a 
> > WWW-Authenticate header field containing at least one challenge applicable 
> > to the requested resource.
> 
> Meaning that despite the confusing name, the status code _401 Unauthorized_ 
> should be used only for authentication purposes. On the other hand, the 
> [RFC-2616 Section 
> 10.4.4](http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.4) 
> states:
> > _(403 Forbidden is returned when)_ The server understood the request, but 
> > is refusing to fulfill it. Authorization will not help and the request 
> > SHOULD NOT be repeated. If the request method was not HEAD and the server 
> > wishes to make public why the request has not been fulfilled, it SHOULD 
> > describe the reason for the refusal in the entity. If the server does not 
> > wish to make this information available to the client, the status code 404 
> > (Not Found) can be used instead.
> 
> As such, _403 (Forbidden)_ seems to be a better return code when replying 
> inside endpoint handlers, while _401 (Unauthorized)_ should be left to the 
> HTTP Authenticators only.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md a928f1722dc67cd791d78ebbe4591f2e8f2e8f2a 
>   src/master/http.cpp 34a70ee50553492fc8c3947497ab5922f4379d72 
>   src/master/quota_handler.cpp f44736cd5849d4fb22a75c1238d433a1c0c9708d 
>   src/tests/master_quota_tests.cpp e8cb074c2913cafdc6b1792896f29e53f1210c9d 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> f0cce190abc90f0fae84d6c3db20e8215c2d8132 
>   src/tests/reservation_endpoints_tests.cpp 
> b8edd6fafedd4c2221a8d19c1ebc71254071a8c7 
>   src/tests/scheduler_http_api_tests.cpp 
> 143bd414c6d9ad0b7b7c23c390b7d497e4be3e6d 
>   src/tests/teardown_tests.cpp 97cc89ba168aefff8512f6d1a25c4f7ddf180bae 
> 
> Diff: https://reviews.apache.org/r/42027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 42027: Changed HTTP responses from Unauthorized (401) to Forbidden (403).

[Till Toenshoff]

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

Ship it!



src/master/http.cpp (lines 548 - 551)


To prosterity; this means that we currently do not support mixed usage of 
old API and HTTP API for frameworks as soon as the master has enabled mandatory 
framework authentication.


- Till Toenshoff


On Jan. 20, 2016, 2:03 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42027/
> ---
> 
> (Updated Jan. 20, 2016, 2:03 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, Joerg Schad, Jan 
> Schlicht, and Till Toenshoff.
> 
> 
> Bugs: MESOS-4305
> https://issues.apache.org/jira/browse/MESOS-4305
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It is a common pattern within Mesos to return an HTTP 401 (Unauthorized) 
> response whenever the request is invalid for whatever reason. However, 
> according to the [RFC-2617 Section 
> 1.2](https://tools.ietf.org/html/rfc2617#section-1.2):
> > The 401 (Unauthorized) response message is used by an origin server  to 
> > challenge the authorization of a user agent. This response MUST include a 
> > WWW-Authenticate header field containing at least one challenge applicable 
> > to the requested resource.
> 
> Meaning that despite the confusing name, the status code _401 Unauthorized_ 
> should be used only for authentication purposes. On the other hand, the 
> [RFC-2616 Section 
> 10.4.4](http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.4) 
> states:
> > _(403 Forbidden is returned when)_ The server understood the request, but 
> > is refusing to fulfill it. Authorization will not help and the request 
> > SHOULD NOT be repeated. If the request method was not HEAD and the server 
> > wishes to make public why the request has not been fulfilled, it SHOULD 
> > describe the reason for the refusal in the entity. If the server does not 
> > wish to make this information available to the client, the status code 404 
> > (Not Found) can be used instead.
> 
> As such, _403 (Forbidden)_ seems to be a better return code when replying 
> inside endpoint handlers, while _401 (Unauthorized)_ should be left to the 
> HTTP Authenticators only.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md a928f1722dc67cd791d78ebbe4591f2e8f2e8f2a 
>   src/master/http.cpp 34a70ee50553492fc8c3947497ab5922f4379d72 
>   src/master/quota_handler.cpp f44736cd5849d4fb22a75c1238d433a1c0c9708d 
>   src/tests/master_quota_tests.cpp e8cb074c2913cafdc6b1792896f29e53f1210c9d 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> f0cce190abc90f0fae84d6c3db20e8215c2d8132 
>   src/tests/reservation_endpoints_tests.cpp 
> b8edd6fafedd4c2221a8d19c1ebc71254071a8c7 
>   src/tests/scheduler_http_api_tests.cpp 
> 143bd414c6d9ad0b7b7c23c390b7d497e4be3e6d 
>   src/tests/teardown_tests.cpp 97cc89ba168aefff8512f6d1a25c4f7ddf180bae 
> 
> Diff: https://reviews.apache.org/r/42027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 42519: Fixed race between SchedDriver.{stop(), abort()} and SchedDriver.join().

[Greg Mann]

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

Ship it!


Ship It!

- Greg Mann


On Jan. 19, 2016, 10:58 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42519/
> ---
> 
> (Updated Jan. 19, 2016, 10:58 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Greg Mann.
> 
> 
> Bugs: MESOS-4409
> https://issues.apache.org/jira/browse/MESOS-4409
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, it was possible for join() to return before a schedDriver
> was actually fully stopped or aborted (breaking the semantics of the
> join() call). The race came from a short circuit in join(), which
> simply checked for status != DRIVER_RUNNING before returning. It appears
> this short circuit was introduced to handle cases where initialize() or
> start() ended up aborting before ever starting the driver to begin with.
> However, it unintentionally covers cases where stop() or abort() were
> called *after* the driver started running as well.
> 
> The problem is that stop() and abort() will change the status
> to DRIVER_STOPPED or DRIVER_ABORTED before actually processing
> dispatched stop or abort events (which happen asynchronously in a
> libprocess thread). Under normal operation, join() would wait for these
> events to trigger a latch that allowed the join() call to return.
> However, with the short circuit, join() exits immediately even if the
> libprocess thread hasn't yet processed the stop() or abort() events.
> 
> This commit fixes the semantics of the join() call to avoid this race.
> We considered removing the latch completely and replacing it with
> process.wait(), but, unlike the latch, this wouldn't ensure that stop()
> or abort() was ever called in the first place.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp 38940b7e2563a2156be2f8c228afdc27c45b6e83 
> 
> Diff: https://reviews.apache.org/r/42519/diff/
> 
> 
> Testing
> ---
> 
> Ran the entire 'make check' suite with no failures on both Mac OS X and 
> ubuntu 14.04.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 40266: Libprocess Reinitialization: Cleanup SocketManager along side ProcessManager.

[Joseph Wu]

> On Jan. 19, 2016, 6:29 p.m., haosdent huang wrote:
> > 3rdparty/libprocess/src/process.cpp, line 1306
> > 
> >
> > Is it possible sockets not empty when exit this loop?

Definitely possible.  We currently do not track the futures from 
`socket->send()` (specifically the calls in `internal::send(Encoder* encoder, 
Socket* socket)`).

i.e.
1) We call `ProcessBase::send` to a remote process.
2) Libprocess creates a socket and starts to write data.
3) We call `process::finalize` (+ this patch).
4) The main process exits, but does not necessarily clean up the socket created 
during `ProcessBase::send`.
5) SocketManager cleanup happens, which closes the socket, but doesn't finish 
sending first.

---

Seems like it would be fairly simple to track the futures from 
`internal::send(Encoder* encoder, Socket* socket)` and wait for them to finish 
during finalization.
(As is required in MESOS-4111.)


- Joseph


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


On Dec. 2, 2015, 4:50 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40266/
> ---
> 
> (Updated Dec. 2, 2015, 4:50 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3910
> https://issues.apache.org/jira/browse/MESOS-3910
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `SocketManager` and `ProcessManager` are highly inter-dependent, which 
> requires some untangling in `process::finalize`.
> 
> * Logic originally found in `~ProcessManager` has been split into 
> `ProcessManager::finalize` due to what happens during cleanup.
> * Some additional cleanup was added for dangling pointers.
> * The future from `__s__->accept()` must be explicitly discarded as libevent 
> does not detect a locally closed socket.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> a7af4671efa2f370137ed4a749e5247c91e6f95e 
> 
> Diff: https://reviews.apache.org/r/40266/diff/
> 
> 
> Testing
> ---
> 
> `make check` (libev)
> `make check` (--enable-libevent --enable-ssl)
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 42473: Multiple Disk: Updated filesystem isolators to use 'DiskInfo.Source'.

[Joris Van Remoortere]

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

(Updated Jan. 20, 2016, 7:15 p.m.)


Review request for mesos, Jie Yu and Michael Park.


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


Repository: mesos


Description (updated)
---

Multiple Disk: Updated filesystem isolators to use 'DiskInfo.Source'.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
ace9e305c24a9841f1716c9bf40cd13b16ef0cec 
  src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
d914587eedc9c66bc14b4088ec211b7f0eea63ea 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 42068: Porting 3rdparty on ppc64le.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [42068]

Failed command: ./support/apply-review.sh -n -r 42068

Error:
 2016-01-20 19:11:48 URL:https://reviews.apache.org/r/42068/diff/raw/ 
[2476/2476] -> "42068.patch" [1]
42068.patch:50: space before tab in indent.
echo powerpc64-unknown-linux-gnu
42068.patch:51: space before tab in indent.
exit ;;
42068.patch:56: space before tab in indent.
case `sed -n '/^cpu model/s/^.*: \(.*\)/\1/p' < /proc/cpuinfo` in
42068.patch:57: space before tab in indent.
  EV5)   UNAME_MACHINE=alphaev5 ;;
warning: 4 lines add whitespace errors.
3rdparty/zookeeper-3.4.5.patch:113: space before tab in indent.
+   echo powerpc64-unknown-linux-gnu
3rdparty/zookeeper-3.4.5.patch:114: space before tab in indent.
+   exit ;;
3rdparty/zookeeper-3.4.5.patch:119: space before tab in indent.
+   case `sed -n '/^cpu model/s/^.*: \(.*\)/\1/p' < /proc/cpuinfo` in
3rdparty/zookeeper-3.4.5.patch:120: space before tab in indent.
+ EV5)   UNAME_MACHINE=alphaev5 ;;

- Mesos ReviewBot


On Jan. 20, 2016, 12:38 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42068/
> ---
> 
> (Updated Jan. 20, 2016, 12:38 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4312
> https://issues.apache.org/jira/browse/MESOS-4312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Porting 3rdparty on ppc64le.
> 
> 
> Diffs
> -
> 
>   3rdparty/leveldb.patch ad8c19b9caa856ff85978ba832d48df11b3a83f0 
>   3rdparty/zookeeper-3.4.5.patch 3ca180d0c81f5de521ada7fb6c1c248a871ab2da 
> 
> Diff: https://reviews.apache.org/r/42068/diff/
> 
> 
> Testing
> ---
> 
> make check (OSX 10.10.5, Ubuntu 14.04.3 LTS ppc64le)
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 42504: Multiple Disk: Modified 'DiskInfo' stripping logic.

[Joris Van Remoortere]

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

(Updated Jan. 20, 2016, 7:18 p.m.)


Review request for mesos, Jie Yu and Michael Park.


Repository: mesos


Description (updated)
---

Refactored persistent volume tests to use a helper to generate disk
resources.


Diffs (updated)
-

  src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
  src/common/resources_utils.cpp d64db54a175299bf7c32ecbeeb1ce9a7afb1c88a 
  src/tests/mesos.hpp 3d9ebc6c9dc3cd1be02dc3771fbd847386907fac 
  src/tests/persistent_volume_tests.cpp 
7acf7ab29d64d891f3288f8042d267dcc82a32e9 
  src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666 
  src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 42503: Multiple Disk: Added Resource arithmetic tests.

[Joris Van Remoortere]

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

(Updated Jan. 20, 2016, 7:16 p.m.)


Review request for mesos, Jie Yu and Michael Park.


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


Repository: mesos


Description (updated)
---

Multiple Disk: Added Resource arithmetic tests.


Diffs (updated)
-

  src/tests/mesos.hpp 3d9ebc6c9dc3cd1be02dc3771fbd847386907fac 
  src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 42474: Multiple Disk: Updated Slave initialize to create DiskInfo paths.

[Joris Van Remoortere]

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

(Updated Jan. 20, 2016, 7:15 p.m.)


Review request for mesos, Jie Yu and Michael Park.


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


Repository: mesos


Description
---

Create disk paths based on 'DiskInfo.Source' if the type is 'PATH'
and the directory does not yet exist.


Diffs (updated)
-

  src/slave/slave.cpp bd7fe03f8a8b07c6201db2f876f4f9cd7dc337cf 

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


Testing
---

make check


Thanks,

Joris Van Remoortere

Re: Review Request 42472: Multiple Disk: Checkpoint persistent volume based on 'DiskInfo.Source'.

[Joris Van Remoortere]

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

(Updated Jan. 20, 2016, 7:12 p.m.)


Review request for mesos, Jie Yu and Michael Park.


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


Repository: mesos


Description
---

We now create persistent volume directories based on 'DiskInfo.Source'
if it is present.


Diffs (updated)
-

  src/slave/slave.cpp bd7fe03f8a8b07c6201db2f876f4f9cd7dc337cf 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 42361: Added dynamic reservation test with no authentication.

[Greg Mann]

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

(Updated Jan. 20, 2016, 7:04 p.m.)


Review request for mesos, Jie Yu, Michael Park, and Vinod Kone.


Changes
---

Changed summary/description.


Summary (updated)
-

Added dynamic reservation test with no authentication.


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


Repository: mesos


Description (updated)
---

Added dynamic reservation test with no authentication.

Currently, we do not allow dynamic reservations without a principal. The 
`ReservationInfo.principal` field is currently being migrated to `optional`, 
and in 0.28.0 we plan to begin allowing dynamic reservations without a 
principal. This test was added to verify that currently, framework reserve 
operations without a principal will fail as expected.


Diffs
-

  src/tests/reservation_tests.cpp 2ea3b6e48e75f438b325211fb562db19bd3a82e0 

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


Testing (updated)
---

A new test, `ReservationTest.ReservationInfoNoAuthentication`, was added, and 
`make check` was used to test. The new test was also run with 
`--gtest_repeat=1000 --gtest_break_on_failure=1`.


Thanks,

Greg Mann

Re: Review Request 42368: Added reservation endpoint test without authentication.

[Greg Mann]

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

(Updated Jan. 20, 2016, 6:59 p.m.)


Review request for mesos, Jie Yu, Michael Park, and Vinod Kone.


Changes
---

Updated comments.


Summary (updated)
-

Added reservation endpoint test without authentication.


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


Repository: mesos


Description (updated)
---

Added reservation endpoint test without authentication.

Currently, dynamic reservation endpoints will not work when HTTP authentication 
is not set. Currently, the authentication code will always return `None()` as 
the principal when authentication is disabled. Furthermore, the 
`ReservationInfo.principal` field is being migrated to `optional`, so dynamic 
reservations without a principal are invalidated by the master. This test 
checks that these endpoints will fail as expected when HTTP authentication is 
disabled.


Diffs (updated)
-

  src/tests/reservation_endpoints_tests.cpp 
b8edd6fafedd4c2221a8d19c1ebc71254071a8c7 

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


Testing
---

A new test, `ReservationEndpointsTest.ReserveAndUnreserveNoAuthentication`, was 
added, and make check was used to test. The new test was also run with 
`--gtest_repeat=1000 --gtest_break_on_failure=1`.


Thanks,

Greg Mann

Re: Review Request 42546: Updated `Master::Http::stateSummary` to use `jsonify`.

[Michael Park]

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

(Updated Jan. 20, 2016, 6:44 p.m.)


Review request for mesos and Benjamin Hindman.


Changes
---

Rebased.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/master/http.cpp 34a70ee50553492fc8c3947497ab5922f4379d72 

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


Testing
---

`make check`


Thanks,

Michael Park

Re: Review Request 42517: Added discussion about allowing multiple frameworks in a role.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [42517]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 19, 2016, 7:34 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42517/
> ---
> 
> (Updated Jan. 19, 2016, 7:34 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added discussion about allowing multiple frameworks in a role.
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md 4af7d6e8dc648fb630f56db5fbad1b7b438ebcfb 
> 
> Diff: https://reviews.apache.org/r/42517/diff/
> 
> 
> Testing
> ---
> 
> Previewed on github.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 41911: Added a test case for corrupt packets

[Ian Downes]

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



src/tests/containerizer/port_mapping_tests.cpp (line 23)


Alphabetize (or comment why this order is needed).



src/tests/containerizer/port_mapping_tests.cpp (line 994)


s/ */* /
This is a strange interface... why not take a `const char* data` and a 
`unsigned size`? and then cast the `data` appropriately.

This version from `http://locklessinc.com/articles/tcp_checksum/` is 
clearer:

```
unsigned short checksum1(const char *buf, unsigned size)
{
unsigned sum = 0;
int i;

/* Accumulate checksum */
for (i = 0; i < size - 1; i += 2)
{
unsigned short word16 = *(unsigned short *) &buf[i];
sum += word16;
}

/* Handle odd-sized case */
if (size & 1)
{
unsigned short word16 = (unsigned char) buf[i];
sum += word16;
}

/* Fold to get the ones-complement result */
while (sum >> 16) sum = (sum & 0x)+(sum >> 16);

/* Invert to get the negative in ones-complement arithmetic */
return ~sum;
}
```



src/tests/containerizer/port_mapping_tests.cpp (line 995)


Is this is standard IP checksum on the IP header? Please comment as such.



src/tests/containerizer/port_mapping_tests.cpp (line 1012)


s/-/ - /



src/tests/containerizer/port_mapping_tests.cpp (line 1027)


This function is doing a lot, both constructing the packet, opening a 
socket and sending the packet. What about splitting this functionality? One 
function to construct, another to open/send/close?



src/tests/containerizer/port_mapping_tests.cpp (line 1038)


s/s/socket/



src/tests/containerizer/port_mapping_tests.cpp (line 1047)


s/iph/ipHeader/



src/tests/containerizer/port_mapping_tests.cpp (line 1048)


s/udph/updHeader/



src/tests/containerizer/port_mapping_tests.cpp (line 1050)


What's stopping this writing beyond `datagram`?



src/tests/containerizer/port_mapping_tests.cpp (line 1082)


s/psize/pseudogramSize/
size_t?



src/tests/containerizer/port_mapping_tests.cpp (line 1086)


For multiline, please split all arguments to separate lines.



src/tests/containerizer/port_mapping_tests.cpp (line 1135)


drop the "1" when there's only a single usage.



src/tests/containerizer/port_mapping_tests.cpp (line 1149)


ditto, drop the "1".



src/tests/containerizer/port_mapping_tests.cpp (line 1172)


s/> >/>>/
No need to have space between > chevrons now.


- Ian Downes


On Jan. 14, 2016, 12:03 p.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41911/
> ---
> 
> (Updated Jan. 14, 2016, 12:03 p.m.)
> 
> 
> Review request for mesos, Ian Downes and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add a test case to ensure no corrupt packet could be delivered to application.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/port_mapping_tests.cpp 
> e3aea53468fa00374320a8b89bdbb64f38e44b01 
> 
> Diff: https://reviews.apache.org/r/41911/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Re: Review Request 42047: Specified the CgroupsNetClsIsolatorProcess class. This adds the ability to isolate a mesos container using the net_cls cgroup subsystem.

[Jie Yu]

> On Jan. 15, 2016, 6 a.m., Cong Wang wrote:
> > Why do we need netcls to regulate framework traffic on a per-container 
> > basis? Given the fact that a) the port range based filters already work and 
> > the code (see egress fq_codel) already exists b) we only have port range 
> > based network isolation so far.
> > 
> > I see no point of this. Please describe your use case with details, just 
> > pointing to netcls kernel doc doesn't help at all.
> 
> Cong Wang wrote:
> Since no one answers this, I assume no one in Mesosphere actually 
> understands it... So looks like you are pushing something no one is actually 
> going to use.
> 
> Avinash sridharan wrote:
> The egress_fq_codel that you are pointing too (I am assuming this is the 
> jira you are refferring to https://issues.apache.org/jira/browse/MESOS-2422) 
> needs port mapping isolator to enforce QoS on any egress traffic 
> shaping/policing, and for that matter any network policy enforcement.  
> 
> The net_cls cgroup is a linux kernel construct that allows operators to 
> support traffic shapping/policing and any network policy enforcement using 
> existing networking tools like tc and iptables. By enabling net_cls cgroup it 
> gives mesos a more generalized way of allowing operators to enforce network 
> policy irrespective of whether the task is running in the global namespace or 
> in a specific network namespace. In other words it will allow network policy 
> enforcement to take place irrespective of the type of network isolator you 
> are using. For e.g., if someone wants to use ip-per-container (MESOS-2044) vs 
> the port mapping isolator, operators would still be able to perform policy 
> enforcement without relying on the network isolator to provide those 
> constructs.
> 
> Cong Wang wrote:
> True, I know what netcls is more than you do, but you just ignore the 
> fact that we _only_ have port mapping isolator in our _current_ code, that is 
> my whole point. We can always add this _after_ ip-per-container work is 
> merged in upstream, it is never too late.
> 
> No need to mention this is hard to work together with the fq_codel 
> filters on egress. This is why I ask for more details, but you still don't 
> give any detail so far.

Cong, we already have other network isolators (e.g., Calico has one for ip per 
container) through modules. I don't think the operator will allow two network 
isolators to co-exists in production so I am not so worried about the egress 
filter conflicts.


- Jie


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


On Jan. 20, 2016, 5:05 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42047/
> ---
> 
> (Updated Jan. 20, 2016, 5:05 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4262
> https://issues.apache.org/jira/browse/MESOS-4262
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Specified the CgroupsNetClsIsolatorProcess class. This adds the ability to 
> isolate a mesos container using the net_cls cgroup subsystem.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt a52203ab9aa47315e6e5c58cc283a7b5df597c76 
>   src/Makefile.am 4fabe600d4ba38c95a777d622b0b675dd5811a53 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/42047/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 42516: Add support for user-defined networks.

[Ezra Silvera]

> On Jan. 20, 2016, 7:41 a.m., Guangya Liu wrote:
> > src/docker/docker.cpp, lines 539-541
> > 
> >
> > I think that you may also want some test cases to verify port mapping 
> > plus user defined network.

We agree that currently it seems we can't add a unit-test


- Ezra


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


On Jan. 20, 2016, 12:25 p.m., Ezra Silvera wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42516/
> ---
> 
> (Updated Jan. 20, 2016, 12:25 p.m.)
> 
> 
> Review request for mesos and TimothyIL TimothyIL.
> 
> 
> Bugs: MESOS-4369
> https://issues.apache.org/jira/browse/MESOS-4369
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Signed-off-by: Ezra Silvera 
> 
> Review: https://reviews.apache.org/r/42549
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 0be4bed336e86a5c377e87ac6212c70ac3b4c66b 
>   include/mesos/v1/mesos.proto c3244e87f9351c71312d2eace7a49bcac926fafd 
>   src/docker/docker.cpp 4d2f1fa14f4450b8fa3401081bf52807d2e79a7e 
> 
> Diff: https://reviews.apache.org/r/42516/diff/
> 
> 
> Testing
> ---
> 
> Using Swarm running on Mesos create a network with "docker  network create 
> --driver=bridge myNetwork"   and then create a container on that network:  
> "docker run --net=myNetwork"
> 
> 
> Thanks,
> 
> Ezra Silvera
> 
>

Re: Review Request 42239: Added tests for the Docker URI fetcher plugin.

[Gilbert Song]

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

Ship it!


Ship It!

- Gilbert Song


On Jan. 18, 2016, 12:14 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42239/
> ---
> 
> (Updated Jan. 18, 2016, 12:14 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-4296
> https://issues.apache.org/jira/browse/MESOS-4296
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for the Docker URI fetcher plugin.
> 
> 
> Diffs
> -
> 
>   src/tests/uri_fetcher_tests.cpp c3bc4eaa26edf2c4e827341794b230d022a91283 
> 
> Diff: https://reviews.apache.org/r/42239/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 42559: Allocated resources as revocable beyond quota.

[Alexander Rukletsov]

> On Jan. 20, 2016, 5:20 p.m., Klaus Ma wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1313
> > 
> >
> > Who will kill this `revocable` resources? So we'll have a patch for 
> > evicting?

Someday, someway... : )

Actually, I thought you guys are working on this (oversubscription for 
reservations, incl. quota)!


- Alexander


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


On Jan. 20, 2016, 5:01 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42559/
> ---
> 
> (Updated Jan. 20, 2016, 5:01 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, Joseph Wu, 
> and Qian Zhang.
> 
> 
> Bugs: MESOS-4441
> https://issues.apache.org/jira/browse/MESOS-4441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6 
>   src/master/allocator/mesos/hierarchical.cpp 
> e32ee4aa3ed9793bb5a99233e699e5cc2bdd796b 
>   src/tests/hierarchical_allocator_tests.cpp 
> 953712149bd951789beb29c72779c4ac65aa48dc 
> 
> Diff: https://reviews.apache.org/r/42559/diff/
> 
> 
> Testing
> ---
> 
> On Mac OS 10.10.4:
> 
> `GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh 
> --gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle`
> `make check`
> 
> Updated `HierarchicalAllocatorTest.DRFWithQuota` succeeds with the change and 
> fails without.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 42559: Allocated resources as revocable beyond quota.

[Klaus Ma]

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



src/master/allocator/mesos/hierarchical.cpp (line 1313)


Who will kill this `revocable` resources? So we'll have a patch for 
evicting?


- Klaus Ma


On Jan. 21, 2016, 1:01 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42559/
> ---
> 
> (Updated Jan. 21, 2016, 1:01 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, Joseph Wu, 
> and Qian Zhang.
> 
> 
> Bugs: MESOS-4441
> https://issues.apache.org/jira/browse/MESOS-4441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6 
>   src/master/allocator/mesos/hierarchical.cpp 
> e32ee4aa3ed9793bb5a99233e699e5cc2bdd796b 
>   src/tests/hierarchical_allocator_tests.cpp 
> 953712149bd951789beb29c72779c4ac65aa48dc 
> 
> Diff: https://reviews.apache.org/r/42559/diff/
> 
> 
> Testing
> ---
> 
> On Mac OS 10.10.4:
> 
> `GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh 
> --gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle`
> `make check`
> 
> Updated `HierarchicalAllocatorTest.DRFWithQuota` succeeds with the change and 
> fails without.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 42543: Updated `json` to handle integral types comprehensively.

[Michael Park]

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

(Updated Jan. 20, 2016, 5:04 p.m.)


Review request for mesos and Benjamin Hindman.


Summary (updated)
-

Updated `json` to handle integral types comprehensively.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp 
a87b9e2ade68eabd8560470b211a4703f3183d73 

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


Testing
---

`make check`


Thanks,

Michael Park

Review Request 42559: Allocated resources as revocable beyond quota.

[Alexander Rukletsov]

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

Review request for mesos, Bernd Mathiske, Joris Van Remoortere, Joseph Wu, and 
Qian Zhang.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6 
  src/master/allocator/mesos/hierarchical.cpp 
e32ee4aa3ed9793bb5a99233e699e5cc2bdd796b 
  src/tests/hierarchical_allocator_tests.cpp 
953712149bd951789beb29c72779c4ac65aa48dc 

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


Testing
---

On Mac OS 10.10.4:

`GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh 
--gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle`
`make check`

Updated `HierarchicalAllocatorTest.DRFWithQuota` succeeds with the change and 
fails without.


Thanks,

Alexander Rukletsov

Re: Review Request 42415: Added a roles field to FrameworkInfo.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [42415]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 19, 2016, 12:45 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42415/
> ---
> 
> (Updated Jan. 19, 2016, 12:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Vinod Kone.
> 
> 
> Bugs: MESOS-4363
> https://issues.apache.org/jira/browse/MESOS-4363
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a roles field to FrameworkInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 0be4bed336e86a5c377e87ac6212c70ac3b4c66b 
>   include/mesos/v1/mesos.proto c3244e87f9351c71312d2eace7a49bcac926fafd 
> 
> Diff: https://reviews.apache.org/r/42415/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.5)
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>