Re: Review Request 42476: Introduced protobuf for set quota requests.

[Bernd Mathiske]

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

Ship it!


After addressing Joerg's remaining issues, ship it!

- Bernd Mathiske


On Jan. 20, 2016, 4:12 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42476/
> ---
> 
> (Updated Jan. 20, 2016, 4:12 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ben Mahler, Joerg Schad, and Joris 
> Van Remoortere.
> 
> 
> Bugs: MESOS-4410
> https://issues.apache.org/jira/browse/MESOS-4410
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6 
>   include/mesos/quota/quota.proto 338412ee967e14aa1957a47f4a50f2e19e4eca79 
>   src/master/quota_handler.cpp f44736cd5849d4fb22a75c1238d433a1c0c9708d 
>   src/tests/master_quota_tests.cpp e8cb074c2913cafdc6b1792896f29e53f1210c9d 
>   src/tests/role_tests.cpp 979391306e2427aaa63a5df32704913f79e20e36 
> 
> Diff: https://reviews.apache.org/r/42476/diff/
> 
> 
> Testing
> ---
> 
> make check on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 42594: Introduced framework registry operations.

[Yongqiao Wang]

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

Review request for mesos, Ben Mahler and Vinod Kone.


Repository: mesos


Description
---

Introduced framework registry operations.


Diffs
-

  src/master/master.hpp 3a7e18232323a1c051bcc97915484b1195fffe58 

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


Testing
---

Make && Make check successfully!


Thanks,

Yongqiao Wang

Re: Review Request 41932: Updated "teardown_framework" requests in the authorizer.

[Adam B]

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

Ship it!


Ship it! I can make this minor change when I commit, so don't worry about it.


src/tests/teardown_tests.cpp (line 188)


s/good ACLs/deprecated (but still good) ACLs,/


- Adam B


On Jan. 21, 2016, 12:52 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41932/
> ---
> 
> (Updated Jan. 21, 2016, 12:52 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-4154
> https://issues.apache.org/jira/browse/MESOS-4154
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated "teardown_framework" requests in the authorizer.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> c1db9c2131ea8fbf835278203a240f108c6372c5 
>   src/tests/teardown_tests.cpp d979e07cacd1438e3d44adbe84229020f9159811 
> 
> Diff: https://reviews.apache.org/r/41932/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 42594: Introduced framework registry operations.

[Yongqiao Wang]

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

(Updated Jan. 21, 2016, 9:45 a.m.)


Review request for mesos, Ben Mahler and Vinod Kone.


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


Repository: mesos


Description
---

Introduced framework registry operations.


Diffs
-

  src/master/master.hpp 3a7e18232323a1c051bcc97915484b1195fffe58 

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


Testing
---

Make && Make check successfully!


Thanks,

Yongqiao Wang

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

[Guangya Liu]

> On 一月 19, 2016, 11:34 p.m., Joseph Wu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 676
> > 
> >
> > I see several places where you overwrite changes from this review in 
> > the subsequent two.  Did you mean to have three reviews for the changes to 
> > `::updateAllocation`, `::updateAvailable`, and `::recoverResources`?
> 
> Guangya Liu wrote:
> There are three patches handling dynamic reservation and 
> 1) https://reviews.apache.org/r/41791 Reserve new resources via 
> `updateAllocation` and `updateAvailable`. (1/3).
> 2) https://reviews.apache.org/r/42113 Unreserve resources via 
> `updateAllocation` (2/3).
> 3) https://reviews.apache.org/r/42194 Unreserve resources via 
> `updateAvailable` (3/3).
> 
> I have updated the description here.
> 
> Joseph Wu wrote:
> It doesn't seem like you need 3 patches.
> 
> I'd recommend a couple of cleanups:
> 
> * Add a helper for all the duplicate logic you have.  Both 
> `updateAllocation` and `updateAvailable` deal with reservations, in 
> essentially the same way (per optimistic offers).
> * Consolidate the overlapping changes into a single patch.  Just don't 
> overwrite your own changes.
> * Consider a different approach than subtracting allocation slack on 
> unreserve.  You could perhaps, for accounting purposes, elevate a portion of 
> the allocation from revocable to non-revocable.

The reason I do not add a helper function because even though the code are 
almost same, there are indeed difference between logs, `updateAllocation` will 
have some logs related to `framework` and `agent`, `updateAvailable` will only 
have some logs related to `agent`. I prefer that we keep the changes separtely 
to make log message clear for different cases, comments? Thanks.


- Guangya


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


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

Re: Review Request 41979: Updated document for teardown_frameworks.

[Adam B]

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

Ship it!


I'll also update the upgrades guide and CHANGELOG.

- Adam B


