Re: Review Request 52534: Dispatch filter expiration twice.

[Jiang Yan Xu]

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


Fix it, then Ship it!





src/master/allocator/mesos/hierarchical.hpp (lines 229 - 230)


This comment here seems a bit odd, we just need to describe what it does 
and not how it's being called.

The original comment LGTM and it can cover both `expire` and `_expire` as 
they are of a group.



src/master/allocator/mesos/hierarchical.hpp (line 243)


No need to change this.



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


No need to change this.



src/master/allocator/mesos/hierarchical.cpp (lines 1061 - 1064)


The ojective is already described in the paragraph above so we don't have 
to reiterate it. We just need to explain the mechanism:

```
// Because `batch()` performs the allocation through a dispatch, we expire 
the filter also through a dispatched `_expire()` to achieve above.
```



src/master/allocator/mesos/hierarchical.cpp (lines 1806 - 1816)


Actually no need to disambiguate now that we only have one `_expire`.


- Jiang Yan Xu


On Jan. 30, 2017, 5:47 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52534/
> ---
> 
> (Updated Jan. 30, 2017, 5:47 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Jiang Yan 
> Xu.
> 
> 
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - With an asynchronous `batch()` allocation,
>   this ensures that filters do not expire
>   before the next allocation.
> - This patch should be reverted when allocation
>   occurs on resource recovery.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 2cda3e311ce339d82065d68de83e86439fa192ff 
>   src/master/allocator/mesos/hierarchical.cpp 
> f471b6848bebae601a7a0509e9c6ad5eab4fa4a2 
> 
> Diff: https://reviews.apache.org/r/52534/diff/
> 
> 
> Testing
> ---
> 
> With https://reviews.apache.org/r/51027/: 
> 
> GTEST_FILTER="-*SmallOfferFilter*" make check -j8
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 56112: Attempt to load module before checking if overlayfs is supported.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56112]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On Jan. 31, 2017, 5:36 a.m., Santhosh Kumar Shanmugham wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56112/
> ---
> 
> (Updated Jan. 31, 2017, 5:36 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Zhitao Li.
> 
> 
> Bugs: MESOS-7034
> https://issues.apache.org/jira/browse/MESOS-7034
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since overlayfs is optional on most kernels, it may not be loaded
> by default. Hence attempt to run a modprobe command to load the
> module before checking /proc/filesystems to verify that overlayfs
> is supported by the running kernel.
> 
> 
> Diffs
> -
> 
>   src/linux/fs.cpp 913e23317291db164fe6bdf77f3eca146dedec9b 
> 
> Diff: https://reviews.apache.org/r/56112/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Santhosh Kumar Shanmugham
> 
>

Re: Review Request 55967: Update the allocator unit tests to reflect MULTI_ROLE support.

[Michael Park]

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


Fix it, then Ship it!





src/tests/hierarchical_allocator_tests.cpp (lines 944 - 945)


This seems like an odd test. Unforunate that we can't seem to use 
`Allocation` because of this.



src/tests/hierarchical_allocator_tests.cpp (lines 3408 - 3411)


Should just use `foreachpair` twice here I think. A few places below as 
well.



src/tests/hierarchical_allocator_tests.cpp (lines 3728 - 3730)


Operators should be placed in the previous lines I think?

I think maybe it'd be better if we were to just pull out the 
`allocatedTaskResources`?

```cpp
  AllocatedResources allocatedTaskResources(task.resources(), "role1");

  expected = Allocation::create(
  framework2.id(),
  {{"role1",
{{slave.id(), updated.get() - allocatedTaskResources + volume);
```



src/tests/hierarchical_allocator_tests.cpp (lines 3810 - 3813)


`s/allocation/allocated/` since we have an `Allocation` type now? Also, 
could you have used `AllocatedResources` here?



src/tests/hierarchical_allocator_tests.cpp (line 4180)


`{` on the newline.


- Michael Park


On Jan. 25, 2017, 4:46 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55967/
> ---
> 
> (Updated Jan. 25, 2017, 4:46 p.m.)
> 
> 
> Review request for mesos, Guangya Liu and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update the allocator unit tests to reflect MULTI_ROLE support.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5 
> 
> Diff: https://reviews.apache.org/r/55967/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note that the rest of the tests do not pass until after my subsequent patches
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55870: Update the allocator to handle frameworks with multiple roles.

[Michael Park]

> On Jan. 30, 2017, 8:12 a.m., Benjamin Bannier wrote:
> > include/mesos/allocator/allocator.hpp, lines 89-92
> > 
> >
> > What is the reason for changing this interface? We should think hard 
> > before making it too specific to requirements from single features such as 
> > `MULTI_ROLE`.
> > 
> > Propagating internal assumptions such as "only allocations towards the 
> > same role in one `Resources`" across module boundaries really limits room 
> > for future developments. In this case both our allocator and the master 
> > rely on above assumption; it should be possible for them to adjust passed 
> > resources on interfaces, e.g., by creating datastructure like you proposed 
> > here internally themself (they could use the same helper function). That 
> > way we could avoid tech debt spilling into interfaces.

I can sympathize with this position, although it doesn't seem terrible to me 
that
the notion of multi-role which is being introduced to Mesos (rather than a 
specific allocator)
changes the allocator API.

But it doesn't seem obvious to me why it should be `hashmap>`,
as opposed to `hashmap>`. I tried to look 
through the code
briefly to see if it happens to make the code a lot simpler or something, but I 
don't really see one..

It seems to me like the pattern I see in a few places could be used here 
without changing the API also.

```cpp
const hashmap& resources;

foreachpair (const SlaveID& slaveId, const Resources& resources) {
  foreachpair (const string& role,
   const Resources& resources,
   resources.allocation()) {
// ...
  }
}
```

I'm more-so trying to understand what the reasoning is here.


- Michael


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


On Jan. 25, 2017, 1:55 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55870/
> ---
> 
> (Updated Jan. 25, 2017, 1:55 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-6635
> https://issues.apache.org/jira/browse/MESOS-6635
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The allocator now sets `Resource.AllocationInfo` when performing
> an allocation, and the interface is updated to expose allocations
> for each role within a framework.
> 
> The allocator also assumes that `Resource.AllocationInfo` is set
> for inbound resources that are allocated (e.g. when adding an agent,
> when adding a framework, when recovering resources). Note however,
> that the necessary changes to the master and agent to enforce this
> will be done via separate patches.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 558594983beb6ff49c1fddf875ba29c1983b1288 
>   src/master/allocator/mesos/allocator.hpp 
> 8e0f37a5eedd4d71d765991c0039b49c96f0ca53 
>   src/master/allocator/mesos/hierarchical.hpp 
> 9b66c23f26b37c02ed850c06c4518ea99078b02d 
>   src/master/allocator/mesos/hierarchical.cpp 
> c2211be7458755aeb91ef078e4bfe92ac474044a 
>   src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b 
>   src/master/master.cpp 73159328ce3fd838e02eba0e6a30cf69efc319ba 
>   src/tests/allocator.hpp 1f9261d1e7afa027bebc07915ba3e871a58e6558 
> 
> Diff: https://reviews.apache.org/r/55870/diff/
> 
> 
> Testing
> ---
> 
> Adjustments to existing tests are split out into a subsequent patch.
> 
> New tests for frameworks having multiple roles will be added as a follow up.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55870: Update the allocator to handle frameworks with multiple roles.

[Michael Park]

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


Fix it, then Ship it!





src/master/master.cpp (lines 6541 - 6544)


Can't we just use `foreachpair` for both?

```cpp
foreachpair (const string& role,
 const hashmap& resources_,
 resources) {
  foreachpair (const SlaveID& slaveId,
   const Resources& offered,
   resources_) {
// ...
  }
}
```


- Michael Park


On Jan. 25, 2017, 1:55 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55870/
> ---
> 
> (Updated Jan. 25, 2017, 1:55 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-6635
> https://issues.apache.org/jira/browse/MESOS-6635
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The allocator now sets `Resource.AllocationInfo` when performing
> an allocation, and the interface is updated to expose allocations
> for each role within a framework.
> 
> The allocator also assumes that `Resource.AllocationInfo` is set
> for inbound resources that are allocated (e.g. when adding an agent,
> when adding a framework, when recovering resources). Note however,
> that the necessary changes to the master and agent to enforce this
> will be done via separate patches.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 558594983beb6ff49c1fddf875ba29c1983b1288 
>   src/master/allocator/mesos/allocator.hpp 
> 8e0f37a5eedd4d71d765991c0039b49c96f0ca53 
>   src/master/allocator/mesos/hierarchical.hpp 
> 9b66c23f26b37c02ed850c06c4518ea99078b02d 
>   src/master/allocator/mesos/hierarchical.cpp 
> c2211be7458755aeb91ef078e4bfe92ac474044a 
>   src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b 
>   src/master/master.cpp 73159328ce3fd838e02eba0e6a30cf69efc319ba 
>   src/tests/allocator.hpp 1f9261d1e7afa027bebc07915ba3e871a58e6558 
> 
> Diff: https://reviews.apache.org/r/55870/diff/
> 
> 
> Testing
> ---
> 
> Adjustments to existing tests are split out into a subsequent patch.
> 
> New tests for frameworks having multiple roles will be added as a follow up.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 51028: Fix tests with rapidly triggered allocations.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [51028, 52534, 51027]

Failed command: python support/apply-reviews.py -n -r 52534

Error:
2017-01-31 05:57:33 URL:https://reviews.apache.org/r/52534/diff/raw/ 
[4204/4204] -> "52534.patch" [1]
error: patch failed: src/master/allocator/mesos/hierarchical.hpp:226
error: src/master/allocator/mesos/hierarchical.hpp: patch does not apply

Full log: https://builds.apache.org/job/Mesos-Reviewbot/16924/console

- Mesos Reviewbot


On Jan. 31, 2017, 1:41 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51028/
> ---
> 
> (Updated Jan. 31, 2017, 1:41 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Per MESOS-3157, if cluster events with the possibility
>   of triggering an allocation occur rapidly and test
>   assertions depend on gleaning information from assumed
>   order, it is likely they will fail due to lack of parity
>   between event and actual allocation. Ensure that expected
>   allocations occur.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> e04d1998679fcf022bb3741676a62da8b01ce97c 
> 
> Diff: https://reviews.apache.org/r/51028/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 49571: Added a benchmark test for allocations.

[Anindya Sinha]

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

(Updated Jan. 31, 2017, 5:59 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Allocations test has the following resource configurations:
(1) REGULAR: Offers from every slave have regular resources.
(2) SHARED: Offers from every slave include a shared resource.
(3) REGULAR: Offers from every alternate slave contain only regular
resources; and offers from every other alternate slave contains
a shared resource.

This test is parameterized based on number of agents, number of
frameworks and resource configuration.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
e04d1998679fcf022bb3741676a62da8b01ce97c 
  src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32 
  src/tests/resources_utils.cpp be1feb97be552af582dbba3a54fa5fa0714b3eb2 

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


Testing
---

All tests passed.

Allocations benchmark test results
==
Support of shared resources has a small impact (roughly 10%) on runtime 
performance in allocations as compared to HEAD (without shared resources). 
Also, there is no visible impact in performance when shared resources are added 
in the tests.

Following is a snapshot with 1000 agents and 200 frameworks.

With the patch (and no shared resources)

[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
Using 1000 agents and 200 frameworks with resource type 0
Added 200 frameworks in 6907us
Added 1000 agents in 2.057098secs
round 0 allocate() took 1.689164secs to make 1000 offers
round 50 allocate() took 1.672373secs to make 1000 offers
round 100 allocate() took 1.680571secs to make 1000 offers
round 150 allocate() took 1.674683secs to make 1000 offers
round 199 allocate() took 1.671525secs to make 1000 offers

With the patch (and shared resources on all agents)
---
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/10
Using 1000 agents and 200 frameworks with resource type 1
Added 200 frameworks in 6888us
Added 1000 agents in 2.096218secs
round 0 allocate() took 1.704491secs to make 1000 offers
round 50 allocate() took 1.718623secs to make 1000 offers
round 100 allocate() took 1.716224secs to make 1000 offers
round 150 allocate() took 1.707343secs to make 1000 offers
round 199 allocate() took 1.727467secs to make 1000 offers

With the patch (and shared resources on alternate agents)
-
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/11
Using 1000 agents and 200 frameworks with resource type 2
Added 200 frameworks in 7304us
Added 1000 agents in 2.071009secs
round 0 allocate() took 1.689045secs to make 1000 offers
round 50 allocate() took 1.691524secs to make 1000 offers
round 100 allocate() took 1.688873secs to make 1000 offers
round 150 allocate() took 1.688713secs to make 1000 offers
round 199 allocate() took 1.691223secs to make 1000 offers

Based on HEAD, with all regular resources (no shared resources in HEAD 
supported)
-
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
Using 1000 agents and 200 frameworks with resource type 0
Added 200 frameworks in 6801us
Added 1000 agents in 1.721447secs
round 0 allocate() took 1.502953secs to make 1000 offers
round 50 allocate() took 1.520157secs to make 1000 offers
round 100 allocate() took 1.517221secs to make 1000 offers
round 150 allocate() took 1.526446secs to make 1000 offers
round 199 allocate() took 1.538005secs to make 1000 offers


Thanks,

Anindya Sinha

Re: Review Request 55359: Consolidate update of allocations in `updateAllocation()`.

[Anindya Sinha]

> On Jan. 26, 2017, 1:11 a.m., Jiang Yan Xu wrote:
> > My feeling is that we were using too many variables for the same thing and 
> > after this review there are still more than we need.
> > 
> > 
> > Feels like there should just be these variables to capture the allocation 
> > change:
> > 
> > `offeredResources`: passed down from the master, this is the "original".
> > `updatedOfferedResources`: the version with all the operations applied, 
> > either in `Resources::apply()`, or within this method for `LAUNCH`. This is 
> > the "updated", but more clear, since we are dealing with both total 
> > resources and allocations.
> > 
> > Then we can update all the allocation related varaibles using these two.
> > 
> > Then, next, we update the agent total resources. Why updating the totals in 
> > the method `updateAllocation()`? We can clarify this in the method (see my 
> > comments there).

As discussed, the flow in `updateAllocation()` is now as follows:

```
  // (1) Make a copy of `offeredResources` (called `updatedOfferedResources`) 
to make changes
  // to original offered resources based on offer operations.
  // (2) Iterate thorugh each of the operations as follows:
  // (2a) For all operations, `updatedOfferedResources = 
updatedOfferedResources.apply()`
  // (2b) For LAUNCH, accumulate all of task resources for all tasks in 
`consumed`.
  // (3) Determine additional copies of shared resources to be added to 
allocations based
  // on `consumed` and `updatedOfferedResources`.
  // (4) Update the allocations in the allocator and the sorters.
  // (5) Update the total resources so that they are consistent with the 
updated allocation.
```

The main change is to process all operastions first and then update the 
allocations followed by total resources together (instead of doing it per 
operation).


> On Jan. 26, 2017, 1:11 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 640-652
> > 
> >
> > To unify these variables, perhaps don't use `_offeredResources`. To be 
> > more consistent with the naming pattern within this method (`something` and 
> > `updatedSomething`), perhaps:
> > 
> > ```
> > Resources updatedOfferedResources = offeredResources;
> > 
> > // (The objective of this loop is to obtain the correct 
> > `updatedOfferedResources`.)
> > foreach (const Offer::Operation& operation, operations) {  
> >   if (operation.type() == Offer::Operation::LAUNCH) {
> > // LAUNCH custom logic.
> > // (Here everything we do is essentially also udpate 
> > `updatedOfferedResources`.)
> > // ...
> >   } else {
> > // (Here we invoke `Resources::apply()` to update 
> > `updatedOfferedResources`.)
> > Try _updatedOfferedResources = 
> > updatedOfferedResources.apply(operation);
> > CHECK_SOME(_updatedOfferedResources);
> > updatedOfferedResources = _updatedOfferedResources.get();
> >   }
> > }
> > 
> > // At this point, we are outside the loop and we have "fully" processed 
> > the offered
> > // resources to obtain `updatedOfferedResources`.
> > // Now use it to update allocator and sorter variables.
> > 
> > // ...
> > ```
> > 
> > We don't need `updated` to capture the updated offered resources again.

Addressed based on the top comment.


> On Jan. 26, 2017, 1:11 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 645
> > 
> >
> > `original` is updated in every iteration?
> > 
> > Isn't original offered resources the variable `offeredResources`?

The updated flow removes the need for `original` and `updated`.


> On Jan. 26, 2017, 1:11 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 786-787
> > 
> >
> > After all operations perhaps it's valuable to CHECK that we indeed 
> > didn't change the quantities. So for this we can save the framework's 
> > allocation at the very beginning and compare with it at the very end.
> > 
> > ```
> > void HierarchicalAllocatorProcess::updateAllocation(
> > const FrameworkID& frameworkId,
> > const SlaveID& slaveId,
> > const Resources& offeredResources,
> > const vector& operations)
> > {
> >   // ... some initial checks.
> >   
> >   // (Save `frameworkAllocation`.)
> >   const Resources frameworkAllocation = 
> > frameworkSorter->allocation(frameworkId.value(), slaveId);
> > 
> >   // Do the work.
> >   
> >   // Check that the quantities are not changed.
> >   const Resources updatedFrameworkAllocation = 
> > frameworkSorter->allocation(frameworkId.value(), slaveId);
> >   CHECK_EQ(frameworkAllocation.cre

Re: Review Request 55359: Consolidate update of allocations in `updateAllocation()`.

[Anindya Sinha]

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

(Updated Jan. 31, 2017, 5:59 a.m.)


Review request for mesos, Benjamin Mahler and Jiang Yan Xu.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

We handle shared resources for `LAUNCH` operations separately in the
`updateAllocation()` API to add additional copies of shared resources.
But the updates to the allocations in the allocator and sorters
can be consolidated together.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
f471b6848bebae601a7a0509e9c6ad5eab4fa4a2 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

[Anindya Sinha]

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

(Updated Jan. 31, 2017, 5:58 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

We maintain a single copy of shared resource in the role and quota
sorter's total resources. So, when we update these resources, we
remove the previous resources at this agent and add the new resources
at this agent.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
2cda3e311ce339d82065d68de83e86439fa192ff 
  src/master/allocator/mesos/hierarchical.cpp 
f471b6848bebae601a7a0509e9c6ad5eab4fa4a2 
  src/tests/persistent_volume_tests.cpp 
468a85b4a6ce09592341afd07ce12a03f5fc4f73 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

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

(Updated Jan. 31, 2017, 5:58 a.m.)


Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Updated a persistent volume test framework to include shared volumes.


Diffs (updated)
-

  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Review Request 56112: Attempt to load module before checking if overlayfs is supported.

[Santhosh Kumar Shanmugham]

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

Review request for mesos, Jie Yu and Zhitao Li.


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


Repository: mesos


Description
---

Since overlayfs is optional on most kernels, it may not be loaded
by default. Hence attempt to run a modprobe command to load the
module before checking /proc/filesystems to verify that overlayfs
is supported by the running kernel.


Diffs
-

  src/linux/fs.cpp 913e23317291db164fe6bdf77f3eca146dedec9b 

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


Testing
---


Thanks,

Santhosh Kumar Shanmugham

Re: Review Request 55870: Update the allocator to handle frameworks with multiple roles.

[Michael Park]

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


Fix it, then Ship it!





src/master/allocator/mesos/hierarchical.hpp (line 355)


`s/strippedAllocation/unallocated/`, since "stripped" means something 
different for us.



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


Does `CHECK_EQ` not work here?



src/master/allocator/mesos/hierarchical.cpp (lines 661 - 670)


We have a bunch of places where this condition needs to hold true I think. 
Is this location more special than others for some reason?

If we're not going to pull this out to a validation of some kind, let's at 
least leave a `TODO` here.


- Michael Park


On Jan. 25, 2017, 1:55 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55870/
> ---
> 
> (Updated Jan. 25, 2017, 1:55 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-6635
> https://issues.apache.org/jira/browse/MESOS-6635
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The allocator now sets `Resource.AllocationInfo` when performing
> an allocation, and the interface is updated to expose allocations
> for each role within a framework.
> 
> The allocator also assumes that `Resource.AllocationInfo` is set
> for inbound resources that are allocated (e.g. when adding an agent,
> when adding a framework, when recovering resources). Note however,
> that the necessary changes to the master and agent to enforce this
> will be done via separate patches.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 558594983beb6ff49c1fddf875ba29c1983b1288 
>   src/master/allocator/mesos/allocator.hpp 
> 8e0f37a5eedd4d71d765991c0039b49c96f0ca53 
>   src/master/allocator/mesos/hierarchical.hpp 
> 9b66c23f26b37c02ed850c06c4518ea99078b02d 
>   src/master/allocator/mesos/hierarchical.cpp 
> c2211be7458755aeb91ef078e4bfe92ac474044a 
>   src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b 
>   src/master/master.cpp 73159328ce3fd838e02eba0e6a30cf69efc319ba 
>   src/tests/allocator.hpp 1f9261d1e7afa027bebc07915ba3e871a58e6558 
> 
> Diff: https://reviews.apache.org/r/55870/diff/
> 
> 
> Testing
> ---
> 
> Adjustments to existing tests are split out into a subsequent patch.
> 
> New tests for frameworks having multiple roles will be added as a follow up.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55893: Fixed OversubscriptionTest.RescindRevocableOfferWithIncreasedRevocable.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [55893, 52534, 51027]

Failed command: python support/apply-reviews.py -n -r 52534

Error:
2017-01-31 05:03:16 URL:https://reviews.apache.org/r/52534/diff/raw/ 
[4204/4204] -> "52534.patch" [1]
error: patch failed: src/master/allocator/mesos/hierarchical.hpp:226
error: src/master/allocator/mesos/hierarchical.hpp: patch does not apply

Full log: https://builds.apache.org/job/Mesos-Reviewbot/16923/console

- Mesos Reviewbot


On Jan. 31, 2017, 12:55 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55893/
> ---
> 
> (Updated Jan. 31, 2017, 12:55 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, and Jacob Janco.
> 
> 
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is another test fix in the chain.
> 
> 
> Diffs
> -
> 
>   src/tests/oversubscription_tests.cpp 
> 22ae069ab71c82c6a6e2f5783b13ebd74a9ccf25 
> 
> Diff: https://reviews.apache.org/r/55893/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 55625: Prevented certain kinds of gaming the quota system.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [55625]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On Jan. 30, 2017, 10:27 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55625/
> ---
> 
> (Updated Jan. 30, 2017, 10:27 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6432
> https://issues.apache.org/jira/browse/MESOS-6432
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the certain coarse-grained allocation scheme it is possible to game
> quota since the allocator will perform allocations until the quotas
> for all resource kinds are satisfied. This can lead to allocations for
> certain resource kinds far exceeding the set quota.
> 
> This patch changes the notion of what constitutes a "satisfied quota".
> Where before a quota could only be satisfied if for all resource kinds
> allocations had been me up to the set quota, we now consider a quota
> satisfied as soon as at least one resource kinds is allocated up to
> the set quota. We here take only "common" resource kinds into account
> where a common resource is a resource present on every agent node. The
> notion of common resource kinds is needed to avoid strong bias from
> rare resources in the introduced quota allocation scheme.
> 
> Ultimately this change can only be temporary and should be replaced
> with a more complete fix, e.g., chunked allocations, see MESOS-3765.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f471b6848bebae601a7a0509e9c6ad5eab4fa4a2 
>   src/tests/hierarchical_allocator_tests.cpp 
> e04d1998679fcf022bb3741676a62da8b01ce97c 
> 
> Diff: https://reviews.apache.org/r/55625/diff/
> 
> 
> Testing
> ---
> 
> Tested of various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 56080: Fixed error handling when writing to a cgroup control file.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56080]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On Jan. 30, 2017, 7:07 p.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56080/
> ---
> 
> (Updated Jan. 30, 2017, 7:07 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7020
> https://issues.apache.org/jira/browse/MESOS-7020
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Due to internal buffering in ofstream, actual write to the underlying
> device may not be performed by the time of checking for failures.
> This patch flushes written data to ensure fail check uses actual value.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 630e2ec2d0eac2bb35488d0416883df1601224c8 
>   src/tests/containerizer/cgroups_tests.cpp 
> 5c1ef70fb1feb76de602418286989b48563f26ff 
> 
> Diff: https://reviews.apache.org/r/56080/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 55453: Updated comments in `HealthCheck` protobuf for clarity.

[Jiang Yan Xu]

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




include/mesos/mesos.proto (lines 424 - 426)


The most direct interpretation for the delay is actually the time since the 
task was launched right? AFAIK Mesos provided executors immediately send 
TASK_RUNNING but this is not generally required right?


- Jiang Yan Xu


On Jan. 20, 2017, 6:48 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55453/
> ---
> 
> (Updated Jan. 20, 2017, 6:48 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, haosdent huang, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8f1d6957a97eff1e0a94817d38e7a7de6d69 
>   include/mesos/v1/mesos.proto 74e7851b147ab821dceeab6e838d34b092f101c3 
> 
> Diff: https://reviews.apache.org/r/55453/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 55874: Added a simple AllocatorBacklog benchmark.

[Jacob Janco]

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


Ship it!




Ship It!

- Jacob Janco


On Jan. 24, 2017, 9:46 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55874/
> ---
> 
> (Updated Jan. 24, 2017, 9:46 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jacob Janco, and Zhitao Li.
> 
> 
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To evaluate MESOS-6904.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5 
> 
> Diff: https://reviews.apache.org/r/55874/diff/
> 
> 
> Testing
> ---
> 
> ## Interpretation the benchmark output
> 
> - With the allocation batching it's faster to process all the `reviveOffers` 
> events after all resources in the cluster have already been allocated. These 
> allocation runs then don't generate allocations and so their execution should 
> be identical and the allocation batching reduces the number of them. 
> - With the allocation batching it takes about the same time to process the 
> same number of `addSlave` events, no matter if allocations are batched or 
> not. This is because individual `addSlave` calls trigger only allocation for 
> that agent while batched allocation allocates all of the accumulated 
> candidates so far, so the total cost should be the similar.
> - It's also faster to process `addFrameworks` with allocation batching but in 
> this benchmark frameworks are added when no agents are connected, so the 
> allocation runs should exit very quickly.
> 
> ## Output
> 
> With the patch set:
> 
> ```
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AllocatorBacklog/0
> Using 1000 agents and 1 frameworks
> Added 1 frameworks in 1.638795ms with 1 allocation runs
> Added 1000 agents in 799.084178ms with 4 allocation runs
> Processed 1 `reviveOffers` calls in 10.030412ms with 1 allocation runs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AllocatorBacklog/0
>  (909 ms)
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AllocatorBacklog/1
> Using 1000 agents and 50 frameworks
> Added 50 frameworks in 5.496104ms with 2 allocation runs
> Added 1000 agents in 1.210938725secs with 3 allocation runs
> Processed 50 `reviveOffers` calls in 129.410729ms with 1 allocation runs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AllocatorBacklog/1
>  (1420 ms)
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AllocatorBacklog/2
> Using 1000 agents and 100 frameworks
> Added 100 frameworks in 9.31721ms with 2 allocation runs
> Added 1000 agents in 1.064227156secs with 3 allocation runs
> Processed 100 `reviveOffers` calls in 229.829276ms with 1 allocation runs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AllocatorBacklog/2
>  (1410 ms)
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AllocatorBacklog/3
> Using 1000 agents and 200 frameworks
> Added 200 frameworks in 19.380803ms with 3 allocation runs
> Added 1000 agents in 1.472364101secs with 2 allocation runs
> Processed 200 `reviveOffers` calls in 419.054445ms with 1 allocation runs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AllocatorBacklog/3
>  (2007 ms)
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AllocatorBacklog/4
> Using 1000 agents and 500 frameworks
> Added 500 frameworks in 39.074317ms with 2 allocation runs
> Added 1000 agents in 2.561990683secs with 2 allocation runs
> Processed 500 `reviveOffers` calls in 754.971627ms with 2 allocation runs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AllocatorBacklog/4
>  (3440 ms)
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AllocatorBacklog/5
> Using 1000 agents and 1000 frameworks
> Added 1000 frameworks in 73.831892ms with 2 allocation runs
> Added 1000 agents in 4.395376381secs with 2 allocation runs
> Processed 1000 `reviveOffers` calls in 1.660467486secs with 1 allocation runs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AllocatorBacklog/5
>  (6218 ms)
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AllocatorBacklog/6
> Using 1000 agents and 3000 frameworks
> Added 3000 frameworks in 139.383796ms with 3 allocation runs   
> Added 1000 agents in 10.951388325secs with 2 allocation runs   
> Processed 3000 `reviveOffers` calls in 4.557919408secs with 2 all

Re: Review Request 52534: Dispatch filter expiration twice.

[Jacob Janco]

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

(Updated Jan. 31, 2017, 1:47 a.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Jiang Yan 
Xu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

- With an asynchronous `batch()` allocation,
  this ensures that filters do not expire
  before the next allocation.
- This patch should be reverted when allocation
  occurs on resource recovery.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
2cda3e311ce339d82065d68de83e86439fa192ff 
  src/master/allocator/mesos/hierarchical.cpp 
f471b6848bebae601a7a0509e9c6ad5eab4fa4a2 

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


Testing
---

With https://reviews.apache.org/r/51027/: 

GTEST_FILTER="-*SmallOfferFilter*" make check -j8


Thanks,

Jacob Janco

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

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

(Updated Jan. 31, 2017, 1:46 a.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, 
and Jiang Yan Xu.


Changes
---

Small fix to batch allocation.


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


Repository: mesos


Description
---

- Triggered allocations dispatch allocate() only
  if there is no pending allocation in the queue.
- Allocation candidates are accumulated and only
  cleared when enqueued allocations are processed.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
2cda3e311ce339d82065d68de83e86439fa192ff 
  src/master/allocator/mesos/hierarchical.cpp 
f471b6848bebae601a7a0509e9c6ad5eab4fa4a2 

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


Testing
---

make check with the filters below

Broken tests: 
- TEST_F(HierarchicalAllocatorTest, SuppressAndReviveOffers), fix in 51028
- TEST_F(HierarchicalAllocatorTest, AllocationRunsMetric), fix in 51028
- TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics), fix in 51028
- TEST_F(HierarchicalAllocatorTest, UpdateWeight), fix in 51028
- TEST_P(HierarchicalAllocator_BENCHMARK_Test, AddAndUpdateSlave), fix in 51028
- TEST_F(HierarchicalAllocatorTest, SmallOfferFilterTimeout), fix in 52534
- TEST_F(OversubscriptionTest, RescindRevocableOfferWithIncreasedRevocable), 
fix in 51621


Thanks,

Jacob Janco

Re: Review Request 51028: Fix tests with rapidly triggered allocations.

[Jacob Janco]

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

(Updated Jan. 31, 2017, 1:41 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

- Per MESOS-3157, if cluster events with the possibility
  of triggering an allocation occur rapidly and test
  assertions depend on gleaning information from assumed
  order, it is likely they will fail due to lack of parity
  between event and actual allocation. Ensure that expected
  allocations occur.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
e04d1998679fcf022bb3741676a62da8b01ce97c 

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


Testing
---

make check


Thanks,

Jacob Janco

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

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

(Updated Jan. 31, 2017, 1:41 a.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, 
and Jiang Yan Xu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

- Triggered allocations dispatch allocate() only
  if there is no pending allocation in the queue.
- Allocation candidates are accumulated and only
  cleared when enqueued allocations are processed.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
2cda3e311ce339d82065d68de83e86439fa192ff 
  src/master/allocator/mesos/hierarchical.cpp 
f471b6848bebae601a7a0509e9c6ad5eab4fa4a2 

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


Testing
---

make check with the filters below

Broken tests: 
- TEST_F(HierarchicalAllocatorTest, SuppressAndReviveOffers), fix in 51028
- TEST_F(HierarchicalAllocatorTest, AllocationRunsMetric), fix in 51028
- TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics), fix in 51028
- TEST_F(HierarchicalAllocatorTest, UpdateWeight), fix in 51028
- TEST_P(HierarchicalAllocator_BENCHMARK_Test, AddAndUpdateSlave), fix in 51028
- TEST_F(HierarchicalAllocatorTest, SmallOfferFilterTimeout), fix in 52534
- TEST_F(OversubscriptionTest, RescindRevocableOfferWithIncreasedRevocable), 
fix in 51621


Thanks,

Jacob Janco

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jiang Yan Xu]

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


Fix it, then Ship it!





src/master/allocator/mesos/hierarchical.cpp (lines 1260 - 1262)


`onAny` doesn't require this. (And we actually don't need `then()` here as 
we don't actually return anything. I was wrong to suggest it previously. :)

```
allocate()
  .onAny([pid, this]() {
delay(allocationInterval, pid, &Self::batch);
  });
```

should just work.


- Jiang Yan Xu


On Jan. 26, 2017, 6:33 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Jan. 26, 2017, 6:33 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Triggered allocations dispatch allocate() only
>   if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
>   cleared when enqueued allocations are processed.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 9b66c23f26b37c02ed850c06c4518ea99078b02d 
>   src/master/allocator/mesos/hierarchical.cpp 
> c2211be7458755aeb91ef078e4bfe92ac474044a 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check with the filters below
> 
> Broken tests: 
> - TEST_F(HierarchicalAllocatorTest, SuppressAndReviveOffers), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunsMetric), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, UpdateWeight), fix in 51028
> - TEST_P(HierarchicalAllocator_BENCHMARK_Test, AddAndUpdateSlave), fix in 
> 51028
> - TEST_F(HierarchicalAllocatorTest, SmallOfferFilterTimeout), fix in 52534
> - TEST_F(OversubscriptionTest, RescindRevocableOfferWithIncreasedRevocable), 
> fix in 51621
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 55893: Fixed OversubscriptionTest.RescindRevocableOfferWithIncreasedRevocable.

[Jiang Yan Xu]

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

(Updated Jan. 30, 2017, 4:55 p.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, and Jacob Janco.


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


Repository: mesos


Description
---

This is another test fix in the chain.


Diffs
-

  src/tests/oversubscription_tests.cpp 22ae069ab71c82c6a6e2f5783b13ebd74a9ccf25 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Re: Review Request 56087: Simplified AppC provisioner cache keys hashing.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56087]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On Jan. 30, 2017, 5:28 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56087/
> ---
> 
> (Updated Jan. 30, 2017, 5:28 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Repalced manual `std::map` contents hashing with specialized 
> `boost::hash_combine()`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/appc/cache.cpp 
> fed5ae97419d5eb9611a6fd1ade1bbed7ed440a2 
> 
> Diff: https://reviews.apache.org/r/56087/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 55893: Fixed OversubscriptionTest.RescindRevocableOfferWithIncreasedRevocable.

[Jacob Janco]

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



Running at --gtest_repeat=1000 failed with: 

../../src/tests/oversubscription_tests.cpp:544: Failure
Value of: resources3
  Actual: { cpus(*){REV}:2 }
Expected: resources2
Which is: { cpus(*){REV}:3 }
*** Aborted at 1485821402 (unix time) try "date -d @1485821402" if you are 
using GNU date ***
PC: @0x108522f60 testing::UnitTest::AddTestPartResult()
*** SIGSEGV (@0x0) received by PID 13601 (TID 0x7fffcaede3c0) stack trace: ***
@ 0x7fffc229ebba _sigtramp
@0xa (unknown)
@0x108522767 testing::internal::AssertHelper::operator=()
@0x107aed3f1 
mesos::internal::tests::OversubscriptionTest_RescindRevocableOfferWithIncreasedRevocable_Test::TestBody()
@0x107ab780a 
testing::internal::HandleSehExceptionsInMethodIfSupported<>()
@0x108534877 
testing::internal::HandleExceptionsInMethodIfSupported<>()
@0x108534725 testing::Test::Run()
@0x108536278 testing::TestInfo::Run()
@0x1085375e7 testing::TestCase::Run()
@0x10854716c testing::internal::UnitTestImpl::RunAllTests()
@0x108582eda 
testing::internal::HandleSehExceptionsInMethodIfSupported<>()
@0x108546b97 
testing::internal::HandleExceptionsInMethodIfSupported<>()
@0x108546a68 testing::UnitTest::Run()
@0x107642ee1 RUN_ALL_TESTS()
@0x10763ec1d main
@ 0x7fffc2091255 start
Segmentation fault: 11

- Jacob Janco


On Jan. 24, 2017, 9:52 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55893/
> ---
> 
> (Updated Jan. 24, 2017, 9:52 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, and Jacob Janco.
> 
> 
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is another test fix in the chain.
> 
> 
> Diffs
> -
> 
>   src/tests/oversubscription_tests.cpp 
> 22ae069ab71c82c6a6e2f5783b13ebd74a9ccf25 
> 
> Diff: https://reviews.apache.org/r/55893/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 55852: Fixed MasterAllocatorTest/1.RebalancedForUpdatedWeights.

[Jacob Janco]

> On Jan. 30, 2017, 11:51 p.m., Jacob Janco wrote:
> > Ship It!
> 
> Jacob Janco wrote:
> Makes sense, with batched allocation recovery and allocation can be 
> interleaved causing flakiness in this test.

Passed --gtest_repeat=1000


- Jacob


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


On Jan. 23, 2017, 7:08 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55852/
> ---
> 
> (Updated Jan. 23, 2017, 7:08 p.m.)
> 
> 
> Review request for mesos, Guangya Liu and Jacob Janco.
> 
> 
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - This test is broken by the batched allocation change.
> 
> 
> Diffs
> -
> 
>   src/tests/master_allocator_tests.cpp 
> 996762f25453f7a8a5e0b7b97006ee2a603cf8c4 
> 
> Diff: https://reviews.apache.org/r/55852/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 55852: Fixed MasterAllocatorTest/1.RebalancedForUpdatedWeights.

[Jacob Janco]

> On Jan. 30, 2017, 11:51 p.m., Jacob Janco wrote:
> > Ship It!

Makes sense, with batched allocation recovery and allocation can be interleaved 
causing flakiness in this test.


- Jacob


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


On Jan. 23, 2017, 7:08 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55852/
> ---
> 
> (Updated Jan. 23, 2017, 7:08 p.m.)
> 
> 
> Review request for mesos, Guangya Liu and Jacob Janco.
> 
> 
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - This test is broken by the batched allocation change.
> 
> 
> Diffs
> -
> 
>   src/tests/master_allocator_tests.cpp 
> 996762f25453f7a8a5e0b7b97006ee2a603cf8c4 
> 
> Diff: https://reviews.apache.org/r/55852/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 55852: Fixed MasterAllocatorTest/1.RebalancedForUpdatedWeights.

[Jacob Janco]

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


Ship it!




Ship It!

- Jacob Janco


On Jan. 23, 2017, 7:08 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55852/
> ---
> 
> (Updated Jan. 23, 2017, 7:08 p.m.)
> 
> 
> Review request for mesos, Guangya Liu and Jacob Janco.
> 
> 
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - This test is broken by the batched allocation change.
> 
> 
> Diffs
> -
> 
>   src/tests/master_allocator_tests.cpp 
> 996762f25453f7a8a5e0b7b97006ee2a603cf8c4 
> 
> Diff: https://reviews.apache.org/r/55852/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 56086: Simplified AppC provisioner cache keys comparison.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56086]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On Jan. 30, 2017, 5:27 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56086/
> ---
> 
> (Updated Jan. 30, 2017, 5:27 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Manual elements searching is not required since `std::map` is ordered. 
> Replaced it with standard `operator==` that checks maps sizes and does linear 
> element by element comparison.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/appc/cache.cpp 
> fed5ae97419d5eb9611a6fd1ade1bbed7ed440a2 
> 
> Diff: https://reviews.apache.org/r/56086/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 55625: Prevented certain kinds of gaming the quota system.

[Benjamin Bannier]

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

(Updated Jan. 30, 2017, 11:27 p.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


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


Repository: mesos


Description
---

In the certain coarse-grained allocation scheme it is possible to game
quota since the allocator will perform allocations until the quotas
for all resource kinds are satisfied. This can lead to allocations for
certain resource kinds far exceeding the set quota.

This patch changes the notion of what constitutes a "satisfied quota".
Where before a quota could only be satisfied if for all resource kinds
allocations had been me up to the set quota, we now consider a quota
satisfied as soon as at least one resource kinds is allocated up to
the set quota. We here take only "common" resource kinds into account
where a common resource is a resource present on every agent node. The
notion of common resource kinds is needed to avoid strong bias from
rare resources in the introduced quota allocation scheme.

Ultimately this change can only be temporary and should be replaced
with a more complete fix, e.g., chunked allocations, see MESOS-3765.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
f471b6848bebae601a7a0509e9c6ad5eab4fa4a2 
  src/tests/hierarchical_allocator_tests.cpp 
e04d1998679fcf022bb3741676a62da8b01ce97c 

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


Testing
---

Tested of various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 53252: Added `hashset` constructor for type that supports `begin()/end()`.

[Benjamin Bannier]

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




3rdparty/stout/include/stout/hashset.hpp (lines 74 - 80)


`std::set` already defines a constructor taking a range, so I'd suggest not 
reimplementing it here, but instead to simply inherit the base class' 
constructors,

using std::unordered_set::unordered_set;

Note that this makes the constructor taking an `std::initializer_list` 
redundant, so it should be removed.

Note that this would allow us to simplify existing constructors with even 
less typing, e.g.,

hashset(const std::set& set) : hashset(set.begin(), set.end()) {}

and similar.


- Benjamin Bannier


On Oct. 28, 2016, 7:12 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53252/
> ---
> 
> (Updated Oct. 28, 2016, 7:12 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6035
> https://issues.apache.org/jira/browse/MESOS-6035
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `hashset` constructor for any arbitrary type that supports
> `begin()` and `end()`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/hashset.hpp 
> 8f633cad4d27c1e5f391279b7f3e8cb5fbf4bd03 
> 
> Diff: https://reviews.apache.org/r/53252/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 55910: Prevent unintended mutation in the allocator.

[Michael Park]

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


Fix it, then Ship it!





src/master/allocator/mesos/hierarchical.cpp (lines 381 - 385)


Not yours, but do you know what's going on here?
The comment doesn't seem to match the code.



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


Seems like we could've just kept:
```cpp
const string& role = frameworks.at(frameworkId).role;
```

and used `role` rather than `framework.role` below.



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


Did you mean `const Owned&` here?

I'd prefer to not introduce more places where `Owned` is being used like a 
`Shared`.

or maybe even:

```cpp
Sorter& frameworkSorter = *frameworkSorters.at(role);
```


- Michael Park


On Jan. 24, 2017, 6:40 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55910/
> ---
> 
> (Updated Jan. 24, 2017, 6:40 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, a lof of code in the allocator makes use of the `[]`
> operator to access the agents, frameworks and sorters, which
> can lead to subtle bugs where insertion was unintended.
> 
> With this change, a few const functions can be marked as such.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 9b66c23f26b37c02ed850c06c4518ea99078b02d 
>   src/master/allocator/mesos/hierarchical.cpp 
> c2211be7458755aeb91ef078e4bfe92ac474044a 
> 
> Diff: https://reviews.apache.org/r/55910/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55868: Cleanups to the allocator tests.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Jan. 23, 2017, 6:31 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55868/
> ---
> 
> (Updated Jan. 23, 2017, 6:31 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This was necessary to greatly simplify the changes needed to the
> allocator tests as we introduce support for multi-role frameworks.
> 
> The main improvement here is to establish and use equality on the
> `Allocation` struct, which makes the tests more readable and avoids
> the manual probing of the allocation structure across all the tests.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5 
> 
> Diff: https://reviews.apache.org/r/55868/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55868: Cleanups to the allocator tests.

[Michael Park]

> On Jan. 30, 2017, 8:11 a.m., Benjamin Bannier wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 85
> > 
> >
> > This should just be a plain constructor, right?

+1


> On Jan. 30, 2017, 8:11 a.m., Benjamin Bannier wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 289-293
> > 
> >
> > If we made `Allocation::create` just a constructor, it should be 
> > possible to create `Allocation`s in test assertions, e.g.,
> > 
> > AWAIT_EXPECT_EQ(Allocation(framework1.id(), {slave1.id(), 
> > slave1.resources()}),
> > allocations.get());
> > 
> > Here and below.
> > 
> > This should also help remove instances where `expected` is reused to 
> > hold completely different values.
> > 
> > What do you think?

Well, to be fair, this is possible with `Allocation::create` too.


> On Jan. 30, 2017, 8:11 a.m., Benjamin Bannier wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 3530-3531
> > 
> >
> > This does look like Google style while we seem to prefer passing a 
> > non-`const` ref in such instances.
> > 
> > Otherwise lets at least `CHECK_NOTNULL`.

The general rule is that pointer to non-`const` is preferred over non-`const` 
references for function parameters.
The reason (as far as I see it) is that we can't tell at the callsite whether 
an argument is to be modified or not.

We can talk about this outside of this review if we want.


- Michael


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


On Jan. 23, 2017, 6:31 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55868/
> ---
> 
> (Updated Jan. 23, 2017, 6:31 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This was necessary to greatly simplify the changes needed to the
> allocator tests as we introduce support for multi-role frameworks.
> 
> The main improvement here is to establish and use equality on the
> `Allocation` struct, which makes the tests more readable and avoids
> the manual probing of the allocation structure across all the tests.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5 
> 
> Diff: https://reviews.apache.org/r/55868/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55543: Fail the build if %PreferredToolArchitecture% is not set to `x64`.

[Joseph Wu]

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



Discussed this offline.  In summary, we want to prevent users from (shooting 
themselves in the foot) bypassing the CMake check by doing something like:
```
PreferredToolArchitecture=x64 cmake ..
msbuild Mesos.sln /m
```

The current solution is effectively only checked at "configure" time.  We 
should add a build-time step to error out instead.

- Joseph Wu


On Jan. 28, 2017, 11:21 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55543/
> ---
> 
> (Updated Jan. 28, 2017, 11:21 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Bugs: MESOS-6720
> https://issues.apache.org/jira/browse/MESOS-6720
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Before building Mesos on a Windows machine, it is necessary to set
> `%PreferredToolArchitecture%` to the value `x64`. This is necessary to
> work around (at least) two bugs in the MSVC backend: in particular, the
> linker can sometimes take hours or days to link `mesos-x.x.x.lib`, and
> the build system occasionally finds it self spuriously unable to find
> file `mesos-x.x.x.lib` to link against.
> 
> These issues are well-known and documented (e.g., in the official Mesos
> "getting started" document), but it is better to simply refuse to build
> Mesos at all on Windows unless that environment variable is set.
> 
> This commit will introduce such a check.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 560935b81603dc58c167918d36e2ae0a4060673d 
> 
> Diff: https://reviews.apache.org/r/55543/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 51028: Fix tests with rapidly triggered allocations.

[Jacob Janco]

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

(Updated Jan. 30, 2017, 11:15 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Updated review to depend on /r/52534


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


Repository: mesos


Description
---

- Per MESOS-3157, if cluster events with the possibility
  of triggering an allocation occur rapidly and test
  assertions depend on gleaning information from assumed
  order, it is likely they will fail due to lack of parity
  between event and actual allocation. Ensure that expected
  allocations occur.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
1edd0ecc8a93cd41532e1cf3641f67c780ab23a5 

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


Testing
---

make check


Thanks,

Jacob Janco

Re: Review Request 56080: Fixed error handling when writing to a cgroup control file.

[Dmitry Zhuk]

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

(Updated Jan. 30, 2017, 7:07 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated test according to review comments.


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


Repository: mesos


Description
---

Due to internal buffering in ofstream, actual write to the underlying
device may not be performed by the time of checking for failures.
This patch flushes written data to ensure fail check uses actual value.


Diffs (updated)
-

  src/linux/cgroups.cpp 630e2ec2d0eac2bb35488d0416883df1601224c8 
  src/tests/containerizer/cgroups_tests.cpp 
5c1ef70fb1feb76de602418286989b48563f26ff 

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


Testing
---

sudo make check


Thanks,

Dmitry Zhuk

Re: Review Request 55625: Prevented certain kinds of gaming the quota system.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [55625]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On Jan. 30, 2017, 4:55 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55625/
> ---
> 
> (Updated Jan. 30, 2017, 4:55 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6432
> https://issues.apache.org/jira/browse/MESOS-6432
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the certain coarse-grained allocation scheme it is possible to game
> quota since the allocator will perform allocations until the quotas
> for all resource kinds are satisfied. This can lead to allocations for
> certain resource kinds far exceeding the set quota.
> 
> This patch changes the notion of what constitutes a "satisfied quota".
> Where before a quota could only be satisfied if for all resource kinds
> allocations had been me up to the set quota, we now consider a quota
> satisfied as soon as at least one resource kinds is allocated up to
> the set quota. We here take only "common" resource kinds into account
> where a common resource is a resource present on every agent node. The
> notion of common resource kinds is needed to avoid strong bias from
> rare resources in the introduced quota allocation scheme.
> 
> Ultimately this change can only be temporary and should be replaced
> with a more complete fix, e.g., chunked allocations, see MESOS-3765.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f471b6848bebae601a7a0509e9c6ad5eab4fa4a2 
>   src/tests/hierarchical_allocator_tests.cpp 
> e04d1998679fcf022bb3741676a62da8b01ce97c 
> 
> Diff: https://reviews.apache.org/r/55625/diff/
> 
> 
> Testing
> ---
> 
> Tested of various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 56080: Fixed error handling when writing to a cgroup control file.

[Dmitry Zhuk]

> On Jan. 30, 2017, 5:14 p.m., James Peach wrote:
> > Since you need to detect errors, why use `iostreams` at all? This code is 
> > just writing a buffer to a file, so `os::write` would let you do the same 
> > thing with lower overhead and improved error handling.
> > 
> > AFAICT this function could be written as:
> > ```C
> > static Try write(
> > const string& hierarchy,
> > const string& cgroup,
> > const string& control,
> > const string& value)
> > {
> >   string path = path::join(hierarchy, cgroup, control);
> >   return os::write(path, value);
> > }
> > 
> > ```

There's similar code in `read`, which can be replaced with `os::read` (issue in 
`TODO(benh)` seems to be resolved already).
Shall this be fixed in this review request? Or I can sumbit a separate review 
for `os::read` and `os::write`, though it probably worth merging this patch to 
update tests.


- Dmitry


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


On Jan. 30, 2017, 3:20 p.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56080/
> ---
> 
> (Updated Jan. 30, 2017, 3:20 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7020
> https://issues.apache.org/jira/browse/MESOS-7020
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Due to internal buffering in ofstream, actual write to the underlying
> device may not be performed by the time of checking for failures.
> This patch flushes written data to ensure fail check uses actual value.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 630e2ec2d0eac2bb35488d0416883df1601224c8 
>   src/tests/containerizer/cgroups_tests.cpp 
> 5c1ef70fb1feb76de602418286989b48563f26ff 
> 
> Diff: https://reviews.apache.org/r/56080/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 55893: Fixed OversubscriptionTest.RescindRevocableOfferWithIncreasedRevocable.

[Jiang Yan Xu]

> On Jan. 26, 2017, 1:40 a.m., Guangya Liu wrote:
> > src/tests/oversubscription_tests.cpp, line 559
> > 
> >
> > How about following?
> > 
> > ```
> > // The latest resource estimate should match the total offered 
> > resources.
> > ```
> > 
> > This can also make the comments match `resources2 and resources3`.

You mean match `EXPECT_EQ(resources2, resources3);`?

So with `EXPECT_EQ`, the signature is `EXPECT_EQ(expected, actual);` so its 
indeed resources3 (offered resources, actual) matches resources2 (the estimate, 
expected).


> On Jan. 26, 2017, 1:40 a.m., Guangya Liu wrote:
> > src/tests/oversubscription_tests.cpp, line 512
> > 
> >
> > How about s/offer1/offer as here you only have one such variable.

True but on the other hand, offer1 matches resources1, as in, the first offer. 
If I s/offerId/offerId1/ I guess this would be more clear.


- Jiang Yan


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


On Jan. 24, 2017, 1:52 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55893/
> ---
> 
> (Updated Jan. 24, 2017, 1:52 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, and Jacob Janco.
> 
> 
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is another test fix in the chain.
> 
> 
> Diffs
> -
> 
>   src/tests/oversubscription_tests.cpp 
> 22ae069ab71c82c6a6e2f5783b13ebd74a9ccf25 
> 
> Diff: https://reviews.apache.org/r/55893/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Review Request 56087: Simplified AppC provisioner cache keys hashing.

[Ilya Pronin]

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

Review request for mesos, Benjamin Mahler and Jie Yu.


Repository: mesos


Description
---

Repalced manual `std::map` contents hashing with specialized 
`boost::hash_combine()`.


Diffs
-

  src/slave/containerizer/mesos/provisioner/appc/cache.cpp 
fed5ae97419d5eb9611a6fd1ade1bbed7ed440a2 

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


Testing
---

`make check`


Thanks,

Ilya Pronin

Review Request 56086: Simplified AppC provisioner cache keys comparison.

[Ilya Pronin]

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

Review request for mesos, Benjamin Mahler and Jie Yu.


Repository: mesos


Description
---

Manual elements searching is not required since `std::map` is ordered. Replaced 
it with standard `operator==` that checks maps sizes and does linear element by 
element comparison.


Diffs
-

  src/slave/containerizer/mesos/provisioner/appc/cache.cpp 
fed5ae97419d5eb9611a6fd1ade1bbed7ed440a2 

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


Testing
---

`make check`


Thanks,

Ilya Pronin

Re: Review Request 56080: Fixed error handling when writing to a cgroup control file.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56080]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On Jan. 30, 2017, 3:20 p.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56080/
> ---
> 
> (Updated Jan. 30, 2017, 3:20 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7020
> https://issues.apache.org/jira/browse/MESOS-7020
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Due to internal buffering in ofstream, actual write to the underlying
> device may not be performed by the time of checking for failures.
> This patch flushes written data to ensure fail check uses actual value.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 630e2ec2d0eac2bb35488d0416883df1601224c8 
>   src/tests/containerizer/cgroups_tests.cpp 
> 5c1ef70fb1feb76de602418286989b48563f26ff 
> 
> Diff: https://reviews.apache.org/r/56080/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 56080: Fixed error handling when writing to a cgroup control file.

[James Peach]

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



Since you need to detect errors, why use `iostreams` at all? This code is just 
writing a buffer to a file, so `os::write` would let you do the same thing with 
lower overhead and improved error handling.

AFAICT this function could be written as:
```C
static Try write(
const string& hierarchy,
const string& cgroup,
const string& control,
const string& value)
{
  string path = path::join(hierarchy, cgroup, control);
  return os::write(path, value);
}

```

- James Peach


On Jan. 30, 2017, 3:20 p.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56080/
> ---
> 
> (Updated Jan. 30, 2017, 3:20 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7020
> https://issues.apache.org/jira/browse/MESOS-7020
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Due to internal buffering in ofstream, actual write to the underlying
> device may not be performed by the time of checking for failures.
> This patch flushes written data to ensure fail check uses actual value.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 630e2ec2d0eac2bb35488d0416883df1601224c8 
>   src/tests/containerizer/cgroups_tests.cpp 
> 5c1ef70fb1feb76de602418286989b48563f26ff 
> 
> Diff: https://reviews.apache.org/r/56080/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 56080: Fixed error handling when writing to a cgroup control file.

[Jie Yu]

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


Fix it, then Ship it!





src/tests/containerizer/cgroups_tests.cpp (lines 497 - 498)


Let's move this up right below:
```
ASSERT_SOME(cgroups::create(hierarchy, TEST_CGROUPS_ROOT));
```


- Jie Yu


On Jan. 30, 2017, 3:20 p.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56080/
> ---
> 
> (Updated Jan. 30, 2017, 3:20 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7020
> https://issues.apache.org/jira/browse/MESOS-7020
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Due to internal buffering in ofstream, actual write to the underlying
> device may not be performed by the time of checking for failures.
> This patch flushes written data to ensure fail check uses actual value.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 630e2ec2d0eac2bb35488d0416883df1601224c8 
>   src/tests/containerizer/cgroups_tests.cpp 
> 5c1ef70fb1feb76de602418286989b48563f26ff 
> 
> Diff: https://reviews.apache.org/r/56080/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 55625: Prevented certain kinds of gaming the quota system.

[Benjamin Bannier]

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

(Updated Jan. 30, 2017, 5:55 p.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


Changes
---

Removed the notion of "common" resource kinds.


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


Repository: mesos


Description
---

In the certain coarse-grained allocation scheme it is possible to game
quota since the allocator will perform allocations until the quotas
for all resource kinds are satisfied. This can lead to allocations for
certain resource kinds far exceeding the set quota.

This patch changes the notion of what constitutes a "satisfied quota".
Where before a quota could only be satisfied if for all resource kinds
allocations had been me up to the set quota, we now consider a quota
satisfied as soon as at least one resource kinds is allocated up to
the set quota. We here take only "common" resource kinds into account
where a common resource is a resource present on every agent node. The
notion of common resource kinds is needed to avoid strong bias from
rare resources in the introduced quota allocation scheme.

Ultimately this change can only be temporary and should be replaced
with a more complete fix, e.g., chunked allocations, see MESOS-3765.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
f471b6848bebae601a7a0509e9c6ad5eab4fa4a2 
  src/tests/hierarchical_allocator_tests.cpp 
e04d1998679fcf022bb3741676a62da8b01ce97c 

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


Testing
---

Tested of various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 55828: Updated Resources::apply to handle Resource.AllocationInfo.

[Benjamin Bannier]

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




src/common/resources.cpp (lines 1263 - 1266)


Could you clarify under what conditions this can happen?

It seems surprising to silently drop information explicitly given by users, 
and I believe rejecting the operation would be clearer. If situations can occur 
where a user has more recent information than the current master (e.g., an 
allocation info not yet known to the master) we should fix failover behavior.



src/common/resources.cpp (lines 1268 - 1270)


`..., see MESOS-6636.` to allow better tracking of the status of this.



src/common/resources.cpp (line 1272)


Did you intend to make this `const`? If not, the following appears more 
straight-forward to me,

bool isAllocated = false;
foreach(const Resource& resource, resources) {
  if (resource.has_allocation_info()) {
isAllocated = true;
break;
  }
}

If you'd went for the optimization suggested by Guangya this does become a 
"one-liner" though,

const bool isAllocated =
  resources.resources().empty()
? false
: resources.resources(0).has_allocation_info();



src/common/resources.cpp (line 1288)


This looks like an expensive computation. Should we precompute it at 
function scope and capture it here by ref?



src/tests/resources_utils.hpp (line 40)


This class seems confusing. It e.g., is not a wrapper since when 
constructing an `AllocatedResources` from some unallocated `Resources` an 
assertion like  `EXPECT_EQ(resources, allocated)` is not true (i.e., it doesn't 
just wrap some `Resources` and some role).

What about making this a function for now, e.g.,

Resources allocatedResources(
const Resources& resources,
const string& role);


- Benjamin Bannier


On Jan. 23, 2017, 11:47 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55828/
> ---
> 
> (Updated Jan. 23, 2017, 11:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-6967
> https://issues.apache.org/jira/browse/MESOS-6967
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, `Resource` did not contain `AllocationInfo`. So for
> backwards compatibility with old schedulers and tooling, we must
> allow operations to contain `Resource`s without an allocation role.
> The two interesting cases for adjusting the operation's resource are:
> 
> (1) The operation `Resource` does not contain an `AllocationInfo`
> but is being applied to an allocated `Resources`. We allow this
> only if the operation is unambiguous, that is, the allocated
> `Resources` are only allocated to a single role.
> 
> (2) The operation `Resource` contains an `AllocationInfo` but is
> being applied to an unallocated `Resources`. In this case we
> simply ignore the `AllocationInfo` of the `Resource`.
> 
> Note that we assume no `Resources` store a mix of allocated and
> unallocated resources. This is a brittle assumption that we should
> have enforcement for.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45 
>   src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68 
>   src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32 
>   src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267 
> 
> Diff: https://reviews.apache.org/r/55828/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55870: Update the allocator to handle frameworks with multiple roles.

[Benjamin Bannier]

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




include/mesos/allocator/allocator.hpp (lines 89 - 92)


What is the reason for changing this interface? We should think hard before 
making it too specific to requirements from single features such as 
`MULTI_ROLE`.

Propagating internal assumptions such as "only allocations towards the same 
role in one `Resources`" across module boundaries really limits room for future 
developments. In this case both our allocator and the master rely on above 
assumption; it should be possible for them to adjust passed resources on 
interfaces, e.g., by creating datastructure like you proposed here internally 
themself (they could use the same helper function). That way we could avoid 
tech debt spilling into interfaces.



include/mesos/allocator/allocator.hpp (line 264)


Do we need this detail of our allocator implementation in the generic 
interface?



include/mesos/allocator/allocator.hpp (lines 333 - 335)


Internal detail?



src/master/allocator/mesos/hierarchical.cpp (lines 685 - 690)


Is this detailed knowledge of `Resource` internals necessary here? Below we 
use the same adjusted `operation` regardless of its type.



src/master/allocator/mesos/hierarchical.cpp (lines 703 - 704)


Lets try to put this code into the caller (e.g., `Master::_accept`). It 
seems that would introduce less current and future breakage in custom allocator 
modules.


- Benjamin Bannier


On Jan. 25, 2017, 10:55 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55870/
> ---
> 
> (Updated Jan. 25, 2017, 10:55 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-6635
> https://issues.apache.org/jira/browse/MESOS-6635
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The allocator now sets `Resource.AllocationInfo` when performing
> an allocation, and the interface is updated to expose allocations
> for each role within a framework.
> 
> The allocator also assumes that `Resource.AllocationInfo` is set
> for inbound resources that are allocated (e.g. when adding an agent,
> when adding a framework, when recovering resources). Note however,
> that the necessary changes to the master and agent to enforce this
> will be done via separate patches.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 558594983beb6ff49c1fddf875ba29c1983b1288 
>   src/master/allocator/mesos/allocator.hpp 
> 8e0f37a5eedd4d71d765991c0039b49c96f0ca53 
>   src/master/allocator/mesos/hierarchical.hpp 
> 9b66c23f26b37c02ed850c06c4518ea99078b02d 
>   src/master/allocator/mesos/hierarchical.cpp 
> c2211be7458755aeb91ef078e4bfe92ac474044a 
>   src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b 
>   src/master/master.cpp 73159328ce3fd838e02eba0e6a30cf69efc319ba 
>   src/tests/allocator.hpp 1f9261d1e7afa027bebc07915ba3e871a58e6558 
> 
> Diff: https://reviews.apache.org/r/55870/diff/
> 
> 
> Testing
> ---
> 
> Adjustments to existing tests are split out into a subsequent patch.
> 
> New tests for frameworks having multiple roles will be added as a follow up.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55868: Cleanups to the allocator tests.

[Benjamin Bannier]

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




src/tests/hierarchical_allocator_tests.cpp (line 85)


This should just be a plain constructor, right?



src/tests/hierarchical_allocator_tests.cpp (lines 289 - 293)


If we made `Allocation::create` just a constructor, it should be possible 
to create `Allocation`s in test assertions, e.g.,

AWAIT_EXPECT_EQ(Allocation(framework1.id(), {slave1.id(), 
slave1.resources()}),
allocations.get());

Here and below.

This should also help remove instances where `expected` is reused to hold 
completely different values.

What do you think?



src/tests/hierarchical_allocator_tests.cpp (lines 3367 - 3368)


This does look like Google style while we seem to prefer passing a 
non-`const` ref in such instances.

Otherwise lets at least `CHECK_NOTNULL`.


- Benjamin Bannier


On Jan. 24, 2017, 3:31 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55868/
> ---
> 
> (Updated Jan. 24, 2017, 3:31 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This was necessary to greatly simplify the changes needed to the
> allocator tests as we introduce support for multi-role frameworks.
> 
> The main improvement here is to establish and use equality on the
> `Allocation` struct, which makes the tests more readable and avoids
> the manual probing of the allocation structure across all the tests.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5 
> 
> Diff: https://reviews.apache.org/r/55868/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55910: Prevent unintended mutation in the allocator.

[Benjamin Bannier]

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


Fix it, then Ship it!





src/master/allocator/mesos/hierarchical.hpp (line 283)


Is this intentionally implicit? Otherwise please make this `explicit`.



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


nit: This could be set in the member declaration.



src/master/allocator/mesos/hierarchical.cpp (lines 237 - 241)


@gyliu: I like introducing `Framework` as a helper doing extraction from a 
`FrameworkInfo` (this is expanded on in r/55870).

@bmahler: `insert` also takes an `initializer_list`, i.e.,

frameworks.insert({frameworkId, Framework(frameworkInfo)});

Otherwise please include ``.


- Benjamin Bannier


On Jan. 25, 2017, 3:40 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55910/
> ---
> 
> (Updated Jan. 25, 2017, 3:40 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, a lof of code in the allocator makes use of the `[]`
> operator to access the agents, frameworks and sorters, which
> can lead to subtle bugs where insertion was unintended.
> 
> With this change, a few const functions can be marked as such.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 9b66c23f26b37c02ed850c06c4518ea99078b02d 
>   src/master/allocator/mesos/hierarchical.cpp 
> c2211be7458755aeb91ef078e4bfe92ac474044a 
> 
> Diff: https://reviews.apache.org/r/55910/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 56080: Fixed error handling when writing to a cgroup control file.

[Dmitry Zhuk]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Due to internal buffering in ofstream, actual write to the underlying
device may not be performed by the time of checking for failures.
This patch flushes written data to ensure fail check uses actual value.


Diffs
-

  src/linux/cgroups.cpp 630e2ec2d0eac2bb35488d0416883df1601224c8 
  src/tests/containerizer/cgroups_tests.cpp 
5c1ef70fb1feb76de602418286989b48563f26ff 

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


Testing
---

sudo make check


Thanks,

Dmitry Zhuk

Re: Review Request 56004: Fixed MULTI_ROLE related bugs when updating framework info.

[Benjamin Bannier]

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




src/master/master.hpp (line 2438)


Not yours, but should likely be

if (source.capabilities.empty()) {
  info.clear_capabilities();
} else {
  info.mutable_capabilities()->CopyFrom(source.capabilities());
}


- Benjamin Bannier


On Jan. 27, 2017, 1:30 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56004/
> ---
> 
> (Updated Jan. 27, 2017, 1:30 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The first issue is that we need to update the capabilities member
> to reflect the new capabilities.
> 
> The second issue is that when we allow an upgrade or downgrade
> to or from MULTI_ROLE, we need to update the `role` and `roles`
> fields of `FrameworkInfo`.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 7e38af41ca16241dbbe3bc2e80c0848e82762a45 
> 
> Diff: https://reviews.apache.org/r/56004/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 56004: Fixed MULTI_ROLE related bugs when updating framework info.

[Benjamin Bannier]

> On Jan. 30, 2017, 2:46 a.m., Guangya Liu wrote:
> > src/master/master.hpp, lines 2441-2450
> > 
> >
> > Can you please add some comments here to clarify that this is used to 
> > handle upgrade/downgrade case to/from multi role framework?

This whole block (and also the added code) are related to any changes in 
framework capabilities, right?


- Benjamin


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


On Jan. 27, 2017, 1:30 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56004/
> ---
> 
> (Updated Jan. 27, 2017, 1:30 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The first issue is that we need to update the capabilities member
> to reflect the new capabilities.
> 
> The second issue is that when we allow an upgrade or downgrade
> to or from MULTI_ROLE, we need to update the `role` and `roles`
> fields of `FrameworkInfo`.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 7e38af41ca16241dbbe3bc2e80c0848e82762a45 
> 
> Diff: https://reviews.apache.org/r/56004/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 56055: Added validation for 'LAUNCH_NESTED_CONTAINER_SESSION'.

[Jan Schlicht]

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




src/tests/slave_validation_tests.cpp (line 282)


Indent with 4 spaces.


- Jan Schlicht


On Jan. 29, 2017, 12:39 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56055/
> ---
> 
> (Updated Jan. 29, 2017, 12:39 a.m.)
> 
> 
> Review request for mesos, Jan Schlicht and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a validation test for the
> `LAUNCH_NESTED_CONTAINER_SESSION` call.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_validation_tests.cpp 
> 5de771114982751e7796f55dcacd4384c6989efb 
> 
> Diff: https://reviews.apache.org/r/56055/diff/
> 
> 
> Testing
> ---
> 
> `bin/mesos-tests.sh --gtest_filter="*Validation*"`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 55955: Added validation tests to ensure environment variable value is set.

[Jan Schlicht]

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


Ship it!




Ship It!

- Jan Schlicht


On Jan. 29, 2017, 12:35 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55955/
> ---
> 
> (Updated Jan. 29, 2017, 12:35 a.m.)
> 
> 
> Review request for mesos, Jan Schlicht and Vinod Kone.
> 
> 
> Bugs: MESOS-6991
> https://issues.apache.org/jira/browse/MESOS-6991
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `value` field within `Environment::Variable` is being
> changed to `optional`, but for the time being we will
> enforce that it must be set for backward compatibility.
> This patch adds tests to ensure that environment variables
> with unset values are correctly rejected.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp c88cd34fd214f111cff62591aa5fc03eb62567e4 
>   src/tests/health_check_tests.cpp debbd3c09b7555145aaf3f62a24d795d1423a269 
>   src/tests/master_validation_tests.cpp 
> edb57407e08cdbd8fbf10a9e1493cab3b4979bb8 
>   src/tests/slave_validation_tests.cpp 
> 5de771114982751e7796f55dcacd4384c6989efb 
> 
> Diff: https://reviews.apache.org/r/55955/diff/
> 
> 
> Testing
> ---
> 
> bin/mesos-tests.sh --gtest_filter="*Validation*"
> 
> 
> Thanks,
> 
> Greg Mann
> 
>