Re: Review Request 55321: Introduced process::after.

[Benjamin Hindman]

> On Feb. 2, 2017, 11:19 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/include/process/after.hpp, line 51
> > 
> >
> > We keep two copies of a shared pointer to `promise` in an `onDiscard` 
> > callback (one directly and one via `timer`). It seems that all code paths 
> > in `after` do clear `onDiscardCallbacks` and hence destruct the promise. 
> > But I'd have tests proving that rather than relying on my mental compiling 
> > abilities : ).

Suggestion on how to create such a test? I've added a comment for now (per 
Benjamin's request).


- Benjamin


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


On Jan. 8, 2017, 7:49 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55321/
> ---
> 
> (Updated Jan. 8, 2017, 7:49 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced process::after.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 5e7fdd06ccbed50f248c81e9df1651a3702e7add 
>   3rdparty/libprocess/include/Makefile.am 
> 1d17edd933562849b35740f3935685c8eb154440 
>   3rdparty/libprocess/include/process/after.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/after_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55321/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 55321: Introduced process::after.

[Benjamin Hindman]

> On Feb. 2, 2017, 11:26 a.m., Gastón Kleiman wrote:
> > 3rdparty/libprocess/Makefile.am, line 237
> > 
> >
> > Shouldn't we update the cmake lists as well?

Thanks!


- Benjamin


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


On Jan. 8, 2017, 7:49 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55321/
> ---
> 
> (Updated Jan. 8, 2017, 7:49 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced process::after.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 5e7fdd06ccbed50f248c81e9df1651a3702e7add 
>   3rdparty/libprocess/include/Makefile.am 
> 1d17edd933562849b35740f3935685c8eb154440 
>   3rdparty/libprocess/include/process/after.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/after_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55321/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 55321: Introduced process::after.

[Benjamin Hindman]

> On Feb. 1, 2017, 4:38 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/include/process/after.hpp, lines 50-55
> > 
> >
> > I was trying to wrap my head around whether this created a bad circular 
> > reference `promise -> future -> discardCallback -> promise` and ultimately 
> > a leak of `Future::data` or not. Are we sure this does not leak when a user 
> > e.g., calls it like this?,
> > 
> > after(Seconds(10)); // return value ignored.
> > 
> > I believe adding a clarifyinh comment, or even better a test would be 
> > great.

I've added a comment Benjamin. Not sure how I can test this, did you have 
something in mind?


- Benjamin


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


On Jan. 8, 2017, 7:49 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55321/
> ---
> 
> (Updated Jan. 8, 2017, 7:49 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced process::after.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 5e7fdd06ccbed50f248c81e9df1651a3702e7add 
>   3rdparty/libprocess/include/Makefile.am 
> 1d17edd933562849b35740f3935685c8eb154440 
>   3rdparty/libprocess/include/process/after.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/after_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55321/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

[Jiang Yan Xu]

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


Ship it!




Ship It!

- Jiang Yan Xu


On Feb. 3, 2017, 4:44 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> ---
> 
> (Updated Feb. 3, 2017, 4:44 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138 
>   src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e 
> 
> Diff: https://reviews.apache.org/r/53791/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 24)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55973: Update the tests to handle MULTI_ROLE support.

[Guangya Liu]

> On 一月 30, 2017, 4:44 a.m., Guangya Liu wrote:
> > src/examples/dynamic_reservation_framework.cpp, line 123
> > 
> >
> > A question here, why need `apply` resources here? What is the problem 
> > is we keeps check if the `unreserved` contains `Allocated TASK RESOURCES`?
> 
> Benjamin Mahler wrote:
> It just seemed a bit unfortunate that this code used 'contains' to assume 
> apply would succeed, rather than directly applying. Thoughts?

Yes, I think that both works but seems your proposal is more simple as with old 
logic, we also need to mark the `TASK RESOURCES` as allocated when check 
`contains`.


- Guangya


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


On 一月 27, 2017, 12:34 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55973/
> ---
> 
> (Updated 一月 27, 2017, 12:34 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A number of tests and example frameworks assume that allocated
> resources did not look different from unallocated resources.
> This updates the tests to reflect the presence of
> `Resource.AllocationInfo`.
> 
> 
> Diffs
> -
> 
>   src/examples/disk_full_framework.cpp 
> e13d4c8a427905793dda9bb01c52b6d372c19150 
>   src/examples/dynamic_reservation_framework.cpp 
> 4d3db965e18d79ed3e1759bb5f6cb41104562f47 
>   src/examples/no_executor_framework.cpp 
> e82ae9a9ea23c7d181b071f0e3f5071b3856d8a4 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
>   src/examples/test_framework.cpp 068b85d551012a390ba2a68bc88807103d3c31f3 
>   src/examples/test_http_framework.cpp 
> 258cb512803d1ed07c7a5ce1465e5663e77b0977 
>   src/tests/api_tests.cpp 400ac6fcd420508509291249cad50679fbe9807d 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> ba268b6918be0e7226a975c38eff5620b076083f 
>   src/tests/hook_tests.cpp f4ef629768beabc014e14472c5818d117d51abe7 
>   src/tests/master_allocator_tests.cpp 
> 996762f25453f7a8a5e0b7b97006ee2a603cf8c4 
>   src/tests/master_tests.cpp da7094dbbafbb0ab1153a0a4a6fcabd63888d67a 
>   src/tests/mesos.hpp 71e919702bccd31bf1e14205cd1e30d473883d48 
>   src/tests/oversubscription_tests.cpp 
> 22ae069ab71c82c6a6e2f5783b13ebd74a9ccf25 
>   src/tests/partition_tests.cpp f03c5bd96861b6643a4ecd9e37775447f86c3710 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 695029fe0cb91d668d6a8495f3ccd0c69c883cd2 
>   src/tests/persistent_volume_tests.cpp 
> 468a85b4a6ce09592341afd07ce12a03f5fc4f73 
>   src/tests/reservation_endpoints_tests.cpp 
> 7bc59c2ded9f7e6f1ccaa37456f654d18efcb2e2 
>   src/tests/reservation_tests.cpp b7061de0dfcc79548d507c7d70376c82d5d647ec 
>   src/tests/resource_offers_tests.cpp 
> 72ceb8696a7277f9add6d89adb55aa08665bdeb5 
>   src/tests/role_tests.cpp 0d0f1a94ff9aaec6ea04280282a52ac88dc6b273 
>   src/tests/slave_recovery_tests.cpp 9cf1d461cf5c6ac1f51ac963360a17e37f558505 
> 
> Diff: https://reviews.apache.org/r/55973/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55971: Updated the agent to be MULTI_ROLE capable.

[Guangya Liu]

> On 一月 30, 2017, 1:46 a.m., Guangya Liu wrote:
> > src/slave/slave.cpp, line 5244
> > 
> >
> > How about include `resources` in the log message?
> 
> Benjamin Mahler wrote:
> Hm.. I'm having a hard time seeing how that would be helpful here, any 
> thoughts?

My intention is having `resources` in the log so that we can know which 
resources do not allocation info in this framework.


- Guangya


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


On 一月 27, 2017, 12:27 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55971/
> ---
> 
> (Updated 一月 27, 2017, 12:27 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-6964
> https://issues.apache.org/jira/browse/MESOS-6964
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With the addition of MULTI_ROLE support, the agent needs to ensure
> that allocated resources reported to the master have the
> `Resource.AllocationInfo` set. The approach taken here is to set
> it only in memory upon recovering tasks and executors. Note that
> once we allow a framework to modify its roles, we need to update
> the agent to re-persist the resources with injected allocation
> info (rather than just setting it in memory).
> 
> 
> Diffs
> -
> 
>   src/slave/resource_estimators/fixed.cpp 
> 2c1268c3423091c6a419020a3af97255de55db0a 
>   src/slave/slave.cpp 0548b04073c0ba4adfc4433d75fd18c2ba79d891 
> 
> Diff: https://reviews.apache.org/r/55971/diff/
> 
> 
> Testing
> ---
> 
> The tests pass at the end of this review chain.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

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

[Guangya Liu]

> On 一月 28, 2017, 11:45 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1133-1137
> > 
> >
> > Add `role` here in the log message?
> 
> Benjamin Mahler wrote:
> Hm.. not sure what you mean, the role will be displayed when printing the 
> resources. Can you clarify?

I see, the `role` is already in `resource`, so no need to add it.


- Guangya


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


On 一月 25, 2017, 9:55 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55870/
> ---
> 
> (Updated 一月 25, 2017, 9: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 56284: Added ProvisionerDockerLocalStoreTest.SkippedLayers test.

[Timothy Chen]

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




src/tests/containerizer/provisioner_docker_tests.cpp (line 396)


How about making this a bit simpler:
"This test verifies that the store skips layers that are already pulled"


- Timothy Chen


On Feb. 3, 2017, 6:08 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56284/
> ---
> 
> (Updated Feb. 3, 2017, 6:08 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-7045
> https://issues.apache.org/jira/browse/MESOS-7045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test that verifies that Docker `Store` is OK when a puller skips 
> layers that already are in the store.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> af9987f88205d00d091f35fa734d5667506aaffd 
> 
> Diff: https://reviews.apache.org/r/56284/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 55973: Update the tests to handle MULTI_ROLE support.

[Benjamin Mahler]

> On Feb. 2, 2017, 6:01 a.m., Michael Park wrote:
> > src/tests/resource_offers_tests.cpp, line 266
> > 
> >
> > Was this for debugging?

Yes, I had left it in since it seemed helpful for the next person.


> On Feb. 2, 2017, 6:01 a.m., Michael Park wrote:
> > src/tests/master_allocator_tests.cpp, lines 1733-1737
> > 
> >
> > It's not obvious to me that this is the same test.
> > I guess in this case it is, but perhaps consider preserving the test.
> > 
> > ```cpp
> > auto unallocated = ...;
> > 
> > EXPECT_EQ(agentResources, unallocated(recoveredResources1.get()));
> > EXPECT_EQ(agentResources, unallocated(recoveredResources2.get()));
> > EXPECT_EQ(agentResources, unallocated(recoveredResources3.get()));
> > ```

Will update it per your suggestion.


- Benjamin


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


On Jan. 27, 2017, 12:34 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55973/
> ---
> 
> (Updated Jan. 27, 2017, 12:34 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A number of tests and example frameworks assume that allocated
> resources did not look different from unallocated resources.
> This updates the tests to reflect the presence of
> `Resource.AllocationInfo`.
> 
> 
> Diffs
> -
> 
>   src/examples/disk_full_framework.cpp 
> e13d4c8a427905793dda9bb01c52b6d372c19150 
>   src/examples/dynamic_reservation_framework.cpp 
> 4d3db965e18d79ed3e1759bb5f6cb41104562f47 
>   src/examples/no_executor_framework.cpp 
> e82ae9a9ea23c7d181b071f0e3f5071b3856d8a4 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
>   src/examples/test_framework.cpp 068b85d551012a390ba2a68bc88807103d3c31f3 
>   src/examples/test_http_framework.cpp 
> 258cb512803d1ed07c7a5ce1465e5663e77b0977 
>   src/tests/api_tests.cpp 400ac6fcd420508509291249cad50679fbe9807d 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> ba268b6918be0e7226a975c38eff5620b076083f 
>   src/tests/hook_tests.cpp f4ef629768beabc014e14472c5818d117d51abe7 
>   src/tests/master_allocator_tests.cpp 
> 996762f25453f7a8a5e0b7b97006ee2a603cf8c4 
>   src/tests/master_tests.cpp da7094dbbafbb0ab1153a0a4a6fcabd63888d67a 
>   src/tests/mesos.hpp 71e919702bccd31bf1e14205cd1e30d473883d48 
>   src/tests/oversubscription_tests.cpp 
> 22ae069ab71c82c6a6e2f5783b13ebd74a9ccf25 
>   src/tests/partition_tests.cpp f03c5bd96861b6643a4ecd9e37775447f86c3710 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 695029fe0cb91d668d6a8495f3ccd0c69c883cd2 
>   src/tests/persistent_volume_tests.cpp 
> 468a85b4a6ce09592341afd07ce12a03f5fc4f73 
>   src/tests/reservation_endpoints_tests.cpp 
> 7bc59c2ded9f7e6f1ccaa37456f654d18efcb2e2 
>   src/tests/reservation_tests.cpp b7061de0dfcc79548d507c7d70376c82d5d647ec 
>   src/tests/resource_offers_tests.cpp 
> 72ceb8696a7277f9add6d89adb55aa08665bdeb5 
>   src/tests/role_tests.cpp 0d0f1a94ff9aaec6ea04280282a52ac88dc6b273 
>   src/tests/slave_recovery_tests.cpp 9cf1d461cf5c6ac1f51ac963360a17e37f558505 
> 
> Diff: https://reviews.apache.org/r/55973/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55971: Updated the agent to be MULTI_ROLE capable.

[Benjamin Mahler]

> On Feb. 1, 2017, 9:01 p.m., Michael Park wrote:
> > src/slave/slave.cpp, lines 5227-5229
> > 
> >
> > Could we add a note here about how checking for `MULTI_ROLE` 
> > capabitility isn't sufficient since there could be tasks persisted from 
> > before a framework upgrade? I find it pretty tricky, and worth noting.

Good point, will do.


- Benjamin


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


On Jan. 27, 2017, 12:27 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55971/
> ---
> 
> (Updated Jan. 27, 2017, 12:27 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-6964
> https://issues.apache.org/jira/browse/MESOS-6964
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With the addition of MULTI_ROLE support, the agent needs to ensure
> that allocated resources reported to the master have the
> `Resource.AllocationInfo` set. The approach taken here is to set
> it only in memory upon recovering tasks and executors. Note that
> once we allow a framework to modify its roles, we need to update
> the agent to re-persist the resources with injected allocation
> info (rather than just setting it in memory).
> 
> 
> Diffs
> -
> 
>   src/slave/resource_estimators/fixed.cpp 
> 2c1268c3423091c6a419020a3af97255de55db0a 
>   src/slave/slave.cpp 0548b04073c0ba4adfc4433d75fd18c2ba79d891 
> 
> Diff: https://reviews.apache.org/r/55971/diff/
> 
> 
> Testing
> ---
> 
> The tests pass at the end of this review chain.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55970: Updated the master's HTTP operations to handle MULTI_ROLE changes.

[Benjamin Mahler]

> On Jan. 31, 2017, 6:32 p.m., Michael Park wrote:
> > src/master/validation.cpp, lines 1674-1700
> > 
> >
> > It's not clear to me why we need to unallocate the resources.
> > 
> > Presumably the resources in `Offer::Operation`, and the resources in 
> > `usedResources` should have the same `AllocationInfo` set? Is this not the 
> > case?

There are two cases:

(1) Allocated operation (framework accept)
(2) Unallocated operation (http endpoint)

Case 1 would be handled without changes but case 2 is not handled, so my 
approach is to unallocate both to perform the contains check, since we're only 
interested in the case where the volume is already in use. I'll add a comment 
to clarify this.


- Benjamin


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


On Jan. 26, 2017, 1:08 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55970/
> ---
> 
> (Updated Jan. 26, 2017, 1:08 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With the addition of MULTI_ROLE framework support, allocated
> resources in the master have `Resource.AllocationInfo` set. This
> means that the master's http endpoints that apply operations or
> perform checks between unallocated and allocated resources must
> be updated to continue to function correctly.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp a44621f39cb059e654a56f57f75b38947f3a4230 
>   src/master/quota_handler.cpp 6e6e7375219d34e6e8d011a025b5f5d70b87383b 
>   src/master/validation.cpp 5f134b781901f2de6a90fa6a10d42cc7629c27a0 
> 
> Diff: https://reviews.apache.org/r/55970/diff/
> 
> 
> Testing
> ---
> 
> The existing tests pass at the end of this review chain.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55972: Updated master to handle non-MULTI_ROLE agents.

[Benjamin Mahler]

> On Jan. 30, 2017, 1:46 a.m., Guangya Liu wrote:
> > src/master/master.cpp, lines 5620-5628
> > 
> >
> > Same comments as /r/55971/ here

Dropped per previous comments, would like to follow up and figure out what to 
do about this! :)


- Benjamin


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


On Jan. 27, 2017, 12:29 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55972/
> ---
> 
> (Updated Jan. 27, 2017, 12:29 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-6968
> https://issues.apache.org/jira/browse/MESOS-6968
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With the addition of MULTI_ROLE framweork support, allocated
> resources have a `Resource.AllocationInfo` set. While the new
> MULTI_ROLE capable agents will be sending allocated resources,
> the old agents may not be since they may not preserve the
> unknown fields.
> 
> To cope with this, the master ensures the `Resource.AllocationInfo`
> is set for non-MULTI_ROLE capable agents, by injecting it. Note
> that we can only do this so long as it is unambiguous! This will
> be prevented by not allowing frameworks with multiple to use
> agents without the MULTI_ROLE support, see: MESOS-6940.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 7e38af41ca16241dbbe3bc2e80c0848e82762a45 
>   src/master/master.cpp 0f2c7cd32161576fad742ef350dff64874b80854 
> 
> Diff: https://reviews.apache.org/r/55972/diff/
> 
> 
> Testing
> ---
> 
> The tests pass at the end of this review chain.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

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

[Benjamin Mahler]

> On Jan. 28, 2017, 11:45 a.m., Guangya Liu wrote:
> > include/mesos/allocator/allocator.hpp, line 91
> > 
> >
> > I think that we also need to reflect this in CHANGELOG to clarify that 
> > this interface was updated for multi_role support.

Sounds good, I added to the description of 
[MESOS-6762](https://issues.apache.org/jira/browse/MESOS-6762).


> On Jan. 28, 2017, 11:45 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 283
> > 
> >
> > I think that the reason you want to update here is want to keep 
> > consistent, right?

Yes


> On Jan. 28, 2017, 11:45 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1133-1137
> > 
> >
> > Add `role` here in the log message?

Hm.. not sure what you mean, the role will be displayed when printing the 
resources. Can you clarify?


> On Jan. 28, 2017, 11:45 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1133-1138
> > 
> >
> > Add `role` here in the log message?

Ditto here.


> On Jan. 28, 2017, 11:45 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1231-1232
> > 
> >
> > Nit, how about update the message a bit:
> > 
> > ```
> > Suppressed offers for roles {r1,r2} in framework xx-xx-xx-xx
> > ```
> > 
> > Ditto for the log message in `reviveOffers`.

Sure, sounds good.


> On Jan. 28, 2017, 11:45 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1455
> > 
> >
> > s/role/roles

This is referring to the reserved role, so this should remain singular.


> On Jan. 28, 2017, 11:45 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1495
> > 
> >
> > Why highlight the `unallocate()` here, I think that this was already 
> > done via `Resources::createStrippedScalarQuantity` and the resources being 
> > `flatten` here should not have `allocation info`.

Yes, sorry! This became stale as I was writing the code.


> On Jan. 28, 2017, 11:45 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 2059-2061
> > 
> >
> > Adding `role` in the log message here to clarify which role on this 
> > framework is being filtered?

Sounds good!


- Benjamin


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


On Jan. 25, 2017, 9: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, 9: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,
> 

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

[Benjamin Mahler]

> On Jan. 30, 2017, 4:12 p.m., Benjamin Bannier wrote:
> >

Thanks!


> On Jan. 30, 2017, 4:12 p.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.
> 
> Michael Park wrote:
> 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 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.

A couple of thoughts here:

Allocations occur to roles. Frameworks that are subscribed to the role can 
receive the allocations. A historical artifact is that we have a second level 
of implicitly weighted fair sharing between frameworks in a role to maintain 
our "pessimistic" offer technique, and so the allocator has knowlege of the 
frameworks. This approach was unfortunate since we have the framework id acting 
as a pseudo role but there is no control over the weights or quota for this 
pseudo role. The vision is that we should just perform allocations to roles, 
what this would mean is that if there are multiple frameworks listening to a 
role, they compete in a first come first serve fashion (if this is not desired, 
sub-roles can be used to impose sharing and quota constraints between the 
frameworks).

Given this, I suspect we could design the allocator to have no knowledge of 
what a framework is, and rather have the allocator operate purely on roles. 
This would lead to a "role" centric API. In this model, the master understands 
how to deliver the allocations to the roles based on which frameworks are 
subscribed to the role. The only wrinkle here that I see is that framework 
capabilities have an influence on allocation.

This is why I made the roles explicit, to reflect that allocations are occuring 
to roles. Thoughts?


> On Jan. 30, 2017, 4:12 p.m., Benjamin Bannier wrote:
> > include/mesos/allocator/allocator.hpp, line 264
> > 
> >
> > Do we need this detail of our allocator implementation in the generic 
> > interface?

Hm.. I'm not sure I follow how this is an implementation detail, it's currently 
an invariant we impose on the caller, so the caller should know about it, no?


> On Jan. 30, 2017, 4:12 p.m., Benjamin Bannier wrote:
> > include/mesos/allocator/allocator.hpp, lines 333-335
> > 
> >
> > Internal detail?

Hm.. I'm not sure I follow how this is an implementation detail, it's currently 
an invariant we impose on the caller, so the caller should know about it, no?


> On Jan. 30, 2017, 4:12 p.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 691-696
> > 
> >
> > Is this detailed knowledge of `Resource` internals necessary here? 
> > Below we use the same adjusted `operation` regardless of its type.

I'll remove the injection and leave the validation TODO.


> On Jan. 30, 2017, 4:12 p.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 709-710
> > 
> >
> > Lets try to put this code into the caller (e.g., `Master::_accept`). It 
> > seems that 

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

[Benjamin Mahler]

> On Jan. 31, 2017, 7:25 a.m., Michael Park wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 3414-3417
> > 
> >
> > Should just use `foreachpair` twice here I think. A few places below as 
> > well.

Yeah we could, just seemed a bit cleaner to avoid introducing another resources 
variable.


> On Jan. 31, 2017, 7:25 a.m., Michael Park wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 3737-3739
> > 
> >
> > 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);
> > ```

Sounds good! Turns out I don't need to pull it out since now the adjustment of 
the operation mutates it rather than returning a copy.


- Benjamin


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


On Jan. 26, 2017, 12:46 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55967/
> ---
> 
> (Updated Jan. 26, 2017, 12:46 a.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 56004: Fixed MULTI_ROLE related bugs when updating framework info.

[Benjamin Mahler]

> On Jan. 30, 2017, 10:26 a.m., Benjamin Bannier wrote:
> > src/master/master.hpp, line 2448
> > 
> >
> > Not yours, but should likely be
> > 
> > if (source.capabilities.empty()) {
> >   info.clear_capabilities();
> > } else {
> >   info.mutable_capabilities()->CopyFrom(source.capabilities());
> > }

Hm.. are you referring to the capabilities block below using empty instead of 
size? I suppose that piece could even just be:

```
  info.mutable_capabilities()->CopyFrom(source.capabilities());
```

Since it's a repeated field.


- Benjamin


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


On Jan. 27, 2017, 12: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, 12: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 55967: Update the allocator unit tests to reflect MULTI_ROLE support.

[Benjamin Mahler]

> On Jan. 28, 2017, 1:16 p.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 3942
> > 
> >
> > Need call `allocation.allocate("*");` here to make sure the agent can 
> > be added successfully.

Good catch Guangya! Unfortunately I didn't run the benchmark, I wish they ran 
in a reasonable amount of time :(


> On Jan. 28, 2017, 1:16 p.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 4093
> > 
> >
> > Need call `allocation.allocate("*");` here to make sure the agent can 
> > be added successfully.

Thanks! Needs to be role1 though FWICT.


- Benjamin


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


On Jan. 26, 2017, 12:46 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55967/
> ---
> 
> (Updated Jan. 26, 2017, 12:46 a.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 56004: Fixed MULTI_ROLE related bugs when updating framework info.

[Benjamin Mahler]

> On Feb. 1, 2017, 11:38 p.m., Michael Park wrote:
> > src/master/master.hpp, lines 2441-2450
> > 
> >
> > Could we just follow this pattern?
> > 
> > ```cpp
> > if (source.has_role()) {
> >   info.set_role(source.role());
> > } else {
> >   info.clear_role();
> > }
> > 
> > if (source.roles_size() > 0) {
> >   info.mutable_roles()->CopyFrom(source.roles());
> > } else {
> >   info.clear_roles();
> > }
> > ```

I found this harder to follow. The one I went with was: wipe the fields and 
then set based on source. The suggestion here seems to be: if the source has it 
set, overwrite, otherwise wipe, which just seemed a little less clear to me. 
Thoughts?


- Benjamin


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


On Jan. 27, 2017, 12: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, 12: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 55973: Update the tests to handle MULTI_ROLE support.

[Benjamin Mahler]

> On Jan. 30, 2017, 4:44 a.m., Guangya Liu wrote:
> > src/examples/dynamic_reservation_framework.cpp, line 123
> > 
> >
> > A question here, why need `apply` resources here? What is the problem 
> > is we keeps check if the `unreserved` contains `Allocated TASK RESOURCES`?

It just seemed a bit unfortunate that this code used 'contains' to assume apply 
would succeed, rather than directly applying. Thoughts?


- Benjamin


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


On Jan. 27, 2017, 12:34 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55973/
> ---
> 
> (Updated Jan. 27, 2017, 12:34 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A number of tests and example frameworks assume that allocated
> resources did not look different from unallocated resources.
> This updates the tests to reflect the presence of
> `Resource.AllocationInfo`.
> 
> 
> Diffs
> -
> 
>   src/examples/disk_full_framework.cpp 
> e13d4c8a427905793dda9bb01c52b6d372c19150 
>   src/examples/dynamic_reservation_framework.cpp 
> 4d3db965e18d79ed3e1759bb5f6cb41104562f47 
>   src/examples/no_executor_framework.cpp 
> e82ae9a9ea23c7d181b071f0e3f5071b3856d8a4 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
>   src/examples/test_framework.cpp 068b85d551012a390ba2a68bc88807103d3c31f3 
>   src/examples/test_http_framework.cpp 
> 258cb512803d1ed07c7a5ce1465e5663e77b0977 
>   src/tests/api_tests.cpp 400ac6fcd420508509291249cad50679fbe9807d 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> ba268b6918be0e7226a975c38eff5620b076083f 
>   src/tests/hook_tests.cpp f4ef629768beabc014e14472c5818d117d51abe7 
>   src/tests/master_allocator_tests.cpp 
> 996762f25453f7a8a5e0b7b97006ee2a603cf8c4 
>   src/tests/master_tests.cpp da7094dbbafbb0ab1153a0a4a6fcabd63888d67a 
>   src/tests/mesos.hpp 71e919702bccd31bf1e14205cd1e30d473883d48 
>   src/tests/oversubscription_tests.cpp 
> 22ae069ab71c82c6a6e2f5783b13ebd74a9ccf25 
>   src/tests/partition_tests.cpp f03c5bd96861b6643a4ecd9e37775447f86c3710 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 695029fe0cb91d668d6a8495f3ccd0c69c883cd2 
>   src/tests/persistent_volume_tests.cpp 
> 468a85b4a6ce09592341afd07ce12a03f5fc4f73 
>   src/tests/reservation_endpoints_tests.cpp 
> 7bc59c2ded9f7e6f1ccaa37456f654d18efcb2e2 
>   src/tests/reservation_tests.cpp b7061de0dfcc79548d507c7d70376c82d5d647ec 
>   src/tests/resource_offers_tests.cpp 
> 72ceb8696a7277f9add6d89adb55aa08665bdeb5 
>   src/tests/role_tests.cpp 0d0f1a94ff9aaec6ea04280282a52ac88dc6b273 
>   src/tests/slave_recovery_tests.cpp 9cf1d461cf5c6ac1f51ac963360a17e37f558505 
> 
> Diff: https://reviews.apache.org/r/55973/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

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

[Benjamin Mahler]

> On Jan. 30, 2017, 1: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?
> 
> Benjamin Bannier wrote:
> This whole block (and also the added code) are related to any changes in 
> framework capabilities, right?

Yeah I would say that this is captured by the comments that are already 
present, no?


- Benjamin


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


On Jan. 27, 2017, 12: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, 12: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 56249: Cleaned up code style slightly in DRF sorter.

[Michael Park]

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


Fix it, then Ship it!





src/master/allocator/sorter/drf/sorter.cpp (line 390)


Do you not want to just go all the way to this?
```cpp
foreach (const Client& client, clients) {
  // ...
}
```

Here and below.


- Michael Park


On Feb. 2, 2017, 2:59 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56249/
> ---
> 
> (Updated Feb. 2, 2017, 2:59 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Cleaned up code style slightly in DRF sorter.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 5681a5d78a7bdde820c3a8633d742d9d6412f1c7 
> 
> Diff: https://reviews.apache.org/r/56249/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 56245: Fixed typos in name of test suite.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Feb. 2, 2017, 2:58 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56245/
> ---
> 
> (Updated Feb. 2, 2017, 2:58 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed typos in name of test suite.
> 
> 
> Diffs
> -
> 
>   src/tests/role_tests.cpp 0d0f1a94ff9aaec6ea04280282a52ac88dc6b273 
> 
> Diff: https://reviews.apache.org/r/56245/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

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

[Jacob Janco]

> On Feb. 3, 2017, 10:45 a.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1263
> > 
> >
> > Could you follow up with a patch so this call is `dispatch`'ed?
> > 
> > Right now, if the `HierarchicalAllocatorProcess` goes away after 
> > `allocate`, but before the `AnyCallback` finishes, 
> > `this->allocationInterval` might be referencing garbage.
> 
> Jiang Yan Xu wrote:
> `delay` is already dispatched isn't it?
> 
> Benjamin Bannier wrote:
> Yes, the `delay`'ed part is safe as it dispatches to a specific process. 
> The issue here is with the `AnyCallBack` (the full argument to `onAny`) since 
> it does capture a raw ptr (`this`), but is not dispatched.
> 
> Jiang Yan Xu wrote:
> Ah I see what you mean now, thanks! I guess we can copy 
> `allocationInterval` in.
> 
> Jacob Janco wrote:
> ^I'll submit a patch with the above fix.

https://reviews.apache.org/r/56296


- Jacob


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


On Jan. 31, 2017, 1:46 a.m., Jacob Janco wrote:
> 
> ---
> 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.
> 
> 
> 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 
> 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 56296: Fix to potential dangling pointer in `batch()`.

[Jacob Janco]

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

(Updated Feb. 3, 2017, 3:53 p.m.)


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


Repository: mesos


Description
---

- If `HierarchicalAllocatorProcess` gone after
  allocate but before `AnyCallback` then
  `this->allocationInterval` may reference
  garbage.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
5f540569043df9d2bb75416c8c36bb4dd7bd68a1 

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


Testing (updated)
---

make check.


Thanks,

Jacob Janco

Re: Review Request 56296: Fix to potential dangling pointer in `batch()`.

[Jiang Yan Xu]

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


Ship it!




Ship It!

- Jiang Yan Xu


On Feb. 3, 2017, 3:50 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56296/
> ---
> 
> (Updated Feb. 3, 2017, 3:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - If `HierarchicalAllocatorProcess` gone after
>   allocate but before `AnyCallback` then
>   `this->allocationInterval` may reference
>   garbage.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5f540569043df9d2bb75416c8c36bb4dd7bd68a1 
> 
> Diff: https://reviews.apache.org/r/56296/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Review Request 56296: Fix to potential dangling pointer in `batch()`.

[Jacob Janco]

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

Review request for mesos.


Repository: mesos


Description
---

- If `HierarchicalAllocatorProcess` gone after
  allocate but before `AnyCallback` then
  `this->allocationInterval` may reference
  garbage.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
5f540569043df9d2bb75416c8c36bb4dd7bd68a1 

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


Testing
---


Thanks,

Jacob Janco

Re: Review Request 56292: Updated isolator module docs with checkpointing information.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Feb. 3, 2017, 3:06 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56292/
> ---
> 
> (Updated Feb. 3, 2017, 3:06 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds information to the module documentation
> which specifies the standard location for checkpointing
> custom isolator module state.
> 
> 
> Diffs
> -
> 
>   docs/modules.md 701b01fb08d85a1f65adaade2162f8ee787916ae 
> 
> Diff: https://reviews.apache.org/r/56292/diff/
> 
> 
> Testing
> ---
> 
> Built the website in the repo's Docker container and viewed the module docs 
> in Chrome.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56293: Added '--runtime_dir' flag to the agent configuration docs.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Feb. 3, 2017, 3:05 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56293/
> ---
> 
> (Updated Feb. 3, 2017, 3:05 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds the `--runtime_dir` flag to the agent
> configuration documentation.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 6a30471d325f35e171e81a687c23fd69e9ec97ee 
> 
> Diff: https://reviews.apache.org/r/56293/diff/
> 
> 
> Testing
> ---
> 
> Built the website in the repo's Docker container and viewed config docs in 
> Chrome.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56270: Made the default executor launch multiple task groups.

[Vinod Kone]

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




src/launcher/default_executor.cpp (line 472)


s/pending/containerIds/



src/launcher/default_executor.cpp (line 493)


s/pending/containerIds/



src/launcher/default_executor.cpp (lines 686 - 725)


I'm wondering if it would be easier to grok if we locally generate a UUID 
for a task group and use that in the executor code.

also, do we really need `killTaskGroups`? looks like we could just look in 
`killed/killing` to avoid killing a task multiple times?



src/launcher/default_executor.cpp (line 1033)


you wouldn't need this if you key it on TaskID per my comment in the 
previous review.



src/tests/default_executor_tests.cpp (lines 333 - 334)


move this to #391.



src/tests/default_executor_tests.cpp (lines 340 - 341)


move this to #391.



src/tests/default_executor_tests.cpp 


i would keep the expectation here and ensure it is still pending after the 
first kill.


- Vinod Kone


On Feb. 3, 2017, 4:45 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56270/
> ---
> 
> (Updated Feb. 3, 2017, 4:45 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6296
> https://issues.apache.org/jira/browse/MESOS-6296
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a task fails or is explicitly killed by the scheduler, the
> default on termination policy kills the particular task group.
> The executor commits suicide when there are no active tasks.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
>   src/tests/default_executor_tests.cpp 
> e8e0aa2f0d2508de6db03685c70d1ed9c1da3659 
> 
> Diff: https://reviews.apache.org/r/56270/diff/
> 
> 
> Testing
> ---
> 
> make check. Also, modified an existing test to launch/kill multiple task 
> groups.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 56284: Added ProvisionerDockerLocalStoreTest.SkippedLayers test.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56174, 56284]

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 Feb. 3, 2017, 6:08 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56284/
> ---
> 
> (Updated Feb. 3, 2017, 6:08 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-7045
> https://issues.apache.org/jira/browse/MESOS-7045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test that verifies that Docker `Store` is OK when a puller skips 
> layers that already are in the store.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> af9987f88205d00d091f35fa734d5667506aaffd 
> 
> Diff: https://reviews.apache.org/r/56284/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Review Request 56292: Updated isolator module docs with checkpointing information.

[Greg Mann]

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

Review request for mesos, Gilbert Song and Jie Yu.


Repository: mesos


Description
---

This patch adds information to the module documentation
which specifies the standard location for checkpointing
custom isolator module state.


Diffs
-

  docs/modules.md 701b01fb08d85a1f65adaade2162f8ee787916ae 

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


Testing
---

Built the website in the repo's Docker container and viewed the module docs in 
Chrome.


Thanks,

Greg Mann

Re: Review Request 56269: Introduced a `Container` struct on the default executor.

[Vinod Kone]

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




src/launcher/default_executor.cpp (line 89)


hmm. it's unfortunate that each task/container stores TaskInfos of all 
tasks in the task group.

can we have a helper function instead that returns a list of container IDs 
that belong to the same task group as a given container?

```
hashset taskGroupContainers(const ContainerID& containerId) {}
```

or store a shared pointer to TaskGroup?



src/launcher/default_executor.cpp (line 824)


looks like we are better off keying `containers` on `TaskID` rather than 
`ContainerID`?



src/launcher/default_executor.cpp (lines 1033 - 1046)


Can we consolidate killed/killing  into the `Container` struct as well? I 
think it will be easy to reason about.


- Vinod Kone


On Feb. 3, 2017, 4:45 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56269/
> ---
> 
> (Updated Feb. 3, 2017, 4:45 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6296
> https://issues.apache.org/jira/browse/MESOS-6296
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This helps us in consolidating various metadata associatd with
> an active child container. Also, this would be used in the future
> for figuring out the task group corresponding to a child container
> and then killing the other child containers belonging to the task
> group when one of the tasks fail; or if a task in the task group
> is explicitly killed by the scheduler.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
> 
> Diff: https://reviews.apache.org/r/56269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 56267: Made `kill` not use pipelining in the default executor.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Feb. 3, 2017, 4:45 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56267/
> ---
> 
> (Updated Feb. 3, 2017, 4:45 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: mesos-6296
> https://issues.apache.org/jira/browse/mesos-6296
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The previous code used to pipeline all the `KILL_NESTED_CONTAINER`
> calls on the same connection. This change removes this and invokes
> `post` for each child container. This simplifies the code and
> makes it more readable.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
> 
> Diff: https://reviews.apache.org/r/56267/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

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 Feb. 3, 2017, 10:01 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Refactor.


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 
1e0b9453ed0c4b2fa68f0258dcd6a597c33f2c14 
  src/tests/resources_utils.hpp 1f41f02babce5c8174ea2223f4dc7470452fbaf1 
  src/tests/resources_utils.cpp 2cef55f7312d671307e097c2c4960c8dcf45c1ff 

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]

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

(Updated Feb. 3, 2017, 10:01 p.m.)


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


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 
5f540569043df9d2bb75416c8c36bb4dd7bd68a1 

Diff: https://reviews.apache.org/r/55359/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 Feb. 3, 2017, 10:01 p.m.)


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


Changes
---

Refactor.


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

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 Feb. 3, 2017, 10:01 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Refactor.


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 
a99ed35169c0a4a1db3ac3b9b09f510f8fcddbfb 
  src/master/allocator/mesos/hierarchical.cpp 
5f540569043df9d2bb75416c8c36bb4dd7bd68a1 
  src/tests/persistent_volume_tests.cpp 
468a85b4a6ce09592341afd07ce12a03f5fc4f73 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 54878: Add some simple ldd() tests.

[Jiang Yan Xu]

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


Ship it!





src/tests/ldd_tests.cpp (line 79)


Two space indentation. No worries I fix it.


- Jiang Yan Xu


On Feb. 1, 2017, 3:52 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54878/
> ---
> 
> (Updated Feb. 1, 2017, 3:52 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add some simple ldd() tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 6c9be54fe0ce3faaa26e2f090773d1b77bb6e430 
>   src/tests/CMakeLists.txt 44b74ee591b14d7d69bc0c03c26167b33eaa789c 
>   src/tests/ldd_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/54878/diff/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25).
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 54712: Use the stout ELF parser to implement ldd.

[Jiang Yan Xu]

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


Ship it!




Ship It!

- Jiang Yan Xu


On Feb. 1, 2017, 4:15 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54712/
> ---
> 
> (Updated Feb. 1, 2017, 4:15 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use the stout ELF parser to implement an ldd() API that recursively
> gathers the link dependencies of a given program.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 09ef1aee680c6b91476092dbf61d4ca3a8d256bb 
>   src/Makefile.am 6c9be54fe0ce3faaa26e2f090773d1b77bb6e430 
>   src/linux/ldd.hpp PRE-CREATION 
>   src/linux/ldd.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/54712/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 56053: Added a 'SECRET' type to the 'Environment' protobuf message.

[Vinod Kone]

> On Jan. 31, 2017, 8:31 p.m., Vinod Kone wrote:
> > include/mesos/v1/mesos.proto, line 1892
> > 
> >
> > ditto.
> 
> Greg Mann wrote:
> Sorry I'm not sure precisely what this issue is referring to? I added 
> text to this comment regarding just one of `value` and `secret` being set.

I meant my same comment above re: "comment should be removed" in mesos.proto 
applies here, v1/mesos.proto, as well.


- Vinod


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


On Feb. 2, 2017, 7:39 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56053/
> ---
> 
> (Updated Feb. 2, 2017, 7:39 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Kapil Arya, Jan Schlicht, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7009
> https://issues.apache.org/jira/browse/MESOS-7009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a field of type `Secret` to the
> `Environment` protobuf message, enabling the passing
> of secrets into the environments of executors and
> tasks. Additional validation and test code is added
> as well.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto a08b8fb2edf26ae730467557ae0e33e3d4252593 
>   include/mesos/v1/mesos.proto 4a4609e7db659252ef7f1ca61f8ba079b901b7d0 
>   src/common/validation.cpp b2548ad87b4227d6e498c49b5694acb362f6281b 
>   src/tests/check_tests.cpp c88cd34fd214f111cff62591aa5fc03eb62567e4 
>   src/tests/health_check_tests.cpp 710cb66eff6c4447caa22772f0cdc97cfa582c50 
>   src/tests/master_validation_tests.cpp 
> edb57407e08cdbd8fbf10a9e1493cab3b4979bb8 
>   src/tests/slave_validation_tests.cpp 
> 5de771114982751e7796f55dcacd4384c6989efb 
> 
> Diff: https://reviews.apache.org/r/56053/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56251: Tightened assertions in sorter and allocator.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56245, 56246, 56247, 56248, 56249, 56250, 56251]

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 Feb. 3, 2017, 5:57 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56251/
> ---
> 
> (Updated Feb. 3, 2017, 5:57 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also replaced a mistaken comment.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5f540569043df9d2bb75416c8c36bb4dd7bd68a1 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 5681a5d78a7bdde820c3a8633d742d9d6412f1c7 
> 
> Diff: https://reviews.apache.org/r/56251/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

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

[Jacob Janco]

> On Feb. 3, 2017, 10:45 a.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1263
> > 
> >
> > Could you follow up with a patch so this call is `dispatch`'ed?
> > 
> > Right now, if the `HierarchicalAllocatorProcess` goes away after 
> > `allocate`, but before the `AnyCallback` finishes, 
> > `this->allocationInterval` might be referencing garbage.
> 
> Jiang Yan Xu wrote:
> `delay` is already dispatched isn't it?
> 
> Benjamin Bannier wrote:
> Yes, the `delay`'ed part is safe as it dispatches to a specific process. 
> The issue here is with the `AnyCallBack` (the full argument to `onAny`) since 
> it does capture a raw ptr (`this`), but is not dispatched.
> 
> Jiang Yan Xu wrote:
> Ah I see what you mean now, thanks! I guess we can copy 
> `allocationInterval` in.

^I'll submit a patch with the above fix.


- Jacob


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


On Jan. 31, 2017, 1:46 a.m., Jacob Janco wrote:
> 
> ---
> 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.
> 
> 
> 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 
> 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]

> On Feb. 3, 2017, 2:45 a.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1263
> > 
> >
> > Could you follow up with a patch so this call is `dispatch`'ed?
> > 
> > Right now, if the `HierarchicalAllocatorProcess` goes away after 
> > `allocate`, but before the `AnyCallback` finishes, 
> > `this->allocationInterval` might be referencing garbage.
> 
> Jiang Yan Xu wrote:
> `delay` is already dispatched isn't it?
> 
> Benjamin Bannier wrote:
> Yes, the `delay`'ed part is safe as it dispatches to a specific process. 
> The issue here is with the `AnyCallBack` (the full argument to `onAny`) since 
> it does capture a raw ptr (`this`), but is not dispatched.

Ah I see what you mean now, thanks! I guess we can copy `allocationInterval` in.


- Jiang Yan


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


On Jan. 30, 2017, 5:46 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Jan. 30, 2017, 5:46 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 
> 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
> 
>

Review Request 56291: Removed unnecessary mkdirs in ProvisionerDockerLocalStoreTest.*.

[Ilya Pronin]

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

Review request for mesos, Jie Yu and Timothy Chen.


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


Repository: mesos


Description
---

`LocalStoreTestWithTar` and `PullingSameImageSimutanuously` started with 
creating directories that were already created by `SetUp()` and directories 
that are not used.


Diffs
-

  src/tests/containerizer/provisioner_docker_tests.cpp 
af9987f88205d00d091f35fa734d5667506aaffd 

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


Testing
---

`make check`


Thanks,

Ilya Pronin

Re: Review Request 54821: Refactored Docker::run() to make it only aware of docker cli options.

[Jie Yu]

> On Feb. 3, 2017, 7:28 p.m., Jie Yu wrote:
> > src/docker/docker.hpp, line 194
> > 
> >
> > I'd try to avoid mesos:: prefixed type in this file. Can this just be a 
> > map?
> 
> Zhitao Li wrote:
> This could be a `vector`, but a `map` would destroy initial 
> order, which used to be sensitive to docker daemon.

Can you give an example which flags are order sensitive. I am just curious.

If that's the case, use LinkedHashMap in stout?


- Jie


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


On Feb. 3, 2017, 6:55 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Feb. 3, 2017, 6:55 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-6808
> https://issues.apache.org/jira/browse/MESOS-6808
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch creates a wrapper struct for all recognizable docker cli
> options, and separate logic of creating these options to a different
> common function.
> 
> This also enables us to overcome gmock's 10 argument limit.
> 
> No logic change happens in this refactoring patch.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp b63b060ba1c1d39dc1702368cf667831edbd39bd 
>   src/docker/executor.cpp d8c72b081fb9a37b3927b73cea3b47956a076691 
>   src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 31d63b1f239055d82470ace9024b584a2096dce4 
>   src/tests/containerizer/docker_tests.cpp 
> 9667d434486c1832f180a297a39a3d5dae6a26bd 
>   src/tests/mock_docker.hpp 829a760d54ad8c7b88256ae5df4c88c9fb18df71 
>   src/tests/mock_docker.cpp 02b6065a01e7e52b0edb38676dfb1bb475584502 
> 
> Diff: https://reviews.apache.org/r/54821/diff/
> 
> 
> Testing
> ---
> 
> `make check` with ROOT and DOCKER filter.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 54821: Refactored Docker::run() to make it only aware of docker cli options.

[Zhitao Li]

> On Feb. 3, 2017, 7:28 p.m., Jie Yu wrote:
> > This is a partial review. Is there way to run this in your staging/prod 
> > environment. This patch touches the core part of Docker containerizer which 
> > we don't have a good test coverage, and has a very board impact.

I will run all tests with a `DOCKER` filter, and also use `mesos-execute` with 
docker containerizer to test this out.

If we want more testing, I can try to build an actual binary to test it out in 
our staging environment but it'll take sometime to get it done, and even that 
doesn't test every combinations of arguments.


> On Feb. 3, 2017, 7:28 p.m., Jie Yu wrote:
> > src/docker/docker.hpp, line 194
> > 
> >
> > I'd try to avoid mesos:: prefixed type in this file. Can this just be a 
> > map?

This could be a `vector`, but a `map` would destroy initial order, 
which used to be sensitive to docker daemon.


> On Feb. 3, 2017, 7:28 p.m., Jie Yu wrote:
> > src/docker/docker.cpp, line 527
> > 
> >
> > This changes the logic? Where is MIN_MEMORY?

Adding the previous logic back right now.


- Zhitao


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


On Feb. 3, 2017, 6:55 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Feb. 3, 2017, 6:55 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-6808
> https://issues.apache.org/jira/browse/MESOS-6808
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch creates a wrapper struct for all recognizable docker cli
> options, and separate logic of creating these options to a different
> common function.
> 
> This also enables us to overcome gmock's 10 argument limit.
> 
> No logic change happens in this refactoring patch.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp b63b060ba1c1d39dc1702368cf667831edbd39bd 
>   src/docker/executor.cpp d8c72b081fb9a37b3927b73cea3b47956a076691 
>   src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 31d63b1f239055d82470ace9024b584a2096dce4 
>   src/tests/containerizer/docker_tests.cpp 
> 9667d434486c1832f180a297a39a3d5dae6a26bd 
>   src/tests/mock_docker.hpp 829a760d54ad8c7b88256ae5df4c88c9fb18df71 
>   src/tests/mock_docker.cpp 02b6065a01e7e52b0edb38676dfb1bb475584502 
> 
> Diff: https://reviews.apache.org/r/54821/diff/
> 
> 
> Testing
> ---
> 
> `make check` with ROOT and DOCKER filter.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 56250: Cleaned up headers in sorter and allocator metrics.

[Neil Conway]

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

(Updated Feb. 3, 2017, 8:21 p.m.)


Review request for mesos and Michael Park.


Changes
---

More header/include fixes, per review suggestion.


Summary (updated)
-

Cleaned up headers in sorter and allocator metrics.


Repository: mesos


Description (updated)
---

Cleaned up headers in sorter and allocator metrics.


Diffs (updated)
-

  src/master/allocator/mesos/metrics.hpp 
753f90acc52ada84883cbbe3350e61d1e1eaff48 
  src/master/allocator/mesos/metrics.cpp 
a36d21c297160bc1c9f43a22743fd5448d7ae5ac 
  src/master/allocator/sorter/drf/sorter.cpp 
5681a5d78a7bdde820c3a8633d742d9d6412f1c7 

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


Testing
---

`make check`


Thanks,

Neil Conway

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

[Benjamin Bannier]

> On Feb. 3, 2017, 11:45 a.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1263
> > 
> >
> > Could you follow up with a patch so this call is `dispatch`'ed?
> > 
> > Right now, if the `HierarchicalAllocatorProcess` goes away after 
> > `allocate`, but before the `AnyCallback` finishes, 
> > `this->allocationInterval` might be referencing garbage.
> 
> Jiang Yan Xu wrote:
> `delay` is already dispatched isn't it?

Yes, the `delay`'ed part is safe as it dispatches to a specific process. The 
issue here is with the `AnyCallBack` (the full argument to `onAny`) since it 
does capture a raw ptr (`this`), but is not dispatched.


- Benjamin


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


On Jan. 31, 2017, 2:46 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Jan. 31, 2017, 2:46 a.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 
> 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 55901: Added support for command health checks to the default executor.

[Gastón Kleiman]

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

(Updated Feb. 3, 2017, 7:52 p.m.)


Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent huang, 
and Vinod Kone.


Changes
---

Split into two patches.


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


Repository: mesos


Description
---

Added support for command health checks to the default executor.


Diffs (updated)
-

  src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
  src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
  src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
  src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb 

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


Testing
---

Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
passes on Linux, but not on macOS.


Thanks,

Gastón Kleiman

Review Request 56288: Improved the wording of what's logged on command health check timeouts.

[Gastón Kleiman]

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

Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent huang, 
and Vinod Kone.


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


Repository: mesos


Description
---

Improved the wording of what's logged on command health check timeouts.


Diffs
-

  src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 

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


Testing
---

None, not a functional change.


Thanks,

Gastón Kleiman

Re: Review Request 55160: Added test for DockerContainerizer when `cgroups_enable_cfs` is set.

[Zhitao Li]

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

(Updated Feb. 3, 2017, 7:34 p.m.)


Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.


Changes
---

Triggering build again.


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


Repository: mesos


Description
---

Added test for DockerContainerizer when `cgroups_enable_cfs` is set.


Diffs (updated)
-

  src/tests/containerizer/docker_containerizer_tests.cpp 
31d63b1f239055d82470ace9024b584a2096dce4 

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


Testing
---

Run the new test.


Thanks,

Zhitao Li

Re: Review Request 51052: Made mesos-docker-execute understand cgroups_enable_cfs.

[Zhitao Li]

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

(Updated Feb. 3, 2017, 7:34 p.m.)


Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.


Changes
---

Fix typo.


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


Repository: mesos


Description
---

This fixed cpu quota for command executor (which runs outside
of the docker container) by ensuing --cpu-quota flag to docker
run.


Diffs (updated)
-

  src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
  src/docker/docker.cpp b63b060ba1c1d39dc1702368cf667831edbd39bd 
  src/docker/executor.hpp 8385631fd170f97b28c4ca3596255ab0546774d6 
  src/docker/executor.cpp d8c72b081fb9a37b3927b73cea3b47956a076691 
  src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d 
  src/tests/containerizer/docker_tests.cpp 
9667d434486c1832f180a297a39a3d5dae6a26bd 

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


Testing
---

I am now able to make docker containers launched through mesos-execute have a 
cpu quota.

Also making sure `make check` still works on mac os for the linux only flag.


Thanks,

Zhitao Li

Re: Review Request 51052: Made mesos-docker-execute understand cgroups_enable_cfs.

[Zhitao Li]

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




src/docker/docker.cpp (line 786)


Fixing this typo right now.


- Zhitao Li


On Feb. 3, 2017, 6:56 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51052/
> ---
> 
> (Updated Feb. 3, 2017, 6:56 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-6134
> https://issues.apache.org/jira/browse/MESOS-6134
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixed cpu quota for command executor (which runs outside
> of the docker container) by ensuing --cpu-quota flag to docker
> run.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp b63b060ba1c1d39dc1702368cf667831edbd39bd 
>   src/docker/executor.hpp 8385631fd170f97b28c4ca3596255ab0546774d6 
>   src/docker/executor.cpp d8c72b081fb9a37b3927b73cea3b47956a076691 
>   src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d 
>   src/tests/containerizer/docker_tests.cpp 
> 9667d434486c1832f180a297a39a3d5dae6a26bd 
> 
> Diff: https://reviews.apache.org/r/51052/diff/
> 
> 
> Testing
> ---
> 
> I am now able to make docker containers launched through mesos-execute have a 
> cpu quota.
> 
> Also making sure `make check` still works on mac os for the linux only flag.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 54821: Refactored Docker::run() to make it only aware of docker cli options.

[Jie Yu]

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



This is a partial review. Is there way to run this in your staging/prod 
environment. This patch touches the core part of Docker containerizer which we 
don't have a good test coverage, and has a very board impact.


src/docker/docker.hpp (line 194)


I'd try to avoid mesos:: prefixed type in this file. Can this just be a map?



src/docker/docker.cpp (line 516)


This changes the logic? Where is MIN_MEMORY?


- Jie Yu


On Feb. 3, 2017, 6:55 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Feb. 3, 2017, 6:55 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-6808
> https://issues.apache.org/jira/browse/MESOS-6808
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch creates a wrapper struct for all recognizable docker cli
> options, and separate logic of creating these options to a different
> common function.
> 
> This also enables us to overcome gmock's 10 argument limit.
> 
> No logic change happens in this refactoring patch.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp b63b060ba1c1d39dc1702368cf667831edbd39bd 
>   src/docker/executor.cpp d8c72b081fb9a37b3927b73cea3b47956a076691 
>   src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 31d63b1f239055d82470ace9024b584a2096dce4 
>   src/tests/containerizer/docker_tests.cpp 
> 9667d434486c1832f180a297a39a3d5dae6a26bd 
>   src/tests/mock_docker.hpp 829a760d54ad8c7b88256ae5df4c88c9fb18df71 
>   src/tests/mock_docker.cpp 02b6065a01e7e52b0edb38676dfb1bb475584502 
> 
> Diff: https://reviews.apache.org/r/54821/diff/
> 
> 
> Testing
> ---
> 
> `make check` with ROOT and DOCKER filter.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 55732: Added `--3way` option to apply-reviews.py.

[Jiang Yan Xu]

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


Ship it!




Ship It!

- Jiang Yan Xu


On Feb. 3, 2017, 10:07 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55732/
> ---
> 
> (Updated Feb. 3, 2017, 10:07 a.m.)
> 
> 
> Review request for mesos, Jason Lai, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-4119
> https://issues.apache.org/jira/browse/MESOS-4119
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This can help on some patches which includes conflicts that can be
> resolved by 3 way merge.
> 
> 
> Diffs
> -
> 
>   support/apply-reviews.py cdfdd58f9310f95b075c025048b6523fc1a4da65 
> 
> Diff: https://reviews.apache.org/r/55732/diff/
> 
> 
> Testing
> ---
> 
> With this fix, I was able to apply the chain in r/52534, which has a conflict 
> otherwise.
> 
> Bash log:
> ```
> 
> $ python ./support/apply-reviews.py -r 52534 -c --3way
> 2017-01-23 10:00:57 URL:https://reviews.apache.org/r/51027/diff/raw/ 
> [9830/9830] -> "51027.patch" [1]
> [3way 93791b1] Track allocation candidates to bound allocator.
>  Author: Jacob Janco 
>  2 files changed, 97 insertions(+), 54 deletions(-)
> 2017-01-23 10:00:59 URL:https://reviews.apache.org/r/52534/diff/raw/ 
> [2950/2950] -> "52534.patch" [1]
> error: patch failed: src/master/allocator/mesos/hierarchical.hpp:224
> Falling back to three-way merge...
> Applied patch to 'src/master/allocator/mesos/hierarchical.hpp' cleanly.
> [3way 6f24fe7] Dispatch filter expiration twice.
>  Author: Jacob Janco 
>  2 files changed, 32 insertions(+), 8 deletions(-)
>  ```
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [55900, 55901]

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 Feb. 3, 2017, 5:15 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 3, 2017, 5:15 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
>   src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55160: Added test for DockerContainerizer when `cgroups_enable_cfs` is set.

[Zhitao Li]

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

(Updated Feb. 3, 2017, 6:56 p.m.)


Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.


Changes
---

Rebase with master.


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


Repository: mesos


Description
---

Added test for DockerContainerizer when `cgroups_enable_cfs` is set.


Diffs (updated)
-

  src/tests/containerizer/docker_containerizer_tests.cpp 
31d63b1f239055d82470ace9024b584a2096dce4 

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


Testing
---

Run the new test.


Thanks,

Zhitao Li

Re: Review Request 51052: Made mesos-docker-execute understand cgroups_enable_cfs.

[Zhitao Li]

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

(Updated Feb. 3, 2017, 6:56 p.m.)


Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.


Changes
---

Rebase with master.


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


Repository: mesos


Description
---

This fixed cpu quota for command executor (which runs outside
of the docker container) by ensuing --cpu-quota flag to docker
run.


Diffs (updated)
-

  src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
  src/docker/docker.cpp b63b060ba1c1d39dc1702368cf667831edbd39bd 
  src/docker/executor.hpp 8385631fd170f97b28c4ca3596255ab0546774d6 
  src/docker/executor.cpp d8c72b081fb9a37b3927b73cea3b47956a076691 
  src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d 
  src/tests/containerizer/docker_tests.cpp 
9667d434486c1832f180a297a39a3d5dae6a26bd 

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


Testing
---

I am now able to make docker containers launched through mesos-execute have a 
cpu quota.

Also making sure `make check` still works on mac os for the linux only flag.


Thanks,

Zhitao Li

Re: Review Request 54821: Refactored Docker::run() to make it only aware of docker cli options.

[Zhitao Li]

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

(Updated Feb. 3, 2017, 6:55 p.m.)


Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.


Changes
---

Rebase with master.


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


Repository: mesos


Description
---

This patch creates a wrapper struct for all recognizable docker cli
options, and separate logic of creating these options to a different
common function.

This also enables us to overcome gmock's 10 argument limit.

No logic change happens in this refactoring patch.


Diffs (updated)
-

  src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
  src/docker/docker.cpp b63b060ba1c1d39dc1702368cf667831edbd39bd 
  src/docker/executor.cpp d8c72b081fb9a37b3927b73cea3b47956a076691 
  src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d 
  src/tests/containerizer/docker_containerizer_tests.cpp 
31d63b1f239055d82470ace9024b584a2096dce4 
  src/tests/containerizer/docker_tests.cpp 
9667d434486c1832f180a297a39a3d5dae6a26bd 
  src/tests/mock_docker.hpp 829a760d54ad8c7b88256ae5df4c88c9fb18df71 
  src/tests/mock_docker.cpp 02b6065a01e7e52b0edb38676dfb1bb475584502 

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


Testing
---

`make check` with ROOT and DOCKER filter.


Thanks,

Zhitao Li

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

[Jiang Yan Xu]

> On Feb. 3, 2017, 2:45 a.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1263
> > 
> >
> > Could you follow up with a patch so this call is `dispatch`'ed?
> > 
> > Right now, if the `HierarchicalAllocatorProcess` goes away after 
> > `allocate`, but before the `AnyCallback` finishes, 
> > `this->allocationInterval` might be referencing garbage.

`delay` is already dispatched isn't it?


- Jiang Yan


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


On Jan. 30, 2017, 5:46 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Jan. 30, 2017, 5:46 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 
> 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 56174: Added skipping already stored layers to local Docker puller.

[Ilya Pronin]

> On Feb. 2, 2017, 3:39 a.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp, line 287
> > 
> >
> > LGTM, do we have tests for the local puller already? Will be good to 
> > have a test around this.

I added a test for this feature in `LocalPuller`. And a test for `Store` as a 
separate RR: https://reviews.apache.org/r/56284/


- Ilya


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


On Feb. 3, 2017, 6:07 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56174/
> ---
> 
> (Updated Feb. 3, 2017, 6:07 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-7045
> https://issues.apache.org/jira/browse/MESOS-7045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a layer is already in the store, there's no need to extract it. 
> `RegistryPuller` already does this and `Store` is ready for such puller 
> behaviour.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> ee391af898886bff9e5b911697f725c5ea53ebd8 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> af9987f88205d00d091f35fa734d5667506aaffd 
> 
> Diff: https://reviews.apache.org/r/56174/diff/
> 
> 
> Testing
> ---
> 
> Added a test verifying that local puller will skip layes that are already in 
> the store. Ran `make check`.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Review Request 56284: Added ProvisionerDockerLocalStoreTest.SkippedLayers test.

[Ilya Pronin]

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

Review request for mesos, Jie Yu and Timothy Chen.


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


Repository: mesos


Description
---

Added a test that verifies that Docker `Store` is OK when a puller skips layers 
that already are in the store.


Diffs
-

  src/tests/containerizer/provisioner_docker_tests.cpp 
af9987f88205d00d091f35fa734d5667506aaffd 

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


Testing
---

`make check`


Thanks,

Ilya Pronin

Re: Review Request 55732: Added `--3way` option to apply-reviews.py.

[Zhitao Li]

> On Feb. 2, 2017, 10:26 p.m., Michael Park wrote:
> > support/apply-reviews.py, line 123
> > 
> >
> > Is there something related to this patch that makes it not exist 
> > sometimes?

>From my testing, `git apply --3way` could remove the patch file when resolving 
>the conflict? I encounted some error complaining patch file non existing, so I 
>added this precondition.


- Zhitao


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


On Feb. 1, 2017, 6:45 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55732/
> ---
> 
> (Updated Feb. 1, 2017, 6:45 p.m.)
> 
> 
> Review request for mesos, Jason Lai, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-4119
> https://issues.apache.org/jira/browse/MESOS-4119
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This can help on some patches which includes conflicts that can be
> resolved by 3 way merge.
> 
> 
> Diffs
> -
> 
>   support/apply-reviews.py cdfdd58f9310f95b075c025048b6523fc1a4da65 
> 
> Diff: https://reviews.apache.org/r/55732/diff/
> 
> 
> Testing
> ---
> 
> With this fix, I was able to apply the chain in r/52534, which has a conflict 
> otherwise.
> 
> Bash log:
> ```
> 
> $ python ./support/apply-reviews.py -r 52534 -c --3way
> 2017-01-23 10:00:57 URL:https://reviews.apache.org/r/51027/diff/raw/ 
> [9830/9830] -> "51027.patch" [1]
> [3way 93791b1] Track allocation candidates to bound allocator.
>  Author: Jacob Janco 
>  2 files changed, 97 insertions(+), 54 deletions(-)
> 2017-01-23 10:00:59 URL:https://reviews.apache.org/r/52534/diff/raw/ 
> [2950/2950] -> "52534.patch" [1]
> error: patch failed: src/master/allocator/mesos/hierarchical.hpp:224
> Falling back to three-way merge...
> Applied patch to 'src/master/allocator/mesos/hierarchical.hpp' cleanly.
> [3way 6f24fe7] Dispatch filter expiration twice.
>  Author: Jacob Janco 
>  2 files changed, 32 insertions(+), 8 deletions(-)
>  ```
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 55732: Added `--3way` option to apply-reviews.py.

[Zhitao Li]

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

(Updated Feb. 3, 2017, 6:07 p.m.)


Review request for mesos, Jason Lai, Joseph Wu, and Michael Park.


Changes
---

Review comments.


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


Repository: mesos


Description
---

This can help on some patches which includes conflicts that can be
resolved by 3 way merge.


Diffs (updated)
-

  support/apply-reviews.py cdfdd58f9310f95b075c025048b6523fc1a4da65 

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


Testing
---

With this fix, I was able to apply the chain in r/52534, which has a conflict 
otherwise.

Bash log:
```

$ python ./support/apply-reviews.py -r 52534 -c --3way
2017-01-23 10:00:57 URL:https://reviews.apache.org/r/51027/diff/raw/ 
[9830/9830] -> "51027.patch" [1]
[3way 93791b1] Track allocation candidates to bound allocator.
 Author: Jacob Janco 
 2 files changed, 97 insertions(+), 54 deletions(-)
2017-01-23 10:00:59 URL:https://reviews.apache.org/r/52534/diff/raw/ 
[2950/2950] -> "52534.patch" [1]
error: patch failed: src/master/allocator/mesos/hierarchical.hpp:224
Falling back to three-way merge...
Applied patch to 'src/master/allocator/mesos/hierarchical.hpp' cleanly.
[3way 6f24fe7] Dispatch filter expiration twice.
 Author: Jacob Janco 
 2 files changed, 32 insertions(+), 8 deletions(-)
 ```


Thanks,

Zhitao Li

Re: Review Request 56174: Added skipping already stored layers to local Docker puller.

[Ilya Pronin]

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

(Updated Feb. 3, 2017, 6:07 p.m.)


Review request for mesos, Jie Yu and Timothy Chen.


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


Repository: mesos


Description
---

If a layer is already in the store, there's no need to extract it. 
`RegistryPuller` already does this and `Store` is ready for such puller 
behaviour.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
ee391af898886bff9e5b911697f725c5ea53ebd8 
  src/tests/containerizer/provisioner_docker_tests.cpp 
af9987f88205d00d091f35fa734d5667506aaffd 

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


Testing (updated)
---

Added a test verifying that local puller will skip layes that are already in 
the store. Ran `make check`.


Thanks,

Ilya Pronin

Re: Review Request 56251: Tightened assertions in sorter and allocator.

[Neil Conway]

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

(Updated Feb. 3, 2017, 5:57 p.m.)


Review request for mesos and Michael Park.


Changes
---

Rebase.


Repository: mesos


Description
---

Also replaced a mistaken comment.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
5f540569043df9d2bb75416c8c36bb4dd7bd68a1 
  src/master/allocator/sorter/drf/sorter.cpp 
5681a5d78a7bdde820c3a8633d742d9d6412f1c7 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 55901: Added support for command health checks to the default executor.

[Gastón Kleiman]

> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/tests/health_check_tests.cpp, lines 2118-2119
> > 
> >
> > Why do we need to expilictly create `containerizer`?

Because the implicit `containerizer` is started in local mode. 
`LaunchNestedContainerSession` (used by cmd health checks) tries to start a IO 
switchboard, which doesn't work in local mode yet.


> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.hpp, line 207
> > 
> >
> > How about calling it `commandCheckViaAgent`?

Good idea, done.


> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, line 130
> > 
> >
> > s/createRequest/createV1AgentAPIRequest?
> > remove `inline`
> > 
> > This looks like a utility function not really related to the health 
> > checker library. Maybe put it into "src/slave/api_utils.{hpp|cpp}"?

I renamed the function. Vinod suggested to make it `inline` what's wrong with 
that?

This function as-is is specific to how we perform requests in this file 
(`keep_alive = false`, protobuf serialization). Since it is used only in two 
places, I'm tempted to removed it and put the code inline in the corresponding 
methods.


> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/tests/health_check_tests.cpp, line 2107
> > 
> >
> > Why not spelling out `cmd`?

Renamed.


> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/tests/health_check_tests.cpp, lines 2154-2155
> > 
> >
> > Formatting.
> > 
> > Also it is probably a good idea to explicitly say you don't care about 
> > further updates.

Fixed the formatting. We don't have such a comment in most of the other tests 
in this file.


- Gastón


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


On Feb. 3, 2017, 5:15 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 3, 2017, 5:15 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
>   src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

[Gastón Kleiman]

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

(Updated Feb. 3, 2017, 5:15 p.m.)


Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent huang, 
and Vinod Kone.


Changes
---

Addressed feedback.


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


Repository: mesos


Description
---

Added support for command health checks to the default executor.


Diffs (updated)
-

  src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
  src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
  src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
  src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb 

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


Testing
---

Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
passes on Linux, but not on macOS.


Thanks,

Gastón Kleiman

Re: Review Request 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56178]

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 Feb. 3, 2017, 12:10 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56178/
> ---
> 
> (Updated Feb. 3, 2017, 12:10 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7022
> https://issues.apache.org/jira/browse/MESOS-7022
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the local authorizer so that MULTI_ROLE frameworks can be
> authorized.
> 
> For non-MULTI_ROLE frameworks we continue to support use of the
> deprecated 'value' field in the authorization request's 'Object';
> however for MULTI_ROLE frameworks the 'value' field will not be set,
> and authorizers still relying on it should be updated to instead use
> the object's 'framework_info' field to extract roles to authorize
> against from.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> b98e1fcdf2ee5ec1f6ac0be6f8accdefaa390a09 
>   src/master/master.cpp 284566ca72bd5c6bd581db9b65d404f86aa7bf61 
> 
> Diff: https://reviews.apache.org/r/56178/diff/
> 
> 
> Testing
> ---
> 
> Tested on various configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

[Alexander Rukletsov]

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




src/checks/health_checker.hpp (line 94)


Could you please add a todo here saying this will go away and link a jira 
ticket?



src/checks/health_checker.hpp (line 207)


How about calling it `commandCheckViaAgent`?



src/checks/health_checker.cpp (line 130)


s/createRequest/createV1AgentAPIRequest?
remove `inline`

This looks like a utility function not really related to the health checker 
library. Maybe put it into "src/slave/api_utils.{hpp|cpp}"?



src/checks/health_checker.cpp (lines 146 - 170)


What is the value of using these helpers? I'd say having 
`connection.send(createRequest(url, call), false);` is clearer and self 
explanatory rather than `post(connection, url, call)`. And you don't need any 
comments then : )



src/checks/health_checker.cpp (line 224)


You can use `{}` instead.



src/checks/health_checker.cpp (lines 434 - 436)


Use `CHECK_SOME` instead.



src/checks/health_checker.cpp (lines 456 - 482)


Could you please link a JIRA here?



src/checks/health_checker.cpp (line 593)


Please keep the order of the definitions in sync with the order of 
declarations.



src/checks/health_checker.cpp (line 746)


Let's pull this (together with another below) into a separate patch.



src/tests/health_check_tests.cpp (line 2107)


Why not spelling out `cmd`?



src/tests/health_check_tests.cpp (lines 2118 - 2119)


Why do we need to expilictly create `containerizer`?



src/tests/health_check_tests.cpp (lines 2154 - 2155)


Formatting.

Also it is probably a good idea to explicitly say you don't care about 
further updates.



src/tests/health_check_tests.cpp (lines 2157 - 2161)


1. I think you can also check that task's env vars are available in the 
health check -> pass exit status in the task's command.
2. Let's avoid using obscure `populateTasks()` here. We'll get rid of it 
altogether in the nearest future.



src/tests/health_check_tests.cpp (lines 2185 - 2186)


check that `healthy` is set please.


- Alexander Rukletsov


On Feb. 2, 2017, 5:02 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 2, 2017, 5:02 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
>   src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.

[Benjamin Bannier]

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

(Updated Feb. 3, 2017, 1:10 p.m.)


Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.


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


Repository: mesos


Description
---

This updates the local authorizer so that MULTI_ROLE frameworks can be
authorized.

For non-MULTI_ROLE frameworks we continue to support use of the
deprecated 'value' field in the authorization request's 'Object';
however for MULTI_ROLE frameworks the 'value' field will not be set,
and authorizers still relying on it should be updated to instead use
the object's 'framework_info' field to extract roles to authorize
against from.


Diffs (updated)
-

  src/authorizer/local/authorizer.cpp b98e1fcdf2ee5ec1f6ac0be6f8accdefaa390a09 
  src/master/master.cpp 284566ca72bd5c6bd581db9b65d404f86aa7bf61 

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


Testing
---

Tested on various configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.

[Benjamin Bannier]

> On Feb. 3, 2017, 11:26 a.m., Adam B wrote:
> > src/authorizer/local/authorizer.cpp, line 241
> > 
> >
> > Does this return a list with a single role, if the framework fills in 
> > `framework_info.role` instead of `framework_info.roles`?
> > 
> > Is there a reason you can't just use `framework_info.roles()` here 
> > instead of using the protobuf util?

Yes, this helper returns single element lists for  single-role frameworks.

The helper is used here since it frees us from introducing extra logic here to 
either examine `role` or `roles`, depending on the framework's capabilities.


> On Feb. 3, 2017, 11:26 a.m., Adam B wrote:
> > src/authorizer/local/authorizer.cpp, line 244
> > 
> >
> > Seems like framework_info is always set, so how/why would we ever fall 
> > through to the other cases?

Yes, this is currently dead code. I was also wondering whether it should be 
removed, but decided against it since it provides some level of redundancy as 
long as `value` still exists and code in the master and in authorizers might 
not evolve consistently.

Do you believe it should be removed?


- Benjamin


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


On Feb. 3, 2017, 1:10 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56178/
> ---
> 
> (Updated Feb. 3, 2017, 1:10 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7022
> https://issues.apache.org/jira/browse/MESOS-7022
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the local authorizer so that MULTI_ROLE frameworks can be
> authorized.
> 
> For non-MULTI_ROLE frameworks we continue to support use of the
> deprecated 'value' field in the authorization request's 'Object';
> however for MULTI_ROLE frameworks the 'value' field will not be set,
> and authorizers still relying on it should be updated to instead use
> the object's 'framework_info' field to extract roles to authorize
> against from.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> b98e1fcdf2ee5ec1f6ac0be6f8accdefaa390a09 
>   src/master/master.cpp 284566ca72bd5c6bd581db9b65d404f86aa7bf61 
> 
> Diff: https://reviews.apache.org/r/56178/diff/
> 
> 
> Testing
> ---
> 
> Tested on various configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 56250: Cleaned up headers in DRF sorter.

[Gastón Kleiman]

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


Fix it, then Ship it!





src/master/allocator/sorter/drf/sorter.cpp (line 32)


According to recent discussions and to our code style guide, this should be 
the first include in the file.


- Gastón Kleiman


On Feb. 2, 2017, 10:59 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56250/
> ---
> 
> (Updated Feb. 2, 2017, 10:59 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Cleaned up headers in DRF sorter.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 5681a5d78a7bdde820c3a8633d742d9d6412f1c7 
> 
> Diff: https://reviews.apache.org/r/56250/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

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

[Benjamin Bannier]

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




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


Could you follow up with a patch so this call is `dispatch`'ed?

Right now, if the `HierarchicalAllocatorProcess` goes away after 
`allocate`, but before the `AnyCallback` finishes, `this->allocationInterval` 
might be referencing garbage.


- Benjamin Bannier


On Jan. 31, 2017, 2:46 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Jan. 31, 2017, 2:46 a.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 
> 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 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.

[Adam B]

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




src/authorizer/local/authorizer.cpp (line 240)


Does this return a list with a single role, if the framework fills in 
`framework_info.role` instead of `framework_info.roles`?

Is there a reason you can't just use `framework_info.roles()` here instead 
of using the protobuf util?



src/authorizer/local/authorizer.cpp (line 243)


Seems like framework_info is always set, so how/why would we ever fall 
through to the other cases?



src/master/master.cpp (line 2175)


s/use/used/


- Adam B


On Feb. 1, 2017, 9:06 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56178/
> ---
> 
> (Updated Feb. 1, 2017, 9:06 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7022
> https://issues.apache.org/jira/browse/MESOS-7022
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the local authorizer so that MULTI_ROLE frameworks can be
> authorized.
> 
> For non-MULTI_ROLE frameworks we continue to support use of the
> deprecated 'value' field in the authorization request's 'Object';
> however for MULTI_ROLE frameworks the 'value' field will not be set,
> and authorizers still relying on it should be updated to instead use
> the object's 'framework_info' field to extract roles to authorize
> against from.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> b98e1fcdf2ee5ec1f6ac0be6f8accdefaa390a09 
>   src/master/master.cpp 284566ca72bd5c6bd581db9b65d404f86aa7bf61 
> 
> Diff: https://reviews.apache.org/r/56178/diff/
> 
> 
> Testing
> ---
> 
> Tested on various configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 56052: Added the 'Secret' protobuf message.

[Adam B]

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




include/mesos/mesos.proto (lines 1967 - 1968)


What are these two fields used for? Can you give an example to demonstrate 
the difference between name and key?


- Adam B


On Feb. 2, 2017, 11:39 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56052/
> ---
> 
> (Updated Feb. 2, 2017, 11:39 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Kapil Arya, Jan Schlicht, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-6996
> https://issues.apache.org/jira/browse/MESOS-6996
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a new `Secret` protobuf message which
> is designed to serve as a generic mechanism for passing
> priviledged information within Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto a08b8fb2edf26ae730467557ae0e33e3d4252593 
>   include/mesos/v1/mesos.proto 4a4609e7db659252ef7f1ca61f8ba079b901b7d0 
> 
> Diff: https://reviews.apache.org/r/56052/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>