On Jan. 21, 2016, 1:04 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41979/
> ---
> 
> (Updated Jan. 21, 2016, 1:04 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-4154
> https://issues.apache.org/jira/browse/MESOS-4154
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated document for teardown_frameworks.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md 016ab65d60933cfe1dcd41e8afd6537abcd75171 
>   docs/configuration.md 1834992907a29f571200fe8422f7c26eaac7d5a0 
>   src/master/flags.cpp 66c26be102229e24ae99f49292734db3cb83f17a 
> 
> Diff: https://reviews.apache.org/r/41979/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 41932: Updated "teardown_framework" requests in the authorizer.

[Adam B]

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


Looks great. Create a separate test for shutdown vs. teardown ACLs, and then we 
can Ship It!


src/tests/teardown_tests.cpp (lines 150 - 157)


In this case, you should be testing both ACLs separately, rather than one 
test for a single ACLs message that includes the same policy in both message 
formats.
So how about a TeardownEndpointGoodACLs and a 
TeardownEndpointGoodDeprecatedACLs?


- Adam B


On Jan. 13, 2016, 11:53 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41932/
> ---
> 
> (Updated Jan. 13, 2016, 11:53 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-4154
> https://issues.apache.org/jira/browse/MESOS-4154
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated "teardown_framework" requests in the authorizer.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> c1db9c2131ea8fbf835278203a240f108c6372c5 
>   src/tests/teardown_tests.cpp 97cc89ba168aefff8512f6d1a25c4f7ddf180bae 
> 
> Diff: https://reviews.apache.org/r/41932/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 42590: Removed reserved() API.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [42590]

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

- Mesos ReviewBot


On Jan. 21, 2016, 6:34 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42590/
> ---
> 
> (Updated Jan. 21, 2016, 6:34 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, Joseph Wu, and Klaus Ma.
> 
> 
> Bugs: ESOS-4447
> https://issues.apache.org/jira/browse/ESOS-4447
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed reserved() API.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa 
>   include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c 
>   src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
>   src/master/http.cpp 12c1fe5a514903f657911302e8770e9b245fdbb7 
>   src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666 
>   src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 
> 
> Diff: https://reviews.apache.org/r/42590/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 41979: Updated document for teardown_frameworks.

[Adam B]

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

Ship it!


Looks great!


docs/authorization.md (line 45)


s/can be teardown by/can be torn down by/


- Adam B


On Jan. 14, 2016, 12:06 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41979/
> ---
> 
> (Updated Jan. 14, 2016, 12:06 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-4154
> https://issues.apache.org/jira/browse/MESOS-4154
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated document for teardown_frameworks.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md a928f1722dc67cd791d78ebbe4591f2e8f2e8f2a 
>   docs/configuration.md 7e0eb9584f3cb24a388c44d7bc81cd87dbb4072e 
>   src/master/flags.cpp 88909590ff421421659e6faac7f3444bdc57b630 
> 
> Diff: https://reviews.apache.org/r/41979/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

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

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [42591]

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

- Mesos ReviewBot


On Jan. 21, 2016, 6:44 a.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42591/
> ---
> 
> (Updated Jan. 21, 2016, 6:44 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-1719
> https://issues.apache.org/jira/browse/MESOS-1719
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add the protobufs in registry to persist some framework informations which do 
> not allow to change when framework re-register.
> 
> 
> Diffs
> -
> 
>   src/master/registry.proto 9958f9c2bdb785390fca2f292b65d5a9310434d5 
> 
> Diff: https://reviews.apache.org/r/42591/diff/
> 
> 
> Testing
> ---
> 
> Make && Make check successfully
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 41932: Updated "teardown_framework" requests in the authorizer.

[Guangya Liu]

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

(Updated 一月 21, 2016, 8:52 a.m.)


Review request for mesos and Adam B.


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


Repository: mesos


Description
---

Updated "teardown_framework" requests in the authorizer.


Diffs (updated)
-

  src/authorizer/local/authorizer.cpp c1db9c2131ea8fbf835278203a240f108c6372c5 
  src/tests/teardown_tests.cpp d979e07cacd1438e3d44adbe84229020f9159811 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 41979: Updated document for teardown_frameworks.

[Guangya Liu]

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

(Updated 一月 21, 2016, 9:04 a.m.)


Review request for mesos and Adam B.


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


Repository: mesos


Description
---

Updated document for teardown_frameworks.


Diffs (updated)
-

  docs/authorization.md 016ab65d60933cfe1dcd41e8afd6537abcd75171 
  docs/configuration.md 1834992907a29f571200fe8422f7c26eaac7d5a0 
  src/master/flags.cpp 66c26be102229e24ae99f49292734db3cb83f17a 

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


Testing
---

make
make check


Thanks,

Guangya Liu

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

[Yongqiao Wang]

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

(Updated Jan. 21, 2016, 9:57 a.m.)


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


Changes
---

Update the dependency to address the build error.


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


Repository: mesos


Description
---

Add tests for /weights endpoint.


Diffs
-

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

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


Testing
---

Make and Make check successfully!

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

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


Thanks,

Yongqiao Wang

Re: Review Request 41789: Expose the http::internal::request function.

[Yongqiao Wang]

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

(Updated Jan. 21, 2016, 9:56 a.m.)


Review request for mesos, Adam B, Joerg Schad, Joris Van Remoortere, Neil 
Conway, and Qian Zhang.


Changes
---

Add dependency to address the build error for #41790


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


Repository: mesos


Description
---

Expose the http::internal::request function.


Diffs
-

  3rdparty/libprocess/include/process/http.hpp 
1d9a944f7b0c559823c8c889d20e942c7d6a7221 
  3rdparty/libprocess/src/http.cpp 762da9a9038fc0a81156b5c03b556084df1bd7e0 
  3rdparty/libprocess/src/tests/http_tests.cpp 
c23d0bf1929686cfc42969f39ce046f4794539d4 

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


Testing
---

Make & Make check successfully.


Thanks,

Yongqiao Wang

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

[Qian Zhang]

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



src/master/allocator/mesos/hierarchical.cpp (lines 1342 - 1344)


So here we offer `offerableResources` to framework and mark `resources` as 
allocated resource of the slave. But for a framework in a quota'ed role, 
`offerableResources` and `resources` may be different, the former is revocable 
and the later is non-revocable, so later when `recoverResources()` is called 
(e.g., the offer is declined by framework), we will subtract offered resources 
from allocated resources (`slaves[slaveId].allocated -= resources;`), I think 
the subtraction will not work as we expect since the offered resources are 
revocable but allocated resources are non-revocable.


- Qian Zhang


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

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

[Alexander Rukletsov]

> On Jan. 21, 2016, 7:17 a.m., Ben Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 449-451
> > 
> >
> > Why mention expiration between two consecutive allocations here? The 
> > way I had been thinking about a test here was that we ensure that a filter 
> > always expires after the next batch allocation. 
> > 
> > So, is the fact that there are two batch allocations here is just an 
> > implementation detail as far as testing is concerned?
> > 
> > Would you mind referencing MESOS-4302 so that the next person to look 
> > at this test can follow the history a little more easily?

I always try to show the reader why we actually cared and wrote the test. I 
believe a breif description of the problem helps the reader to better 
understand motivation behind the chosen behaviour.

However I agree that such background information should not substitute the 
exact description. I'll try to rephrase.


- Alexander


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


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

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

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Jan. 21, 2016, 7:35 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42470/
> ---
> 
> (Updated Jan. 21, 2016, 7:35 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Michael Park.
> 
> 
> Bugs: MESOS-4378
> https://issues.apache.org/jira/browse/MESOS-4378
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Multiple Disk: Added 'Source' to 'Resource.DiskInfo'.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 0be4bed336e86a5c377e87ac6212c70ac3b4c66b 
>   include/mesos/v1/mesos.proto c3244e87f9351c71312d2eace7a49bcac926fafd 
> 
> Diff: https://reviews.apache.org/r/42470/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 41092: CMake: Added CMake file for agent executable build.

[Diana Arroyo]

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

(Updated Jan. 21, 2016, 4:58 p.m.)


Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
Remoortere, and Joseph Wu.


Changes
---

Addressed latest review comments.


Summary (updated)
-

CMake: Added CMake file for agent executable build.


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


Repository: mesos


Description
---

Added CMake file for agent executable build.


Diffs (updated)
-

  src/slave/CMakeLists.txt PRE-CREATION 
  src/slave/cmake/SlaveConfigure.cmake fbdfdaa27fbd8c7429861eea5baf401a221f748b 

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


Testing
---

Tested on Ubuntu and OSX.


Thanks,

Diana Arroyo

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

[Alexander Rukletsov]

> On Jan. 21, 2016, 7:17 a.m., Ben Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 544-560
> > 
> >
> > In the bottom section of this test, I'm not sure folks without our 
> > context will understand what is being done and what is expected to occur.
> > 
> > Maybe we just write something as simple as the following, for example:
> > 
> > The allocator will ensure that offer filters are removed after at least 
> > one batch allocation has occurred. Therefore, we expect that after the 
> > timeout elapses, the filter remains active for the next allocation and the 
> > resources are sent to framework1.
> > 
> > Then, I'm curious why you don't have a subsequent allocation to verify 
> > that the offer filter was really removed, is there a test for that?

Good suggestions. I'll reword comments and add one more allocation.


> On Jan. 21, 2016, 7:17 a.m., Ben Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 546
> > 
> >
> > Can you use offerFilter.refuse_seconds() here? Or create a 'timeout' 
> > variable which is used to both set the offerFitlter.refuse_seconds and is 
> > passed to this advance() call?

Yep, makes sense. Thanks!


- Alexander


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


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

Review Request 42605: Fixed a NULL pointer dereference bug in Slave.

[Anand Mazumdar]

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

Review request for mesos and Joris Van Remoortere.


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


Repository: mesos


Description
---

This fixes a `NULL` pointer dereference erroneously added as part of 
introducing an output operator for the `Executor` struct in 
https://reviews.apache.org/r/39569.


Diffs
-

  src/slave/slave.cpp e23c3295c8ebed580751a5aabaf26e1773c54859 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 42590: Removed reserved() API.

[Joseph Wu]

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



include/mesos/v1/resources.hpp (line 224)


Instead of changing this to an Option, you should default to `"*"`.

Otherwise, you end up with two arguments (`None()` and `"*"`) that give you 
the same result.



src/master/http.cpp (line 478)


Since this is present for backwards compatibility, just add a `NOTE:` here.


- Joseph Wu


On Jan. 21, 2016, 7:23 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42590/
> ---
> 
> (Updated Jan. 21, 2016, 7:23 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, Joseph Wu, and Klaus Ma.
> 
> 
> Bugs: MESOS-4447
> https://issues.apache.org/jira/browse/MESOS-4447
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed reserved() API.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa 
>   include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c 
>   src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
>   src/master/http.cpp 12c1fe5a514903f657911302e8770e9b245fdbb7 
>   src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666 
>   src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 
> 
> Diff: https://reviews.apache.org/r/42590/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

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

[Jie Yu]

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



src/common/resources.cpp (lines 102 - 108)


Can you do the following to be more consistent with others
```
if (left.has_path() && left.path() != right.path()) {
  return false;
}
if (left.has_mount() && left.mount() != right.mount()) {
  return false;
}
```



src/common/resources.cpp (lines 251 - 255)


I'd prefer removing this CHECK here since it's on the critical path of 
resource calculation.



src/common/resources.cpp (lines 309 - 328)


Ditto on merge them into a single if clause.



src/common/resources.cpp (lines 315 - 317)


Ditto on removing this CHECK.



src/common/resources.cpp (lines 319 - 322)


This does not look correct. Two identical MOUNT disk resource should be 
subtractable.



src/v1/resources.cpp (lines 102 - 104)


Can you do the following to be consistent with others:
```
if (left.has_path() && left.path() != right.path()) {
  return false;
}

if (left.has_mount() && left.mount() != right.mount()) {
  return false;
}

return true;
```



src/v1/resources.cpp (lines 247 - 268)


We should merge those check under the same `has_disk` if clause:

```
if (left.has_disk()) {
  // Check if disk() are equal
  // MOUNT check
  // persistence check
}
```



src/v1/resources.cpp (lines 253 - 255)


Ditto on removing it.



src/v1/resources.cpp (lines 315 - 317)


Ditto on removing it.



src/v1/resources.cpp (lines 319 - 322)


Ditto. Please adjust the logics here according to my comments above.


- Jie Yu


On Jan. 21, 2016, 7:36 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42471/
> ---
> 
> (Updated Jan. 21, 2016, 7:36 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Michael Park.
> 
> 
> Bugs: MESOS-4380
> https://issues.apache.org/jira/browse/MESOS-4380
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a 
>   include/mesos/v1/mesos.hpp 961042d8e4944a475076b829966020d62175d726 
>   src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
>   src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 
> 
> Diff: https://reviews.apache.org/r/42471/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

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

[Joseph Wu]

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

Not quite:
```
slaves[slaveId].total = slaves[slaveId].total.nonUsageSlack() + oversubscribed;

// Only add this if you really want to check the flag.  
// But optimistic offers shouldn't even be considered in this method.
if (enableReservationOversubscription) {
  CHECK_EQ(Resources(), slaves[slaveId].total.allocationSlack());
}
```


- Joseph


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


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

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

[Vinod Kone]

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



src/executor/executor.cpp (line 170)


should be a CHECK as this is not expected.



src/executor/executor.cpp (lines 198 - 200)


This should be a CHECK at #190, because this is not expected.



src/executor/executor.cpp (line 214)


Ditto. This should be a CHECK instead.



src/executor/executor.cpp (lines 219 - 221)


why not set these in the initializer list?



src/executor/executor.cpp (lines 238 - 259)


I think it would be a bit clearer to follow if you do something like this

```
if (call.type() == Call::SUBSCRIBE) {
  // If we are in `CONNECTED` state, subscribe connection should exist.
  CHECK_SOME(subscribe);
  
  _send(call);
} else {
  if (nonSubscribe.isSome()) {
// If nonSubscribe connection already exists, use it. 
_send(call);
  } else {
// Create a new nonSubscribe connection.
...
  }
}

```



src/executor/executor.cpp (line 239)


what happens if this future is failed or discarded?



src/executor/executor.cpp (line 241)


Don't we have to check here that nonSubscribe is already set? For example, 
2 back to back non-subscribe calls might result in 2 non-subscribe connections?



src/executor/executor.cpp (line 247)


log the future's error if it is in error state.



src/executor/executor.cpp (line 269)


Lets comment here that the library is considered connected if we can 
establish the subscribe connection. Worth mentioning here that non-subscribe 
calls use a different `nonSubscribe` connection and that its disconnection 
doesn't change the `state`.



src/executor/executor.cpp (line 274)


what happens if this future is discarded?



src/executor/executor.cpp (line 300)


can we convert `_connected` into a lambda?



src/executor/executor.cpp (line 309)


How is this possible? Can you expand the comment?



src/executor/executor.cpp (lines 313 - 314)


Move this comment down to #327.



src/executor/executor.cpp (line 320)


does this need to be a function?



src/executor/executor.cpp (line 323)


can we convert `_disconnected` to a lambda?



src/executor/executor.cpp (lines 376 - 378)


Can there be more than one oustanding connection attempts at any given time?



src/executor/executor.cpp (line 596)


no need for stringify?



src/executor/executor.cpp (line 601)


can you add a comment on why you do `false` here?



src/executor/executor.cpp (line 624)


Add a comment here (or above where I commented) that the state tracks the 
`subscribe` connection. Also, do we really need this enum? Can't we just use 
`subscribe` to figure if we are connected or not?


- Vinod Kone


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

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

[Jie Yu]

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



src/slave/slave.cpp (lines 2260 - 2272)


I'm still not sure if we should create sub-directories in MOUNT disk or 
not. The reason is because we won't be able to create sub-directories in 
BLOCK/RAW disk, do we want to introduce a different behavior here?

I'd prefer to be consistent between BLOCK and MOUNT type.


- Jie Yu


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

Review Request 42615: mesos: Cleaned up usage of namespace-qualified identifiers.

[Neil Conway]

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

Review request for mesos.


Repository: mesos


Description
---

For example, avoid using `std::string` in a .cpp file that already does
`using std::string`.


Diffs
-

  src/authentication/http/basic_authenticator_factory.cpp 
6eb1c5bd09b136d3bc20481ddcc65cb8bd153682 
  src/cli/execute.cpp 0add77558e07ff49f0134ddb9085509501c61aab 
  src/common/http.cpp b7e71eb79ccb8ef1d0bbd168d77d4d4591ecb09b 
  src/common/protobuf_utils.cpp 53324ab569751924f25290641d1a70da790c2104 
  src/docker/docker.cpp 4d2f1fa14f4450b8fa3401081bf52807d2e79a7e 
  src/examples/balloon_framework.cpp 3c17676c92f72d41c501b2435a7017e3c9f01278 
  src/examples/test_container_logger_module.cpp 
6b1f4dbd63145afe4de17830c0a2a6202a896be7 
  src/examples/test_http_authenticator_module.cpp 
acf51a6deb8e7dc4ab6ac0cf70380ddbb1839906 
  src/examples/test_qos_controller_module.cpp 
f8aa1d730f6cdf5ab3f11789e9699682cc08ddd5 
  src/files/files.cpp dd64976ca454786152a8e29f590e8c1ed1df3b54 
  src/health-check/main.cpp 0beaed575ec865d81e6e3d83d8a0c894613acba4 
  src/hook/manager.cpp 6ee93038dce1247cffbc82c736a1c7a1ecb84bb0 
  src/launcher/fetcher.cpp 902e927ce0cbcf70e41041375e61987752629957 
  src/linux/routing/queueing/fq_codel.cpp 
d840e383f28481ba58d8269217a8a194c4e87db5 
  src/linux/routing/queueing/ingress.cpp 
c25a93966caca5c5a9eef91e6caa5d9e29091bee 
  src/linux/systemd.cpp d3f4a63d833a5bc599494e925d709ba0cc70d9ec 
  src/local/local.cpp 582d4a10444831b0753d20650698e5d3b51cca9f 
  src/log/tool/initialize.cpp c9706d1e33659f7b0e375419197b207cb96d4ca9 
  src/log/tool/read.cpp 9abf5a82338c554920283e0329cd83e4c787c103 
  src/log/tool/replica.cpp 8baf79755da420cf594d706f31aa561b5d665051 
  src/master/allocator/sorter/drf/sorter.cpp 
3a442f121f3a1505513877a5c78458a4b8d0a824 
  src/master/main.cpp 0d5ac49121491152d8426cd764a1f1a0f37483ae 
  src/master/maintenance.cpp df7cd6ca5a097611e5ca5a46cc988ef636da096b 
  src/master/master.cpp 9ee56277c8a472be9e683d5db505becfb5f7c422 
  src/module/manager.cpp a53f71b9965f7ab85aadb6c0c7af18de958faf38 
  src/slave/containerizer/docker.cpp 40f6f0bc08d7d75ec9dc6494914793e3649820d0 
  src/slave/containerizer/fetcher.cpp 4ac9149980cf7f013b318218a1b29369c5dcff17 
  src/slave/containerizer/mesos/linux_launcher.cpp 
61801fffe9bac1a995a57d8b4e8c004d624bbd63 
  src/slave/containerizer/mesos/provisioner/docker/paths.cpp 
9c5c24992f9ea36159271691cca0147851240088 
  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
b74c760e90b12f5bf45b6e626e2b661f841e5a8d 
  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
03425daaf015cf231920b7eda1d5424895a41286 
  src/slave/qos_controllers/load.cpp 52520d6220784423c09001b8bd59e090ebf787ec 
  src/slave/state.cpp fae7738a1cb7168abc0cfe35b075bbc73306b820 
  src/tests/attributes_tests.cpp 3f3dde1301566f279c3fc7dc24e3ef67039a4c14 
  src/tests/common/http_tests.cpp 0ea06341b092cd6ad278075b12dd970b84c84464 
  src/tests/containerizer/cgroups_isolator_tests.cpp 
0d24809701c42dc036bbd051791545f4742d1e9a 
  src/tests/containerizer/external_containerizer_test.cpp 
da52860f4a1d5363d7b61b9a8bb6abad02d89736 
  src/tests/containerizer/provisioner_docker_tests.cpp 
33df60065903228749833bbad20449ba8784594a 
  src/tests/credentials_tests.cpp 6e3725c61a55a2c1d183ee278cf3d72be16a6e40 
  src/tests/fetcher_cache_tests.cpp 2747b72ba49c9fde04e556b649601b037517283e 
  src/tests/gc_tests.cpp ef5544b627ac4a9f9c13fe13b0c51b21d5b98a12 
  src/tests/hierarchical_allocator_tests.cpp 
953712149bd951789beb29c72779c4ac65aa48dc 
  src/tests/hook_tests.cpp 152984b01069acd4cf195bfce58835f0304a97f2 
  src/tests/http_authentication_tests.cpp 
bd622576973648e0dfeae1453a5ce631e4171352 
  src/tests/master_quota_tests.cpp 7d07be355b9323f2a7f27f84020a6ecc7f061140 
  src/tests/mesos.cpp 365ebe8335c37bfdb983a5424d4c995fa9b76a22 
  src/tests/monitor_tests.cpp 2226458b59b4a279a92e1353bd61457a0018d2a9 
  src/tests/registrar_tests.cpp 58b3fccf5450d20032c5c6d20d76dbd868c424a3 
  src/tests/registrar_zookeeper_tests.cpp 
3861dcf8341bfa520f94fd244b4814827299b861 
  src/tests/scheduler_driver_tests.cpp f35c4957b08803dba924ebf1a5f0af9daac9d0c5 
  src/tests/slave_tests.cpp e943ab99baf3d74679a5da89a89f6a4b7ead 
  src/tests/values_tests.cpp a4eb68ad13407f471a07a9a923ed31c7890da9f7 
  src/watcher/whitelist_watcher.cpp 14d7de751884d4734942e315e61a94c29868ff4b 
  src/zookeeper/detector.cpp a3d68c12e3800805a35f9bb05e7689830eedbae6 

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


Testing
---

"make check" on OSX and Arch Linux.


Thanks,

Neil Conway

Review Request 42623: Reduced severity level for 'HTTP GET for ...' messages.

[Kapil Arya]

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

Review request for mesos, Cody Maloney and Joris Van Remoortere.


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


Repository: mesos


Description
---

Changed it from `LOG(INFO)` to `VLOG(2)`.


Diffs
-

  src/master/http.cpp 34a70ee50553492fc8c3947497ab5922f4379d72 
  src/slave/http.cpp 0ba2e82fd8ce58508f364aff761d50dc4f264f65 

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


Testing
---

Verified that the log messages don't appear in the logs.


Thanks,

Kapil Arya

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

[Greg Mann]

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

(Updated Jan. 21, 2016, 11:21 p.m.)


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


Changes
---

Addressed comment.


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


Repository: mesos


Description
---

Added dynamic reservation test with no authentication.

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


Diffs (updated)
-

  src/tests/reservation_tests.cpp 2ea3b6e48e75f438b325211fb562db19bd3a82e0 

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


Testing
---

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


Thanks,

Greg Mann

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

[Michael Park]

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



src/tests/reservation_endpoints_tests.cpp (lines 1159 - 1160)


Can we leave the `TODO` here saying this should work and should be fixed in 
`0.28`? There are like 5 or 6 cases in this test, so the note at the top 
doesn't tell us which ones need to be updated.



src/tests/reservation_endpoints_tests.cpp (line 1161)


Can we name these as `dynamicallyReservedWithNoPrincipal`, and for the rest 
as well? e.g. `dynamicallyReservedWithDefaultPrincipal`.



src/tests/reservation_endpoints_tests.cpp (lines 1174 - 1176)


(1) `HTTP authorization`? -- Isn't it authentication?

(2) `and dynamic reservations are currently unallowed without a principal`? 
-- What does this mean? We passed a `ReservationInfo` with a `principal` set 
here, and generally, passing `None()` as the authentication header isn't 
disallowed.



src/tests/reservation_endpoints_tests.cpp (line 1192)


Can we call this: `dynamicallyReservedWithNonMatchingPrincipal`? "New 
principal" makes it sound as if we're in the context of trying to update an 
existing principal to a new principal or something.


- Michael Park


On Jan. 20, 2016, 6:59 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42368/
> ---
> 
> (Updated Jan. 20, 2016, 6:59 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Michael Park, and Vinod Kone.
> 
> 
> Bugs: MESOS-4195
> https://issues.apache.org/jira/browse/MESOS-4195
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added reservation endpoint test without authentication.
> 
> Currently, dynamic reservation endpoints will not work when HTTP 
> authentication is not set. Currently, the authentication code will always 
> return `None()` as the principal when authentication is disabled. 
> Furthermore, the `ReservationInfo.principal` field is being migrated to 
> `optional`, so dynamic reservations without a principal are invalidated by 
> the master. This test checks that these endpoints will fail as expected when 
> HTTP authentication is disabled.
> 
> 
> Diffs
> -
> 
>   src/tests/reservation_endpoints_tests.cpp 
> b8edd6fafedd4c2221a8d19c1ebc71254071a8c7 
> 
> Diff: https://reviews.apache.org/r/42368/diff/
> 
> 
> Testing
> ---
> 
> A new test, `ReservationEndpointsTest.ReserveAndUnreserveNoAuthentication`, 
> was added, and make check was used to test. The new test was also run with 
> `--gtest_repeat=1000 --gtest_break_on_failure=1`.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 42617: stout: Cleaned up usage of namespace-qualified identifiers.

[Neil Conway]

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

Review request for mesos.


Repository: mesos


Description
---

For example, avoid using `std::string` in a .cpp file that already does
`using std::string`.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
5a10f59f3698a6ad85b7c2386def9686af8a23a6 
  3rdparty/libprocess/3rdparty/stout/tests/linkedhashmap_tests.cpp 
21a98b85277d946b887cb23c4ebb4d32cfcb0c6b 
  3rdparty/libprocess/3rdparty/stout/tests/os/filesystem_tests.cpp 
c5ffd017f83149a58a2dee8c87461cd06bb43bed 
  3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
dc8cf74f1bc90f6234a00d8cbca9c93a350cffa0 
  3rdparty/libprocess/3rdparty/stout/tests/some_tests.cpp 
c0be974ea2dc74c63ec3561403b3f4d3722a8df3 
  3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 
7715fa4baa150f370bdd0fd5c2f98dc4c6f5fc55 

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


Testing
---

"make check" on OSX and Arch Linux.


Thanks,

Neil Conway

Re: Review Request 42362: Added persistent volume endpoint test without authentication.

[Greg Mann]

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

(Updated Jan. 21, 2016, 9:03 p.m.)


Review request for mesos, Michael Park and Neil Conway.


Changes
---

Added another test with authorization enabled.


Summary (updated)
-

Added persistent volume endpoint test without authentication.


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


Repository: mesos


Description (updated)
---

Added persistent volume endpoint tests with HTTP authentication disabled, both 
with and without authorization enabled.

The persistent volume endpoint tests allow volume creation and destruction 
without authorization and without a principal; this patch introduces test cases 
for this scenario: 
`PersistentVolumeEndpointsTest.NoAuthenticationNoAuthorization` and 
`PersistentVolumeEndpointsTest.NoAuthenticationWithAuthorization`. NOTE: due to 
the current behavior of the HTTP authentication code, when HTTP authentication 
is disabled, the endpoint callbacks always receive a principal of `None()`, 
which means that authorization cannot be properly used when HTTP authentication 
is disabled.


Diffs (updated)
-

  src/tests/persistent_volume_endpoints_tests.cpp 
22e18758ee91a649486725473d9e50fae9d43b01 

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


Testing
---

A new test, `PersistentVolumeEndpointsTest.NoAuthorization`, was added to the 
persistent volume endpoint tests.

`make check` was used to test, and the new test was run with 
`--gtest_repeat=1000 -gtest_break_on_failure=1`.


Thanks,

Greg Mann

Re: Review Request 41092: CMake: Added CMake file for agent executable build.

[Joseph Wu]

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


I was expecting a `mesos-slave` executable, but I don't see any 
`add_executable` (or something that actually generates an executable) in this 
patch.  Was that intentional?


src/slave/CMakeLists.txt (lines 17 - 19)


Clean this up?



src/slave/CMakeLists.txt (line 42)


Nit: Pre-commit hooks complain about this line.  (Delete it)


- Joseph Wu


On Jan. 21, 2016, 8:58 a.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41092/
> ---
> 
> (Updated Jan. 21, 2016, 8:58 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Bugs: MESOS-3843
> https://issues.apache.org/jira/browse/MESOS-3843
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added CMake file for agent executable build.
> 
> 
> Diffs
> -
> 
>   src/slave/CMakeLists.txt PRE-CREATION 
>   src/slave/cmake/SlaveConfigure.cmake 
> fbdfdaa27fbd8c7429861eea5baf401a221f748b 
> 
> Diff: https://reviews.apache.org/r/41092/diff/
> 
> 
> Testing
> ---
> 
> Tested on Ubuntu and OSX.
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Re: Review Request 41305: MESOS-1718: move getExecutorInfo from slave to master.

[Joseph Wu]

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


General questions:

- Besides removing the possibility for overcommit, does this change give us 
anything else?
- What happens if a master sends this modified `TaskInfo` to an agent that does 
not have this chain of patches?
- Have you considered completely converting a `CommandInfo` to `ExecutorInfo`?  
If we do that, it may be possible to remove all the other places with special 
logic for command tasks.


src/master/master.cpp (line 2860)


`launcher_dir` is an optional field.

Same for the other fields you added (`sandbox_dir`, `switch_user`, 
`executor_rootfs).



src/master/master.cpp (lines 3770 - 3779)


It should still be invalid to supply both a CommandInfo and ExecutorInfo in 
the same TaskInfo.


- Joseph Wu


On Jan. 18, 2016, 6:55 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41305/
> ---
> 
> (Updated Jan. 18, 2016, 6:55 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Guangya Liu, Artem Harutyunyan, Ian 
> Downes, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
> 
> 
> Bugs: MESOS-1718
> https://issues.apache.org/jira/browse/MESOS-1718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-1718: move getExecutorInfo from slave to master
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp 2c3299bc1aaa0888f7e47a71965c56ada8ecc21f 
>   src/master/master.hpp 3a7e18232323a1c051bcc97915484b1195fffe58 
>   src/master/master.cpp 9ee56277c8a472be9e683d5db505becfb5f7c422 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/slave.hpp b7586ce42bfac9d9885a3eb8d82deb94680c236c 
>   src/slave/slave.cpp bb501810d7bb1261ebbbdd147c49948e5a2f8665 
> 
> Diff: https://reviews.apache.org/r/41305/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 42590: Removed reserved() API.

[Guangya Liu]

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

(Updated 一月 22, 2016, 3:37 a.m.)


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


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


Repository: mesos


Description
---

Removed reserved() API.


Diffs (updated)
-

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

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


Testing
---

make
make check


Thanks,

Guangya Liu

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

[Greg Mann]

> On Jan. 21, 2016, 10:42 p.m., Michael Park wrote:
> >

Thanks MPark! Per our discussion, I've removed all of the requests from this 
test except the reserve and unreserve requests which contain no authentication 
headers as well as no principal in `ReservationInfo`, since the other requests 
were actually either testing for non-matching principals or testing that HTTP 
authentication behaves as we expect.


- Greg


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


On Jan. 22, 2016, 3:33 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42368/
> ---
> 
> (Updated Jan. 22, 2016, 3:33 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Michael Park, and Vinod Kone.
> 
> 
> Bugs: MESOS-4195
> https://issues.apache.org/jira/browse/MESOS-4195
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added reservation endpoint test without authentication.
> 
> Currently, dynamic reservation endpoints will not work when HTTP 
> authentication is not set. Currently, the authentication code will always 
> return `None()` as the principal when authentication is disabled. 
> Furthermore, the `ReservationInfo.principal` field is being migrated to 
> `optional`, so dynamic reservations without a principal are invalidated by 
> the master. This test checks that these endpoints will fail as expected when 
> HTTP authentication is disabled.
> 
> 
> Diffs
> -
> 
>   src/tests/reservation_endpoints_tests.cpp 
> a471f858061a694073afeebdf76be659da938d01 
> 
> Diff: https://reviews.apache.org/r/42368/diff/
> 
> 
> Testing
> ---
> 
> A new test, `ReservationEndpointsTest.ReserveAndUnreserveNoAuthentication`, 
> was added, and make check was used to test. The new test was also run with 
> `--gtest_repeat=1000 --gtest_break_on_failure=1`.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 41950: Cleaned up hierarchical allocator tests.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [41950]

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

Error:
 2016-01-22 03:35:47 URL:https://reviews.apache.org/r/41950/diff/raw/ 
[24032/24032] -> "41950.patch" [1]
error: patch failed: src/tests/hierarchical_allocator_tests.cpp:1803
error: src/tests/hierarchical_allocator_tests.cpp: patch does not apply

- Mesos ReviewBot


On Jan. 22, 2016, 1:20 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41950/
> ---
> 
> (Updated Jan. 22, 2016, 1:20 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van 
> Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes made:
> - empty resource map promoted to a const class field;
> - removed variable numeric suffixes where appropriate;
> - added const where appropriate.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> e044f832c2c16e53e663c6ced5452649bb0dcb59 
> 
> Diff: https://reviews.apache.org/r/41950/diff/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS 10.10.5
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

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

[Greg Mann]

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

(Updated Jan. 22, 2016, 3:33 a.m.)


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


Changes
---

Addressed comments; removed extraneous test cases.


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


Repository: mesos


Description
---

Added reservation endpoint test without authentication.

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


Diffs (updated)
-

  src/tests/reservation_endpoints_tests.cpp 
a471f858061a694073afeebdf76be659da938d01 

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


Testing
---

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


Thanks,

Greg Mann

Re: Review Request 42274: Added common command utils file.

[Jojy Varghese]

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

(Updated Jan. 22, 2016, 12:55 a.m.)


Review request for mesos and Jie Yu.


Changes
---

added method for sha512 digest.


Repository: mesos


Description
---

This common file is a good place to add common command line utilities like tar,
digests(sha256, sha512, etc). Currently this functionality is spread in the code
base and this change would enable all those call sites to be replaced with a
common code.


Diffs (updated)
-

  src/CMakeLists.txt 39a23df3227a4f524ea0d408dc894fa5bbab7d10 
  src/Makefile.am 8cbfb1ba5fa49f2d3cc26ea325838a1c68a79660 
  src/common/command_utils.hpp PRE-CREATION 
  src/common/command_utils.cpp PRE-CREATION 
  src/tests/common/command_utils_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

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

[Guangya Liu]

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

Got it, will update here to `slaves[slaveId].total = 
slaves[slaveId].total.nonUsageSlack() + oversubscribed;` as you proposed. 
Thanks ;-)


- Guangya


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


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

Re: Review Request 42605: Fixed a NULL pointer dereference bug in Slave.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [42605]

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

- Mesos ReviewBot


On Jan. 21, 2016, 6:11 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42605/
> ---
> 
> (Updated Jan. 21, 2016, 6:11 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-4449
> https://issues.apache.org/jira/browse/MESOS-4449
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixes a `NULL` pointer dereference erroneously added as part of 
> introducing an output operator for the `Executor` struct in 
> https://reviews.apache.org/r/39569.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp e23c3295c8ebed580751a5aabaf26e1773c54859 
> 
> Diff: https://reviews.apache.org/r/42605/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Review Request 42634: Updated a comment for consistency.

[Alexander Rukletsov]

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

Review request for mesos and Ben Mahler.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
953712149bd951789beb29c72779c4ac65aa48dc 

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


Testing
---

None


Thanks,

Alexander Rukletsov

Review Request 42636: Replaced term 'periodic allocation' for consistency.

[Alexander Rukletsov]

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

Review request for mesos and Ben Mahler.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
953712149bd951789beb29c72779c4ac65aa48dc 

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


Testing
---

None


Thanks,

Alexander Rukletsov

Review Request 42632: Cleaned up formatting in the allocator.

[Alexander Rukletsov]

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

Review request for mesos and Ben Mahler.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/master/allocator/mesos/hierarchical.hpp 
18f77bcf596c2ee17b4e407fac4367b0c737ce82 
  src/master/allocator/mesos/hierarchical.cpp 
53d8784a3c4f0467756d7bd924e624ba4585e776 

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


Testing
---

None


Thanks,

Alexander Rukletsov

Review Request 42631: Renamed resource offer timeout for clarity.

[Alexander Rukletsov]

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

Review request for mesos and Ben Mahler.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
53d8784a3c4f0467756d7bd924e624ba4585e776 

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


Testing
---

make check


Thanks,

Alexander Rukletsov

Review Request 42629: Added tests for offer filters.

[Alexander Rukletsov]

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

Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
953712149bd951789beb29c72779c4ac65aa48dc 

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


Testing
---

On Mac OS 10.10.4:

`make check`

`GTEST_FILTER="HierarchicalAllocatorTest.FilterTimeout" ./bin/mesos-tests.sh 
--gtest_repeat=100 --gtest_break_on_failure` passes with the patch and fails 
without.

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


Thanks,

Alexander Rukletsov

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

[Anand Mazumdar]

> On Jan. 21, 2016, 7:33 p.m., Vinod Kone wrote:
> >

Moved the code to:

`connected`: Invoke this callback after `Subscribe`/non subscribe connection 
have been established.
`disconnected`: Invoke this callback after any of `Subscribe`/non subscribe 
connection is broken.


> On Jan. 21, 2016, 7:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 170
> > 
> >
> > should be a CHECK as this is not expected.

As per our discussion, I am keeping this as is since it's in sync with 
`src/exec/exec.cpp`. Would file another patch to update this in both.


> On Jan. 21, 2016, 7:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, lines 238-259
> > 
> >
> > I think it would be a bit clearer to follow if you do something like 
> > this
> > 
> > ```
> > if (call.type() == Call::SUBSCRIBE) {
> >   // If we are in `CONNECTED` state, subscribe connection should exist.
> >   CHECK_SOME(subscribe);
> >   
> >   _send(call);
> > } else {
> >   if (nonSubscribe.isSome()) {
> > // If nonSubscribe connection already exists, use it. 
> > _send(call);
> >   } else {
> > // Create a new nonSubscribe connection.
> > ...
> >   }
> > }
> > 
> > ```

The given code was removed. Hence, dropping the issue.


> On Jan. 21, 2016, 7:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 241
> > 
> >
> > Don't we have to check here that nonSubscribe is already set? For 
> > example, 2 back to back non-subscribe calls might result in 2 non-subscribe 
> > connections?

The given code is removed. Dropping.


> On Jan. 21, 2016, 7:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 269
> > 
> >
> > Lets comment here that the library is considered connected if we can 
> > establish the subscribe connection. Worth mentioning here that 
> > non-subscribe calls use a different `nonSubscribe` connection and that its 
> > disconnection doesn't change the `state`.

Added a comment before we invoke the `connected` callback that this is only 
invoked after we establish both `Subscribe`/Non-subscribe connections.


> On Jan. 21, 2016, 7:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 309
> > 
> >
> > How is this possible? Can you expand the comment?

Added a comment. The first time `disconnected` is invoked. We are still in 
`CONNECTED` state. Since, we backoff and retry, the connection state can again 
go from `DISCONNECTED`->`CONNECTED` with a separate `Subscribe` connection. We 
don't do anything thereafter and just return.


> On Jan. 21, 2016, 7:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, lines 376-378
> > 
> >
> > Can there be more than one oustanding connection attempts at any given 
> > time?

There cannot be. However, we need to compare the connection objects due to this 
scenario:

We disconnect from agent, start the recoveryTimeout clock.
We connect now after some time.
We disconnect again, start another recoveryTimeout clock.

We should now terminate when the second recoveryTimeout clock expires and we 
should ignore the first clock expiration. Passing the `Connection` object 
allows us to do that. This is similar to the existing logic in 
`src/exec/exec.cpp`


> On Jan. 21, 2016, 7:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 601
> > 
> >
> > can you add a comment on why you do `false` here?

Added a comment. The intention here was to process the pending `received` 
events from the agent before terminating.


> On Jan. 21, 2016, 7:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 624
> > 
> >
> > Add a comment here (or above where I commented) that the state tracks 
> > the `subscribe` connection. Also, do we really need this enum? Can't we 
> > just use `subscribe` to figure if we are connected or not?

Added a comment. Also, see my earlier comment regarding comparing connection 
object in `_recoveryTimeout`. We cannot initialize `subscribe` to `None` due to 
this. Hence, we need this enum to be the source of truth as to whether we are 
disconnected or not.


- Anand


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


On Jan. 22, 2016, 1:36 a.m., 

Re: Review Request 41648: Used initializer list c-tor for brevity.

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On 一月 22, 2016, 1:24 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41648/
> ---
> 
> (Updated 一月 22, 2016, 1:24 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ben Mahler, Joerg Schad, and Joris 
> Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 53d8784a3c4f0467756d7bd924e624ba4585e776 
> 
> Diff: https://reviews.apache.org/r/41648/diff/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 42633: Corrected a comment in the allocator.

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On 一月 22, 2016, 1:24 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42633/
> ---
> 
> (Updated 一月 22, 2016, 1:24 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 18f77bcf596c2ee17b4e407fac4367b0c737ce82 
> 
> Diff: https://reviews.apache.org/r/42633/diff/
> 
> 
> Testing
> ---
> 
> None
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 42637: Cleaned up formatting in allocator tests.

[Guangya Liu]

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



src/tests/hierarchical_allocator_tests.cpp (line 795)


I think that another clean up is also needed, as some tests are using 
`agent` while some using `slave` as variable name, it is better to make 
consistent in this file.


- Guangya Liu


On 一月 22, 2016, 1:24 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42637/
> ---
> 
> (Updated 一月 22, 2016, 1:24 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 953712149bd951789beb29c72779c4ac65aa48dc 
> 
> Diff: https://reviews.apache.org/r/42637/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41648: Used initializer list c-tor for brevity.

[Alexander Rukletsov]

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

(Updated Jan. 22, 2016, 1:24 a.m.)


Review request for mesos, Bernd Mathiske, Ben Mahler, Joerg Schad, and Joris 
Van Remoortere.


Changes
---

Rebased.


Repository: mesos


Description (updated)
---

See summary.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
53d8784a3c4f0467756d7bd924e624ba4585e776 

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


Testing
---

`make check` on Mac OS 10.10.4


Thanks,

Alexander Rukletsov

Review Request 42633: Corrected a comment in the allocator.

[Alexander Rukletsov]

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

Review request for mesos and Ben Mahler.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/master/allocator/mesos/hierarchical.hpp 
18f77bcf596c2ee17b4e407fac4367b0c737ce82 

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


Testing
---

None


Thanks,

Alexander Rukletsov

Re: Review Request 42623: Reduced severity level for 'HTTP GET for ...' messages.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [42606, 42623]

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

- Mesos ReviewBot


On Jan. 21, 2016, 11:44 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42623/
> ---
> 
> (Updated Jan. 21, 2016, 11:44 p.m.)
> 
> 
> Review request for mesos, Cody Maloney and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-4430
> https://issues.apache.org/jira/browse/MESOS-4430
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed it from `LOG(INFO)` to `VLOG(2)`.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 34a70ee50553492fc8c3947497ab5922f4379d72 
>   src/slave/http.cpp 0ba2e82fd8ce58508f364aff761d50dc4f264f65 
> 
> Diff: https://reviews.apache.org/r/42623/diff/
> 
> 
> Testing
> ---
> 
> Verified that the log messages don't appear in the logs.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 42632: Cleaned up formatting in the allocator.

[Guangya Liu]

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

Ship it!


Ship It!


src/master/allocator/mesos/hierarchical.cpp 


I think that it is better adding some log here to clarify what is 
happending: Why this agent was not offeredd even this agent has enough 
resources. I will upload a patch to fix this.


- Guangya Liu


On 一月 22, 2016, 1:24 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42632/
> ---
> 
> (Updated 一月 22, 2016, 1:24 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 18f77bcf596c2ee17b4e407fac4367b0c737ce82 
>   src/master/allocator/mesos/hierarchical.cpp 
> 53d8784a3c4f0467756d7bd924e624ba4585e776 
> 
> Diff: https://reviews.apache.org/r/42632/diff/
> 
> 
> Testing
> ---
> 
> None
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 42547: Added helper function to get non usage slack resources.

[Guangya Liu]

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

(Updated 一月 22, 2016, 3:15 a.m.)


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


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


Repository: mesos


Description
---

Added helper function to get non usage slack resources.


Diffs (updated)
-

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

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 41950: Cleaned up hierarchical allocator tests.

[Alexander Rukletsov]

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

(Updated Jan. 22, 2016, 1:20 a.m.)


Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.


Repository: mesos


Description
---

Changes made:
- empty resource map promoted to a const class field;
- removed variable numeric suffixes where appropriate;
- added const where appropriate.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
e044f832c2c16e53e663c6ced5452649bb0dcb59 

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


Testing
---

`make check` on Mac OS 10.10.5


Thanks,

Alexander Rukletsov

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

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [42530, 42361, 42368]

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

- Mesos ReviewBot


On Jan. 20, 2016, 6:59 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42368/
> ---
> 
> (Updated Jan. 20, 2016, 6:59 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Michael Park, and Vinod Kone.
> 
> 
> Bugs: MESOS-4195
> https://issues.apache.org/jira/browse/MESOS-4195
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added reservation endpoint test without authentication.
> 
> Currently, dynamic reservation endpoints will not work when HTTP 
> authentication is not set. Currently, the authentication code will always 
> return `None()` as the principal when authentication is disabled. 
> Furthermore, the `ReservationInfo.principal` field is being migrated to 
> `optional`, so dynamic reservations without a principal are invalidated by 
> the master. This test checks that these endpoints will fail as expected when 
> HTTP authentication is disabled.
> 
> 
> Diffs
> -
> 
>   src/tests/reservation_endpoints_tests.cpp 
> b8edd6fafedd4c2221a8d19c1ebc71254071a8c7 
> 
> Diff: https://reviews.apache.org/r/42368/diff/
> 
> 
> Testing
> ---
> 
> A new test, `ReservationEndpointsTest.ReserveAndUnreserveNoAuthentication`, 
> was added, and make check was used to test. The new test was also run with 
> `--gtest_repeat=1000 --gtest_break_on_failure=1`.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 42157: Changed ProvisionerAppcTest to use AppcStoreTest suite.

[Jojy Varghese]

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

(Updated Jan. 22, 2016, 6:48 a.m.)


Review request for mesos and Jie Yu.


Changes
---

rebased


Repository: mesos


Description
---

This change will enable ProvisionerAppcTest suite to reuse common code like
test image creation.


Diffs (updated)
-

  src/tests/containerizer/provisioner_appc_tests.cpp 
e3d08d9e49df93d5290099c8bfd917f60c93e51b 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 42274: Added common command utils file.

[Jojy Varghese]

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

(Updated Jan. 22, 2016, 6:52 a.m.)


Review request for mesos and Jie Yu.


Changes
---

rebased.


Repository: mesos


Description
---

This common file is a good place to add common command line utilities like tar,
digests(sha256, sha512, etc). Currently this functionality is spread in the code
base and this change would enable all those call sites to be replaced with a
common code.


Diffs (updated)
-

  src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e 
  src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f 
  src/common/command_utils.hpp PRE-CREATION 
  src/common/command_utils.cpp PRE-CREATION 
  src/tests/common/command_utils_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

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

[Joseph Wu]

> On Jan. 20, 2016, 2:54 p.m., Joseph Wu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 662
> > 
> >
> > Have you considered modifying `Resources::apply` to "create" allocation 
> > slack upon a reservation?  (And to "destroy" allocation slack upon 
> > unreserve?)
> 
> Guangya Liu wrote:
> I do not want to update `Resources::apply` as it will be called when 
> `reserve` and `unreserve` resources, when `reserve` resources, we can simply 
> add some `allocation slack`; when `unreserve` resources, life become complex, 
> we cannot simply decrease the `allocation slack` as if the `allocation slack` 
> decreased, there will be new `unreserved` resources which can be send as 
> offer and launch task because the `allocation slack` still running task and 
> there might be resource confilict for this, that's why I have another 
> following two patches handlding `unreserve` separately.
> 
> Take a case:
> 1) agent: cpus(r1):100;cpus(*){ALLOCATION_SLACK}:100
> 2) framework use up all cpus(*){ALLOCATION_SLACK}:100 resources.
> 3) unreserve 30 cpus.
> 4) The resources would become:
> Total: cpus(*):30;cpus(r1):70;cpus(*){ALLOCATION_SLACK}:100 NOTE: We do 
> not decrease the total allocation slack here.
> Free: cpus(*):30;cpus(r1):70
> Used: cpus(*){ALLOCATION_SLACK}:100
> 5) At this time, we cannot send cpus(*):30 out as offer because there are 
> still 30 over committed allocation slack in use.
> 6) recover 20 allocaiton slack, we can update the total resources as this:
> Total: cpus(*):30;cpus(r1):70;cpus(*){ALLOCATION_SLACK}:80 NOTE: decrease 
> the total allocation slack when recover resource but not when `unreserve` 
> resources in such case.
> Used: cpus(*){ALLOCATION_SLACK}:80
> Free: cpus(*):30;cpus(r1):70
> The allocate can send out offer with cpus(*):20 but not the whole 30 
> resources.
> 7) recover another 10 allocation slack.
> Total: cpus(*):30;cpus(r1):70;cpus(*){ALLOCATION_SLACK}:70 NOTE: from now 
> on, the allocatioin slack is same as reserved resources.
> Used: cpus(*)20;cpus(*){ALLOCATION_SLACK}:70
> Free: cpus(*):10;cpus(r1):70
> The allocater can send out offer with cpus(*):10.

Hm...  How about this:
- You calculate the "total" allocation slack strictly.  
- * Reserve   => Total allocation slack increases.
- * Unreserve => Total allocation slack decreases.
- * Allocator::recoverResources => Total allocation slack unchanged.
- * Master::removeExecutor (for executors using allocation slack)   => Total 
allocation slack unchanged.
- * Master::removeExecutor (for executors using reserved resources) => Total 
allocation slack increases.
- * Master::evictExecutor (new method) => Total allocation slack decreases.
 ^^ The agent can report exactly how much allocation slack should decrease.  We 
recover all allocation slack from the evicted executor and subtract reserved 
resources from the new ("evicting") executor.

In this scheme, the "allocated" allocation slack can surpass the "total" (i.e. 
MESOS-4442), but the totals should remain consistent.


- Joseph


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


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

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

[Joseph Wu]

> On Jan. 19, 2016, 3:34 p.m., Joseph Wu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 676
> > 
> >
> > I see several places where you overwrite changes from this review in 
> > the subsequent two.  Did you mean to have three reviews for the changes to 
> > `::updateAllocation`, `::updateAvailable`, and `::recoverResources`?
> 
> Guangya Liu wrote:
> There are three patches handling dynamic reservation and 
> 1) https://reviews.apache.org/r/41791 Reserve new resources via 
> `updateAllocation` and `updateAvailable`. (1/3).
> 2) https://reviews.apache.org/r/42113 Unreserve resources via 
> `updateAllocation` (2/3).
> 3) https://reviews.apache.org/r/42194 Unreserve resources via 
> `updateAvailable` (3/3).
> 
> I have updated the description here.
> 
> Joseph Wu wrote:
> It doesn't seem like you need 3 patches.
> 
> I'd recommend a couple of cleanups:
> 
> * Add a helper for all the duplicate logic you have.  Both 
> `updateAllocation` and `updateAvailable` deal with reservations, in 
> essentially the same way (per optimistic offers).
> * Consolidate the overlapping changes into a single patch.  Just don't 
> overwrite your own changes.
> * Consider a different approach than subtracting allocation slack on 
> unreserve.  You could perhaps, for accounting purposes, elevate a portion of 
> the allocation from revocable to non-revocable.
> 
> Guangya Liu wrote:
> The reason I do not add a helper function because even though the code 
> are almost same, there are indeed difference between logs, `updateAllocation` 
> will have some logs related to `framework` and `agent`, `updateAvailable` 
> will only have some logs related to `agent`. I prefer that we keep the 
> changes separtely to make log message clear for different cases, comments? 
> Thanks.

For the log differences, you can just pass in the `FrameworkID` as an 
`Option frameworkId`.
Then, in your logs, add: `<< (frameworkId.isSome() ? " for framework " + 
frameworkId.get() : "")`.


- Joseph


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


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