Re: Review Request 56376: Updated allocator test to support create multi role framework.

2017-02-14 Thread Guangya Liu

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

(Updated 二月 14, 2017, 10:40 a.m.)


Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Updated allocator test to support create multi role framework.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
a866d03c0b7a676d08fb2fb1e321133c9f5363fc 

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


Testing
---

make
make check


Thanks,

Guangya Liu



Re: Review Request 56376: Updated allocator test to support create multi role framework.

2017-02-14 Thread Guangya Liu


> On 二月 13, 2017, 11:31 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 221-222
> > <https://reviews.apache.org/r/56376/diff/2/?file=1630723#file1630723line221>
> >
> > You should probably do this only if the caller did not specify the 
> > multi role capability explicitly? Or FAIL the test if the caller specifies 
> > it with a message saying that we mandate that these tests are all using 
> > MULTI_ROLE, so no need to pass it.

OK, I will do this only if the caller did not specify the multi role capability 
explicitly, this option seems simple and also easy to be used by others when 
creating multi role frameworks.


- Guangya


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


On 二月 11, 2017, 10:09 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56376/
> ---
> 
> (Updated 二月 11, 2017, 10:09 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated allocator test to support create multi role framework.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> a866d03c0b7a676d08fb2fb1e321133c9f5363fc 
> 
> Diff: https://reviews.apache.org/r/56376/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>



Re: Review Request 56378: Added test case for suppress and revive with multi role framework.

2017-02-11 Thread Guangya Liu

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

(Updated 二月 11, 2017, 10:29 a.m.)


Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Added test case for suppress and revive with multi role framework.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
a866d03c0b7a676d08fb2fb1e321133c9f5363fc 

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


Testing
---

make
make check

```
./bin/mesos-tests.sh  
--gtest_filter="HierarchicalAllocatorTest.SuppressAndReviveOffersWithMultiRole" 
--gtest_repeat=100
```


Thanks,

Guangya Liu



Re: Review Request 56376: Updated allocator test to support create multi role framework.

2017-02-11 Thread Guangya Liu

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

(Updated 二月 11, 2017, 10:09 a.m.)


Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Updated allocator test to support create multi role framework.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
a866d03c0b7a676d08fb2fb1e321133c9f5363fc 

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


Testing
---

make
make check


Thanks,

Guangya Liu



Re: Review Request 56370: Added a test to ensure multi-role framework receiving offers.

2017-02-10 Thread Guangya Liu

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


Fix it, then Ship it!




Ship It!


src/tests/master_tests.cpp (line 6379)
<https://reviews.apache.org/r/56370/#comment237057>

s/->/.get().



src/tests/master_tests.cpp (line 6382)
<https://reviews.apache.org/r/56370/#comment237058>

ditto


- Guangya Liu


On 二月 9, 2017, 2:28 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56370/
> ---
> 
> (Updated 二月 9, 2017, 2:28 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and 
> Michael Park.
> 
> 
> Bugs: MESOS-7062
> https://issues.apache.org/jira/browse/MESOS-7062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test ensures that multi-role framework should receive offers for
> each of its roles.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 3b4123b49ee32c902a5d2a01fcc7026da21fdd18 
> 
> Diff: https://reviews.apache.org/r/56370/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> make check GTEST_FILTER="MasterTest.MultiRoleFrameworkReceivesOffers"
> 
> 
> Thanks,
> 
> Jay Guo
> 
>



Re: Review Request 56569: Made ignoring logging use WARNING in master.

2017-02-10 Thread Guangya Liu

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

(Updated 二月 11, 2017, 1:52 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Updated commit message and also more ignore log message use WARNING.


Summary (updated)
-

Made ignoring logging use WARNING in master.


Repository: mesos


Description (updated)
---

Made ignoring logging use WARNING in master.


Diffs (updated)
-

  src/master/master.cpp 0cf81adeb1d087f298a9c70cfb40179ad457bed2 

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


Testing
---

make
make check


Thanks,

Guangya Liu



Review Request 56569: Updated `drop` log message from `ERROR` to `WARNING`.

2017-02-10 Thread Guangya Liu

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Updated `drop` log message from `ERROR` to `WARNING`.


Diffs
-

  src/master/master.cpp 80d481b2ea1435147cd213383008435f35112d92 

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


Testing
---

make
make check


Thanks,

Guangya Liu



Re: Review Request 56524: Added `drop` for overload to avoid custom logging.

2017-02-10 Thread Guangya Liu

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

(Updated 二月 11, 2017, 1:02 a.m.)


Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Added `drop` for overload to avoid custom logging.


Diffs (updated)
-

  src/master/master.hpp 70f50f4264584be55312842b038c925687afb2cb 
  src/master/master.cpp 80d481b2ea1435147cd213383008435f35112d92 

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


Testing
---

make
make check


Thanks,

Guangya Liu



Re: Review Request 56524: Added `drop` for overload to avoid custom logging.

2017-02-10 Thread Guangya Liu


> On 二月 10, 2017, 9:27 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp, lines 2246-2247
> > <https://reviews.apache.org/r/56524/diff/1/?file=1629166#file1629166line2246>
> >
> > Can you follow the same format at the drop two lines above?
> > 
> > ```
> >   LOG(ERROR) << "Dropping " << call.type() << " call"
> >  << " from framework " << *framework
> >  << ": " << message;
> > ```
> > 
> > Not yours, but just as an aside it seems like all of the drop logging 
> > should probably be at the warning instead of error level. Feel free to 
> > leave as is for now.

Added a `TODO` here to follow up the `WARNING` message.

```
// TODO(gyliu513): Update all `drop` logging to `WARNING`.
LOG(ERROR) << "Dropping " << call.type() << " call"
   << " from framework " << *framework
   << ": " << message;
```


- Guangya


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


On 二月 10, 2017, 1:42 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56524/
> ---
> 
> (Updated 二月 10, 2017, 1:42 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `drop` for overload to avoid custom logging.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 767ac48f2a65514a19c4a3f8ca7b35f0259f9aeb 
>   src/master/master.cpp 620919ecfe85367b5c1281afc5216cc20e5e2e3c 
> 
> Diff: https://reviews.apache.org/r/56524/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>



Re: Review Request 56499: Added roles field to framework.

2017-02-09 Thread Guangya Liu


> On 二月 9, 2017, 9:38 p.m., Benjamin Mahler wrote:
> > src/master/master.hpp, lines 2425-2451
> > <https://reviews.apache.org/r/56499/diff/1/?file=1628557#file1628557line2425>
> >
> > We need to update the newly introduced `roles` field here. Also, 
> > oldRoles can be removed in favor of directly accessing `roles`.

Since we do not allow updating `roles` for frameworks and the `roles` was 
already initialized when construct the framework, so do we still need to update 
the `roles` here?


- Guangya


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


On 二月 10, 2017, 12:24 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56499/
> ---
> 
> (Updated 二月 10, 2017, 12:24 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added roles field to framework.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 767ac48f2a65514a19c4a3f8ca7b35f0259f9aeb 
>   src/master/master.cpp 620919ecfe85367b5c1281afc5216cc20e5e2e3c 
> 
> Diff: https://reviews.apache.org/r/56499/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>



Review Request 56524: Added `drop` for overload to avoid custom logging.

2017-02-09 Thread Guangya Liu

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

Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Added `drop` for overload to avoid custom logging.


Diffs
-

  src/master/master.hpp 767ac48f2a65514a19c4a3f8ca7b35f0259f9aeb 
  src/master/master.cpp 620919ecfe85367b5c1281afc5216cc20e5e2e3c 

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


Testing
---

make
make check


Thanks,

Guangya Liu



Re: Review Request 56499: Added roles field to framework.

2017-02-09 Thread Guangya Liu

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

(Updated 二月 10, 2017, 12:24 a.m.)


Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Added roles field to framework.


Diffs (updated)
-

  src/master/master.hpp 767ac48f2a65514a19c4a3f8ca7b35f0259f9aeb 
  src/master/master.cpp 620919ecfe85367b5c1281afc5216cc20e5e2e3c 

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


Testing
---

make
make check


Thanks,

Guangya Liu



Review Request 56499: Added roles field to framework.

2017-02-09 Thread Guangya Liu

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

Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Added roles field to framework.


Diffs
-

  src/master/master.hpp 767ac48f2a65514a19c4a3f8ca7b35f0259f9aeb 
  src/master/master.cpp 620919ecfe85367b5c1281afc5216cc20e5e2e3c 

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


Testing
---

make
make check


Thanks,

Guangya Liu



Re: Review Request 56376: Updated allocator test to support create multi role framework.

2017-02-09 Thread Guangya Liu


> On 二月 8, 2017, 6:48 a.m., Jay Guo wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 203-205
> > <https://reviews.apache.org/r/56376/diff/1/?file=1626287#file1626287line203>
> >
> > IMO, since this is for testing purpose, we probably don't need to 
> > rigidly require test writers to construct frameworkInfo with `MULTI_ROLE` 
> > capability. If `createFrameworkInfo` is called with multiple roles, we 
> > could simply add `MULTI_ROLE` implicitly. And one could still explicitly 
> > add `MULTI_ROLE` to capabilities with a single role. Then we don't need to 
> > change existing test cases and avoid future confusion. What do you think?
> 
> Guangya Liu wrote:
> Yes, but the only problem is that we cannot specfify multiple roles with 
> the first paramter here `const string& role`, so I have to update it to 
> `const set& roles`.
> 
> Jay Guo wrote:
> I agree that tests need to be updated to pass in `set`. However, 
> I feel return type `Try` is a bit complicated. In this form, 
> should we perform `isSome()` check? I would suggest to stick to 
> `FrameworkInfo createFrameworkInfo(...)`.

As I added some error handling as folllowing, so I have to return `Try` here, 
any comments? 

```
if (!multiRole && roles.size() > 1) {
  return Error("Non multi-role framework support only one role");
}
```


- Guangya


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


On 二月 7, 2017, 10:10 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56376/
> ---
> 
> (Updated 二月 7, 2017, 10:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated allocator test to support create multi role framework.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> c681d03c3f94f7d071143366a5aad0421108ebec 
> 
> Diff: https://reviews.apache.org/r/56376/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>



Re: Review Request 56376: Updated allocator test to support create multi role framework.

2017-02-08 Thread Guangya Liu


> On 二月 8, 2017, 6:48 a.m., Jay Guo wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 203-205
> > <https://reviews.apache.org/r/56376/diff/1/?file=1626287#file1626287line203>
> >
> > IMO, since this is for testing purpose, we probably don't need to 
> > rigidly require test writers to construct frameworkInfo with `MULTI_ROLE` 
> > capability. If `createFrameworkInfo` is called with multiple roles, we 
> > could simply add `MULTI_ROLE` implicitly. And one could still explicitly 
> > add `MULTI_ROLE` to capabilities with a single role. Then we don't need to 
> > change existing test cases and avoid future confusion. What do you think?

Yes, but the only problem is that we cannot specfify multiple roles with the 
first paramter here `const string& role`, so I have to update it to `const 
set& roles`.


- Guangya


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


On 二月 7, 2017, 10:10 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56376/
> ---
> 
> (Updated 二月 7, 2017, 10:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated allocator test to support create multi role framework.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> c681d03c3f94f7d071143366a5aad0421108ebec 
> 
> Diff: https://reviews.apache.org/r/56376/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>



Re: Review Request 56374: Enabled revive offer per role.

2017-02-08 Thread Guangya Liu

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

(Updated 二月 9, 2017, 6:36 a.m.)


Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Enabled revive offer per role.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
71a40537b673e44ecdd5327d9a9f083faa7fc13a 
  src/master/allocator/mesos/allocator.hpp 
e3c86181677302dbfc3b213715196122f96e312c 
  src/master/allocator/mesos/hierarchical.hpp 
896abcdf0727f986eef3a1a9304a0e4847094057 
  src/master/allocator/mesos/hierarchical.cpp 
017253cc8f5fbcccd9d4057c5b189f352779513c 
  src/master/master.cpp 0b65345d48192a1536d43973cf782ade3c1c8163 
  src/tests/allocator.hpp 32c291213d18d1c8fe5d9e8194b92c10716b9961 
  src/tests/hierarchical_allocator_tests.cpp 
c681d03c3f94f7d071143366a5aad0421108ebec 

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


Testing
---

make
make check


Thanks,

Guangya Liu



Re: Review Request 56371: Enabled `ReviveOffersMessage` support revive per role.

2017-02-08 Thread Guangya Liu

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

(Updated 二月 9, 2017, 6:21 a.m.)


Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Enabled `ReviveOffersMessage` support revive per role.


Diffs (updated)
-

  src/master/master.hpp 32915d049a2d91d648b7d33c15ef50c8bc0c72cd 
  src/master/master.cpp 0b65345d48192a1536d43973cf782ade3c1c8163 
  src/messages/messages.proto 7a2f37b78a8edcd372558f77f15e6b249742e321 

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


Testing
---

make
make check


Thanks,

Guangya Liu



Re: Review Request 56371: Enabled `ReviveOffersMessage` support revive per role.

2017-02-08 Thread Guangya Liu


> On 二月 8, 2017, 9:20 p.m., Benjamin Mahler wrote:
> > I'm fine adding this here, but it seems weird that we wold update the 
> > protobuf but not update the scheduler driver. Are we assuming that other 
> > bindings might benefit from this? Are we still updating the old API?

Had some offline discussion with Anand, till now, the Go bindings are the only 
one that use the old style protobuf handlers on the master currently, but they 
would eventually move to use the v1 API too once James merges the changes to 
use the v1 API from the next branch 
(https://github.com/mesos/mesos-go/tree/next) to the master, so I will not 
update the old API, comments?


- Guangya


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


On 二月 7, 2017, 10:10 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56371/
> ---
> 
> (Updated 二月 7, 2017, 10:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled `ReviveOffersMessage` support revive per role.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp ceb9bf7e4be7e0e43a166d7706bd0ab4fd9ed578 
>   src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
>   src/messages/messages.proto 7a2f37b78a8edcd372558f77f15e6b249742e321 
> 
> Diff: https://reviews.apache.org/r/56371/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>



Re: Review Request 56330: Enabled suppress offer per role.

2017-02-08 Thread Guangya Liu


> On 二月 8, 2017, 9:44 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp, lines 3230-3236
> > <https://reviews.apache.org/r/56330/diff/2/?file=1626023#file1626023line3230>
> >
> > Why do we need to validate the role? It seems sufficient to just check 
> > whether it is one of the framework's roles since all of the framework roles 
> > are valid.

Yes, all of the frameworks roles are valid here, but there maybe cases that the 
framework developer set an `invalid role` when setting the `Call:SUPPRESS` in 
the framework? 

I updated the comments a bit here:

```
// There maybe cases that the framework developer set an invalid role
// when constructing `scheduler::Call::Suppress`.
```


> On 二月 8, 2017, 9:44 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp, lines 3238-3246
> > <https://reviews.apache.org/r/56330/diff/2/?file=1626023#file1626023line3238>
> >
> > " because it is not one of the framework's subscribed roles"
> > 
> > I would suggest two follow ups patches here:
> > 
> > (1) Let's store the roles set within the Framework struct, so that we 
> > don't have to keep re-computing it and can just write:
> > 
> > ```
> > if (framework->roles.count(role.get()) == 0) {
> >   ...
> > }
> > ```
> > 
> > (2) Add a drop overload to avoid custom logging here:
> > 
> > ```
> > drop(framework,
> >   suppress,
> >  "suppression role ' + role.get() + " is not one"
> >  " of the frameworks's subscribed roles");
> > ```

Added some `TODO` here to follow up later.


- Guangya


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


On 二月 7, 2017, 10:10 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56330/
> ---
> 
> (Updated 二月 7, 2017, 10:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled suppress offer per role.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 71a40537b673e44ecdd5327d9a9f083faa7fc13a 
>   src/master/allocator/mesos/allocator.hpp 
> e3c86181677302dbfc3b213715196122f96e312c 
>   src/master/allocator/mesos/hierarchical.hpp 
> 896abcdf0727f986eef3a1a9304a0e4847094057 
>   src/master/allocator/mesos/hierarchical.cpp 
> 56d6791baa64189523df668749f4a7ab67d6b363 
>   src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
>   src/tests/allocator.hpp 32c291213d18d1c8fe5d9e8194b92c10716b9961 
>   src/tests/hierarchical_allocator_tests.cpp 
> c681d03c3f94f7d071143366a5aad0421108ebec 
> 
> Diff: https://reviews.apache.org/r/56330/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> Will add a test case to enable suppress per role in follow up patches.
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>



Re: Review Request 56330: Enabled suppress offer per role.

2017-02-08 Thread Guangya Liu

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

(Updated 二月 9, 2017, 6:05 a.m.)


Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Enabled suppress offer per role.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
71a40537b673e44ecdd5327d9a9f083faa7fc13a 
  src/master/allocator/mesos/allocator.hpp 
e3c86181677302dbfc3b213715196122f96e312c 
  src/master/allocator/mesos/hierarchical.hpp 
896abcdf0727f986eef3a1a9304a0e4847094057 
  src/master/allocator/mesos/hierarchical.cpp 
017253cc8f5fbcccd9d4057c5b189f352779513c 
  src/master/master.cpp 0b65345d48192a1536d43973cf782ade3c1c8163 
  src/tests/allocator.hpp 32c291213d18d1c8fe5d9e8194b92c10716b9961 
  src/tests/hierarchical_allocator_tests.cpp 
c681d03c3f94f7d071143366a5aad0421108ebec 

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


Testing
---

make
make check

Will add a test case to enable suppress per role in follow up patches.


Thanks,

Guangya Liu



Re: Review Request 51942: Removed scalars() API.

2017-02-07 Thread Guangya Liu

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

(Updated 二月 7, 2017, 3:14 p.m.)


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


Repository: mesos


Description
---

This API is not used anymore because of performance issues and
replaced by `createStrippedScalarQuantity`.


Diffs (updated)
-

  include/mesos/resources.hpp 73e44be8aa03cfd167ff47bbff5d708fb545c01b 
  include/mesos/v1/resources.hpp 19132298cf9f51a60f700ed5ac8c3fbe93e8c40f 
  src/common/resources.cpp 388e3ef3eabea0dd8d1300e56d493b92b70c75e3 
  src/v1/resources.cpp e47c4d49e69b915d37e40a91fef69f75dff52463 

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


Testing
---

make
make check


Thanks,

Guangya Liu



Re: Review Request 56330: Enabled suppress offer per role.

2017-02-07 Thread Guangya Liu


> On 二月 6, 2017, 9:15 p.m., Benjamin Mahler wrote:
> > include/mesos/allocator/allocator.hpp, lines 352-353
> > <https://reviews.apache.org/r/56330/diff/1/?file=1624787#file1624787line352>
> >
> > Hm.. it doesn't look like any of the comments in this header make use 
> > of @param.

Do you mean we do not need `@param`? But there are actually many places using 
such format as 
https://github.com/apache/mesos/blob/master/include/mesos/allocator/allocator.hpp#L76-L86
 , comments?


- Guangya


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


On 二月 7, 2017, 10:10 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56330/
> ---
> 
> (Updated 二月 7, 2017, 10:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled suppress offer per role.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 71a40537b673e44ecdd5327d9a9f083faa7fc13a 
>   src/master/allocator/mesos/allocator.hpp 
> e3c86181677302dbfc3b213715196122f96e312c 
>   src/master/allocator/mesos/hierarchical.hpp 
> 896abcdf0727f986eef3a1a9304a0e4847094057 
>   src/master/allocator/mesos/hierarchical.cpp 
> 56d6791baa64189523df668749f4a7ab67d6b363 
>   src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
>   src/tests/allocator.hpp 32c291213d18d1c8fe5d9e8194b92c10716b9961 
>   src/tests/hierarchical_allocator_tests.cpp 
> c681d03c3f94f7d071143366a5aad0421108ebec 
> 
> Diff: https://reviews.apache.org/r/56330/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> Will add a test case to enable suppress per role in follow up patches.
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>



Review Request 56378: Added test case for suppress and revive with multi role framework.

2017-02-07 Thread Guangya Liu

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

Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Added test case for suppress and revive with multi role framework.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
c681d03c3f94f7d071143366a5aad0421108ebec 

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


Testing
---

make
make check

```
./bin/mesos-tests.sh  
--gtest_filter="HierarchicalAllocatorTest.SuppressAndReviveOffersWithMultiRole" 
--gtest_repeat=100
```


Thanks,

Guangya Liu



Review Request 56371: Enabled `ReviveOffersMessage` support revive per role.

2017-02-07 Thread Guangya Liu

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

Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Enabled `ReviveOffersMessage` support revive per role.


Diffs
-

  src/master/master.hpp ceb9bf7e4be7e0e43a166d7706bd0ab4fd9ed578 
  src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
  src/messages/messages.proto 7a2f37b78a8edcd372558f77f15e6b249742e321 

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


Testing
---

make
make check


Thanks,

Guangya Liu



Review Request 56374: Enabled revive offer per role.

2017-02-07 Thread Guangya Liu

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

Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Enabled revive offer per role.


Diffs
-

  include/mesos/allocator/allocator.hpp 
71a40537b673e44ecdd5327d9a9f083faa7fc13a 
  src/master/allocator/mesos/allocator.hpp 
e3c86181677302dbfc3b213715196122f96e312c 
  src/master/allocator/mesos/hierarchical.hpp 
896abcdf0727f986eef3a1a9304a0e4847094057 
  src/master/allocator/mesos/hierarchical.cpp 
56d6791baa64189523df668749f4a7ab67d6b363 
  src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
  src/tests/allocator.hpp 32c291213d18d1c8fe5d9e8194b92c10716b9961 
  src/tests/hierarchical_allocator_tests.cpp 
c681d03c3f94f7d071143366a5aad0421108ebec 

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


Testing
---

make
make check


Thanks,

Guangya Liu



Review Request 56373: Augmented master `Revive` API to accept `Call::Revive`.

2017-02-07 Thread Guangya Liu

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

Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Augmented master `Revive` API to accept `Call::Revive`.


Diffs
-

  src/master/http.cpp d881ad6dba9ba96057988db265faf0b3013c9b05 
  src/master/master.hpp ceb9bf7e4be7e0e43a166d7706bd0ab4fd9ed578 
  src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 

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


Testing
---

make
make check


Thanks,

Guangya Liu



Re: Review Request 56330: Enabled suppress offer per role.

2017-02-07 Thread Guangya Liu

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

(Updated 二月 7, 2017, 10:10 a.m.)


Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Enabled suppress offer per role.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
71a40537b673e44ecdd5327d9a9f083faa7fc13a 
  src/master/allocator/mesos/allocator.hpp 
e3c86181677302dbfc3b213715196122f96e312c 
  src/master/allocator/mesos/hierarchical.hpp 
896abcdf0727f986eef3a1a9304a0e4847094057 
  src/master/allocator/mesos/hierarchical.cpp 
56d6791baa64189523df668749f4a7ab67d6b363 
  src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
  src/tests/allocator.hpp 32c291213d18d1c8fe5d9e8194b92c10716b9961 
  src/tests/hierarchical_allocator_tests.cpp 
c681d03c3f94f7d071143366a5aad0421108ebec 

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


Testing
---

make
make check

Will add a test case to enable suppress per role in follow up patches.


Thanks,

Guangya Liu



Review Request 56376: Updated allocator test to support create multi role framework.

2017-02-07 Thread Guangya Liu

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

Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Updated allocator test to support create multi role framework.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
c681d03c3f94f7d071143366a5aad0421108ebec 

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


Testing
---

make
make check


Thanks,

Guangya Liu



Re: Review Request 56327: Updated Suppress and Revive proto to support per role.

2017-02-06 Thread Guangya Liu

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

(Updated 二月 7, 2017, 2:08 a.m.)


Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Updated Suppress and Revive proto to support per role.


Diffs (updated)
-

  include/mesos/scheduler/scheduler.proto 
5f4635d523286754a61aa99e18e79d6c1db9463f 
  include/mesos/v1/scheduler/scheduler.proto 
096c76dfffe03c0e2d6abe84d438c396cc1b0be9 

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


Testing
---

make
make check


Thanks,

Guangya Liu



Review Request 56330: Enabled suppress offer per role.

2017-02-06 Thread Guangya Liu

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

Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Enabled suppress offer per role.


Diffs
-

  include/mesos/allocator/allocator.hpp 
71a40537b673e44ecdd5327d9a9f083faa7fc13a 
  src/master/allocator/mesos/allocator.hpp 
e3c86181677302dbfc3b213715196122f96e312c 
  src/master/allocator/mesos/hierarchical.hpp 
896abcdf0727f986eef3a1a9304a0e4847094057 
  src/master/allocator/mesos/hierarchical.cpp 
3429a6591e5041240736dd033acc23253d9942c8 
  src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
  src/tests/allocator.hpp 32c291213d18d1c8fe5d9e8194b92c10716b9961 
  src/tests/hierarchical_allocator_tests.cpp 
c681d03c3f94f7d071143366a5aad0421108ebec 

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


Testing
---

make
make check

Will add a test case to enable suppress per role in follow up patches.


Thanks,

Guangya Liu



Review Request 56328: Augmented master `Suppress` API to accept `Call::Suppress`.

2017-02-06 Thread Guangya Liu

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

Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Augmented master `Suppress` API to accept `Call::Suppress`.


Diffs
-

  src/master/http.cpp d881ad6dba9ba96057988db265faf0b3013c9b05 
  src/master/master.hpp ceb9bf7e4be7e0e43a166d7706bd0ab4fd9ed578 
  src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 

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


Testing
---

make
make check


Thanks,

Guangya Liu



Review Request 56327: Updated Suppress and Revive proto to support per role.

2017-02-06 Thread Guangya Liu

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

Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Updated Suppress and Revive proto to support per role.


Diffs
-

  include/mesos/scheduler/scheduler.proto 
5f4635d523286754a61aa99e18e79d6c1db9463f 
  include/mesos/v1/scheduler/scheduler.proto 
096c76dfffe03c0e2d6abe84d438c396cc1b0be9 

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


Testing
---

make
make check


Thanks,

Guangya Liu



Re: Review Request 56314: Fix the OS X build.

2017-02-04 Thread Guangya Liu

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



Seems the docker_archive.hpp also need some update.

- Guangya Liu


On 二月 5, 2017, 3:47 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56314/
> ---
> 
> (Updated 二月 5, 2017, 3:47 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix the OS X build.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 5dae4e8e186b2cd763672dda1c73378a37c07364 
> 
> Diff: https://reviews.apache.org/r/56314/diff/
> 
> 
> Testing
> ---
> 
> make (on OS X)
> 
> 
> Thanks,
> 
> James Peach
> 
>



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

2017-02-03 Thread Guangya Liu


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

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


- Guangya


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


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



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

2017-02-03 Thread Guangya Liu


> On 一月 30, 2017, 1:46 a.m., Guangya Liu wrote:
> > src/slave/slave.cpp, line 5244
> > <https://reviews.apache.org/r/55971/diff/2/?file=1617172#file1617172line5244>
> >
> > How about include `resources` in the log message?
> 
> Benjamin Mahler wrote:
> Hm.. I'm having a hard time seeing how that would be helpful here, any 
> thoughts?

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


- Guangya


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


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



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

2017-02-03 Thread Guangya Liu


> On 一月 28, 2017, 11:45 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1133-1137
> > <https://reviews.apache.org/r/55870/diff/1/?file=1615405#file1615405line1133>
> >
> > Add `role` here in the log message?
> 
> Benjamin Mahler wrote:
> Hm.. not sure what you mean, the role will be displayed when printing the 
> resources. Can you clarify?

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


- Guangya


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


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



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

2017-01-31 Thread Guangya Liu


> On 二月 1, 2017, 1:06 a.m., Guangya Liu wrote:
> > src/tests/master_allocator_tests.cpp, lines 1751-1752
> > <https://reviews.apache.org/r/55852/diff/1/?file=1612985#file1612985line1751>
> >
> > This is only checking one offer but not two?
> 
> Jiang Yan Xu wrote:
> `framework2offers` is a `Queue`, calling 
> [get()](https://github.com/apache/mesos/blob/d0f4cd4d9e98c27fd03eeafcef6e16bb04f51bc8/3rdparty/libprocess/include/process/queue.hpp#L55)
>  on it repeatedly will pop out the elements.
> 
> Guangya Liu wrote:
> Ah, I see, can you please update the comments a bit here to reflect this, 
> then we can ship it.
> 
> Jiang Yan Xu wrote:
> What would you like to see? How about
> 
> ```
> // All offers for framework2 are enqueued by now.
> ```
> 
> ? Or did you mean a comment about `Queue::get()`? 
> 
> The latter should probably be a comment in `Queue` and not here right?
> 
> Guangya Liu wrote:
> How about 
> 
> ```
> // All offers for framework2 are enqueued by now, the
> // `Queue::get()` will pop out the elements.
> ```
> 
> BTW: I did not saw any comments in `Queue::get()`, so it would be great 
> if we can put some comments here, perhaps also need a follow up patch to 
> update the comments for `Queue::get()`, comments?
> 
> Guangya Liu wrote:
> @Yan, just think more, how about update the function name for `Queue` 
> from `Queue::put` and `Queue::get` to `Queue::push` and `Queue::pop`? My 
> thinking is `get` should be a `readonly` operation.
> 
> Jiang Yan Xu wrote:
> I am reluctant to add comments to explain a particular API. We can 
> certainly improve docuemntation around `Queue`, would you like to file a JIRA?
> 
> As for there, how about I make the code more obvious?
> 
> ```
> for (int i = 0; i < 2; i++) {
>   Future offer = framework2offers.get();
>   
>   // All offers for framework2 are enqueued by now.
>   AWAIT_READY(offer);
>   EXPECT_EQ(Resources(offer->resources()),
> Resources::parse(agentResources).get());
> }
> ```
> 
> Jiang Yan Xu wrote:
> re push/pop, it's a separate discussion but my feeling is that `pop()` 
> doesn't convey the sense that it's an async operation but agreed that perhaps 
> there could be a better name than `get()`.

re update code, yes, it is better to make the code obvious, also file JIRA 
https://issues.apache.org/jira/browse/MESOS-7044 to trace this.


- Guangya


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


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



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

2017-01-31 Thread Guangya Liu


> On 二月 1, 2017, 1:06 a.m., Guangya Liu wrote:
> > src/tests/master_allocator_tests.cpp, lines 1751-1752
> > <https://reviews.apache.org/r/55852/diff/1/?file=1612985#file1612985line1751>
> >
> > This is only checking one offer but not two?
> 
> Jiang Yan Xu wrote:
> `framework2offers` is a `Queue`, calling 
> [get()](https://github.com/apache/mesos/blob/d0f4cd4d9e98c27fd03eeafcef6e16bb04f51bc8/3rdparty/libprocess/include/process/queue.hpp#L55)
>  on it repeatedly will pop out the elements.
> 
> Guangya Liu wrote:
> Ah, I see, can you please update the comments a bit here to reflect this, 
> then we can ship it.
> 
> Jiang Yan Xu wrote:
> What would you like to see? How about
> 
> ```
> // All offers for framework2 are enqueued by now.
> ```
> 
> ? Or did you mean a comment about `Queue::get()`? 
> 
> The latter should probably be a comment in `Queue` and not here right?
> 
> Guangya Liu wrote:
> How about 
> 
> ```
> // All offers for framework2 are enqueued by now, the
> // `Queue::get()` will pop out the elements.
> ```
> 
> BTW: I did not saw any comments in `Queue::get()`, so it would be great 
> if we can put some comments here, perhaps also need a follow up patch to 
> update the comments for `Queue::get()`, comments?

@Yan, just think more, how about update the function name for `Queue` from 
`Queue::put` and `Queue::get` to `Queue::push` and `Queue::pop`? My thinking is 
`get` should be a `readonly` operation.


- Guangya


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


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



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

2017-01-31 Thread Guangya Liu


> On 二月 1, 2017, 1:06 a.m., Guangya Liu wrote:
> > src/tests/master_allocator_tests.cpp, lines 1751-1752
> > <https://reviews.apache.org/r/55852/diff/1/?file=1612985#file1612985line1751>
> >
> > This is only checking one offer but not two?
> 
> Jiang Yan Xu wrote:
> `framework2offers` is a `Queue`, calling 
> [get()](https://github.com/apache/mesos/blob/d0f4cd4d9e98c27fd03eeafcef6e16bb04f51bc8/3rdparty/libprocess/include/process/queue.hpp#L55)
>  on it repeatedly will pop out the elements.
> 
> Guangya Liu wrote:
> Ah, I see, can you please update the comments a bit here to reflect this, 
> then we can ship it.
> 
> Jiang Yan Xu wrote:
> What would you like to see? How about
> 
> ```
> // All offers for framework2 are enqueued by now.
> ```
> 
> ? Or did you mean a comment about `Queue::get()`? 
> 
> The latter should probably be a comment in `Queue` and not here right?

How about 

```
// All offers for framework2 are enqueued by now, the
// `Queue::get()` will pop out the elements.
```

BTW: I did not saw any comments in `Queue::get()`, so it would be great if we 
can put some comments here, perhaps also need a follow up patch to update the 
comments for `Queue::get()`, comments?


- Guangya


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


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



Re: Review Request 55893: Fixed OversubscriptionTest.RescindRevocableOfferWithIncreasedRevocable.

2017-01-31 Thread Guangya Liu

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


Ship it!




Ship It!

- Guangya Liu


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



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

2017-01-31 Thread Guangya Liu


> On 二月 1, 2017, 1:06 a.m., Guangya Liu wrote:
> > src/tests/master_allocator_tests.cpp, lines 1751-1752
> > <https://reviews.apache.org/r/55852/diff/1/?file=1612985#file1612985line1751>
> >
> > This is only checking one offer but not two?
> 
> Jiang Yan Xu wrote:
> `framework2offers` is a `Queue`, calling 
> [get()](https://github.com/apache/mesos/blob/d0f4cd4d9e98c27fd03eeafcef6e16bb04f51bc8/3rdparty/libprocess/include/process/queue.hpp#L55)
>  on it repeatedly will pop out the elements.

Ah, I see, can you please update the comments a bit here to reflect this, then 
we can ship it.


- Guangya


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


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



Re: Review Request 55893: Fixed OversubscriptionTest.RescindRevocableOfferWithIncreasedRevocable.

2017-01-31 Thread Guangya Liu


> On 一月 31, 2017, 10:20 p.m., Jacob Janco wrote:
> > Ship It!
> 
> Guangya Liu wrote:
> @Jacob, does the flaky test fixed with "--gtest_repeat=1000"?
> 
> Jiang Yan Xu wrote:
> Yes. See "Testing Done".

Thanks @Yan, but I did not see any code update after @Jacob's flaky test 
result, am I missed anything?


- Guangya


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


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



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

2017-01-31 Thread Guangya Liu

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




src/tests/master_allocator_tests.cpp (lines 1750 - 1751)
<https://reviews.apache.org/r/55852/#comment235266>

This is only checking one offer but not two?


- Guangya Liu


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



Re: Review Request 55893: Fixed OversubscriptionTest.RescindRevocableOfferWithIncreasedRevocable.

2017-01-31 Thread Guangya Liu


> On 一月 31, 2017, 10:20 p.m., Jacob Janco wrote:
> > Ship It!

@Jacob, does the flaky test fixed with "--gtest_repeat=1000"?


- Guangya


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


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



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

2017-01-29 Thread Guangya Liu

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




src/examples/dynamic_reservation_framework.cpp (line 123)
<https://reviews.apache.org/r/55973/#comment234922>

A question here, why need `apply` resources here? What is the problem is we 
keeps check if the `unreserved` contains `Allocated TASK RESOURCES`?



src/tests/master_allocator_tests.cpp (line 266)
<https://reviews.apache.org/r/55973/#comment234923>

kill this



src/tests/master_allocator_tests.cpp (line 273)
<https://reviews.apache.org/r/55973/#comment234924>

kill this



src/tests/master_tests.cpp (lines 73 - 74)
<https://reviews.apache.org/r/55973/#comment234925>

switch the order here


- Guangya Liu


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



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

2017-01-29 Thread Guangya Liu

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


Fix it, then Ship it!




Ship It!


src/master/master.hpp (lines 2431 - 2440)
<https://reviews.apache.org/r/56004/#comment234917>

Can you please add some comments here to clarify that this is used to 
handle upgrade/downgrade case to/from multi role framework?


- Guangya Liu


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



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

2017-01-29 Thread Guangya Liu

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




src/master/master.cpp (lines 5620 - 5628)
<https://reviews.apache.org/r/55972/#comment234916>

Same comments as /r/55971/ here


- Guangya Liu


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



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

2017-01-29 Thread Guangya Liu

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




src/slave/slave.cpp (lines 1874 - 1878)
<https://reviews.apache.org/r/55971/#comment234888>

Ditto as comments in previous patches, move this to a resources util.



src/slave/slave.cpp (lines 5241 - 5249)
<https://reviews.apache.org/r/55971/#comment234887>

Since we do not support store mix allocated and unallocated resources, how 
about optimize the logic a big as following?

```
if (resource->has_allocation_info()) {
  break;
}

if (roles.size() != 1) {
  LOG(FATAL) << "Missing 'Resource.AllocationInfo' for resources"
 << " allocated to MULTI_ROLE framework"
 << " '" << frameworkInfo.name() << "'";
}

resource->mutable_allocation_info()->set_role(*roles.begin());
```



src/slave/slave.cpp (line 5244)
<https://reviews.apache.org/r/55971/#comment234886>

How about include `resources` in the log message?


- Guangya Liu


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



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

2017-01-28 Thread Guangya Liu

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




src/master/http.cpp (lines 4614 - 4615)
<https://reviews.apache.org/r/55970/#comment234865>

I think this can be replaced by the util function in below comments?



src/master/quota_handler.cpp (lines 196 - 200)
<https://reviews.apache.org/r/55970/#comment234864>

Per the comments in /r/55870/ , how about move this to resources_utils.cpp?



src/master/validation.cpp (lines 1654 - 1667)
<https://reviews.apache.org/r/55970/#comment234867>

Did not quite catch up the `TODO` here, can you please elaborate for why 
want such a function?



src/master/validation.cpp (line 1718)
<https://reviews.apache.org/r/55970/#comment234866>

How about 

```
if (unallocated(resources).contains(volume)) {
```

and then kill #1715 ?


- Guangya Liu


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



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

2017-01-28 Thread Guangya Liu

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




src/master/allocator/mesos/hierarchical.cpp (lines 820 - 824)
<https://reviews.apache.org/r/55870/#comment234862>

I found that this was used in many places for both master and agent, how 
about put this in resources_utils.cpp?


- Guangya Liu


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



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

2017-01-28 Thread Guangya Liu


> On 一月 28, 2017, 11:45 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1176-1177
> > <https://reviews.apache.org/r/55870/diff/1/?file=1615405#file1615405line1176>
> >
> > A question here: Why not call `resources.unallocate()` at #1130? 
> > 
> > This can make the logic of `allocate` and `recoverResources` as pair: 
> > `allocate` will set allocation info while `recoverResources` will clear 
> > allocatio info.

I see, it is enough to only decrese the `allocated` in `recoverResources`.


- Guangya


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


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



Re: Review Request 55968: Added TODOs to make the master code safer.

2017-01-28 Thread Guangya Liu

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


Ship it!




Ship It!

- Guangya Liu


On 一月 26, 2017, 12:55 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55968/
> ---
> 
> (Updated 一月 26, 2017, 12:55 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The executor and tasks fields in the master's structs for agents
> and frameworks are publicly visible, but yet for correctness it
> is required that callers made modifications through the member
> functions which carry additional logic (e.g. tracking allocated
> resources). The TODO here is to make these private and to provide
> public views that are const.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b 
> 
> Diff: https://reviews.apache.org/r/55968/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>



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

2017-01-28 Thread Guangya Liu

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




src/tests/hierarchical_allocator_tests.cpp (line 3933)
<https://reviews.apache.org/r/55967/#comment234859>

Need call `allocation.allocate("*");` here to make sure the agent can be 
added successfully.



src/tests/hierarchical_allocator_tests.cpp (line 3934)
<https://reviews.apache.org/r/55967/#comment234855>

Need call `allocation.allocate("*");` here to make sure the agent can be 
added successfully.



src/tests/hierarchical_allocator_tests.cpp (line 3934)
<https://reviews.apache.org/r/55967/#comment234856>

Need call `allocation.allocate("*");` here to make sure the agent can be 
added successfully.



src/tests/hierarchical_allocator_tests.cpp (line 4084)
<https://reviews.apache.org/r/55967/#comment234858>

Need call `allocation.allocate("*");` here to make sure the agent can be 
added successfully.



src/tests/hierarchical_allocator_tests.cpp (lines 4188 - 4189)
<https://reviews.apache.org/r/55967/#comment234846>

Why moving this here?



src/tests/hierarchical_allocator_tests.cpp (line 4243)
<https://reviews.apache.org/r/55967/#comment234857>

Need call `allocation.allocate("*");` here to make sure the agent can be 
added successfully.


- Guangya Liu


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



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

2017-01-28 Thread Guangya Liu

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




include/mesos/allocator/allocator.hpp (line 91)
<https://reviews.apache.org/r/55870/#comment234820>

I think that we also need to reflect this in CHANGELOG to clarify that this 
interface was updated for multi_role support.



src/master/allocator/mesos/hierarchical.hpp (line 232)
<https://reviews.apache.org/r/55870/#comment234821>

How about update the comments a bit?
```
// Remove an offer filter for the specified role of a framework.
```



src/master/allocator/mesos/hierarchical.hpp (lines 251 - 252)
<https://reviews.apache.org/r/55870/#comment234822>

How about update the comments a bit?
```
// Returns true if there is a resource offer filter for the
// speficied role of the framework on this slave.
```



src/master/allocator/mesos/hierarchical.hpp (line 283)
<https://reviews.apache.org/r/55870/#comment234823>

I think that the reason you want to update here is want to keep consistent, 
right?



src/master/allocator/mesos/hierarchical.cpp (line 428)
<https://reviews.apache.org/r/55870/#comment234824>

s/a new 'role'/new 'roles'



src/master/allocator/mesos/hierarchical.cpp (lines 1124 - 1128)
<https://reviews.apache.org/r/55870/#comment234841>

Add `role` here in the log message?



src/master/allocator/mesos/hierarchical.cpp (lines 1124 - 1129)
<https://reviews.apache.org/r/55870/#comment234842>

Add `role` here in the log message?



src/master/allocator/mesos/hierarchical.cpp (lines 1167 - 1168)
<https://reviews.apache.org/r/55870/#comment234843>

A question here: Why not call `resources.unallocate()` at #1130? 

This can make the logic of `allocate` and `recoverResources` as pair: 
`allocate` will set allocation info while `recoverResources` will clear 
allocatio info.



src/master/allocator/mesos/hierarchical.cpp (lines 1219 - 1220)
<https://reviews.apache.org/r/55870/#comment234836>

Nit, how about update the message a bit:

```
Suppressed offers for roles {r1,r2} in framework xx-xx-xx-xx
```

Ditto for the log message in `reviveOffers`.



src/master/allocator/mesos/hierarchical.cpp (line 1440)
<https://reviews.apache.org/r/55870/#comment234837>

s/role/roles



src/master/allocator/mesos/hierarchical.cpp (line 1480)
<https://reviews.apache.org/r/55870/#comment234838>

Why highlight the `unallocate()` here, I think that this was already done 
via `Resources::createStrippedScalarQuantity` and the resources being `flatten` 
here should not have `allocation info`.



src/master/allocator/mesos/hierarchical.cpp (lines 2044 - 2046)
<https://reviews.apache.org/r/55870/#comment234839>

Adding `role` in the log message here to clarify which role on this 
framework is being filtered?



src/master/allocator/mesos/hierarchical.cpp (lines 2044 - 2046)
<https://reviews.apache.org/r/55870/#comment234840>

Add `role` in the log message here to clarify which role on this framework 
is being filtered?


- Guangya Liu


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

Re: Review Request 55863: Introduce a helper for injecting AllocationInfo into offer operations.

2017-01-28 Thread Guangya Liu

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




src/common/protobuf_utils.cpp (lines 345 - 347)
<https://reviews.apache.org/r/55863/#comment234834>

Since we do not support one `Resources` store a mix of allocated and 
unallocated resources, how about optimize this a bit as:

```
if (resource->has_allocation_info()) {
  break;
}

resource->mutable_allocation_info()->CopyFrom(allocationInfo);
```

Ditto for here and everywhere



src/tests/protobuf_utils_tests.cpp (lines 101 - 103)
<https://reviews.apache.org/r/55863/#comment234835>

How about make this less jagged

```
  // Test the LAUNCH_GROUP case. This should be constructing a valid
  // task and executor, but for now this just sets the resources in
  // order to verify the allocation info injection.
```

Ditto here and following comments.


- Guangya Liu


On 一月 23, 2017, 10:59 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55863/
> ---
> 
> (Updated 一月 23, 2017, 10:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-6967
> https://issues.apache.org/jira/browse/MESOS-6967
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, `Resource` did not contain `AllocationInfo`. So for
> backwards compatibility with old schedulers and tooling, we must
> allow operations to contain `Resource`s without an `AllocationInfo`.
> 
> This introduces a function which allows the master to inject the
> offer's `AllocationInfo` into the operation's resources.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp faa7e2a759fd2b1ce5def662679f573ec6fefd28 
>   src/common/protobuf_utils.cpp dd20759affe3adf2867a33bf875c9ee82862038d 
>   src/tests/protobuf_utils_tests.cpp bc2a3d0ebe544a58d0de8f2f7174c170113ddf2e 
> 
> Diff: https://reviews.apache.org/r/55863/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>



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

2017-01-27 Thread Guangya Liu

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


Fix it, then Ship it!




Ship It!


src/common/resources.cpp (lines 1274 - 1276)
<https://reviews.apache.org/r/55828/#comment234819>

This indicates that if the `Resource` do not have `AllocationInfo`, then we 
will loop all of the `Resource` till finished then return false, but we have 
the assumption of no `Resources` store a mix of allocated and unallocated 
resources, so how about `return resource.has_allocation_info();` here?



src/tests/resources_utils.hpp (line 27)
<https://reviews.apache.org/r/55828/#comment234690>

I think that we do not like `using xxx` in a header file.



src/tests/resources_utils.hpp (line 33)
<https://reviews.apache.org/r/55828/#comment234691>

I think that we can kill the keyword `TODO` here?


- Guangya Liu


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



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

2017-01-26 Thread Guangya Liu

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


Fix it, then Ship it!




Ship It!


src/master/allocator/mesos/hierarchical.cpp (lines 237 - 241)
<https://reviews.apache.org/r/55910/#comment234689>

Just a nit here: It seems a bit strange to me here of getting roles as 
following:
1) Construct `frameworks`
2) Get related framework info
3) Get related framework role

How about keep the logic as before but put the logic of construct of 
`frameworks` to #275 here?


- Guangya Liu


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



Re: Review Request 56005: Added a safety CHECK when accessing activeRoles in the master.

2017-01-26 Thread Guangya Liu

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


Ship it!




Ship It!

- Guangya Liu


On 一月 27, 2017, 12:31 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56005/
> ---
> 
> (Updated 一月 27, 2017, 12:31 a.m.)
> 
> 
> Review request for mesos, Michael Park and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently there is a use of the [] operator that is not preceded
> by a CHECK that the key is contained in the map. This leads to a
> hard to diagnose issue if there are bugs that violate this
> assumption.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 0f2c7cd32161576fad742ef350dff64874b80854 
> 
> Diff: https://reviews.apache.org/r/56005/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>



Re: Review Request 56006: Added CHECK logging to the allocator.

2017-01-26 Thread Guangya Liu

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




src/master/allocator/mesos/hierarchical.cpp (line 432)
<https://reviews.apache.org/r/56006/#comment234686>

How about make the log messsage more clear such as 

```
CHECK(framework.roles == newRoles)
  << stringify(framework.roles) << " does not match " << 
stringify(newRoles);

```


- Guangya Liu


On 一月 27, 2017, 12:32 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56006/
> ---
> 
> (Updated 一月 27, 2017, 12:32 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added CHECK logging to the allocator.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> c2211be7458755aeb91ef078e4bfe92ac474044a 
> 
> Diff: https://reviews.apache.org/r/56006/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>



Re: Review Request 55893: Fixed OversubscriptionTest.RescindRevocableOfferWithIncreasedRevocable.

2017-01-26 Thread Guangya Liu

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



I think that your patch need depend on /r/51027 , also in `Testing Done` 
section, can you please make the test repeat or use `--gtest_repeat=-1 
--gtest_break_on_failure` to make sure the test is not flaky?


src/tests/oversubscription_tests.cpp (line 507)
<https://reviews.apache.org/r/55893/#comment234561>

How about s/offer1/offer as here you only have one such variable.



src/tests/oversubscription_tests.cpp (line 543)
<https://reviews.apache.org/r/55893/#comment234562>

How about following?

```
// The latest resource estimate should match the total offered resources.
```

This can also make the comments match `resources2 and resources3`.


- Guangya Liu


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



Re: Review Request 55893: Fixed OversubscriptionTest.RescindRevocableOfferWithIncreasedRevocable.

2017-01-26 Thread Guangya Liu


> On 一月 25, 2017, 9:55 a.m., Guangya Liu wrote:
> > @Yan, I posted some comments at https://reviews.apache.org/r/51027/ for 
> > this issue with some comments as:
> > 
> > ```
> > Jacob, regaring the test failure of 
> > OversubscriptionTest.RescindRevocableOfferWithIncreasedRevocable, I think 
> > that you can disable it as 
> > DISABLED_RescindRevocableOfferWithIncreasedRevocable in your patch, and I 
> > will fix this in /r/51621
> > 
> > I found that with current logic, it is difficult to handle this case, as we 
> > cannot make sure the complete order for allocator->updateSlave and 
> > allocator->recoverResources, if allocator->recoverResources finished first, 
> > then the offer will have 3 REV resources; if allocator->updateSlave 
> > finished first, then the offer will have 2 REV resources.
> > 
> > With /r/51621 , after we enable recoverResources allocate resources, we can 
> > always make sure we get 3 REV resources in the offer, so I propose that you 
> > disable the test case first and I will fix this after we enable allocate 
> > resources when recoverResoures.
> > ```
> > 
> > As I will have a patch for `allocate resources when recoverResoures`, so 
> > how about `disable` this test first and then I can fix this when my patch 
> > ready in https://reviews.apache.org/r/51621/ ?
> 
> Jiang Yan Xu wrote:
> Hey Guangya I took a look at your fix in /r/51621/ but had a question, 
> when `recoverResources()` also triggers allocations. it's true that we don't 
> need `Clock::advance` to trigger a periodic allocation anymore but the race 
> still exists, i.e., you don't know how many `Offers` events the scheduler is 
> going to get, right? With my fix you won't need to care about the number of 
> events since you really only care about the number/amount of offers (you 
> eventually get). So I think this fix will work with your patch as well?

@Yan, you are right. Your fix can also help `allocate when recoverResources`, 
my fix in /r/51621/ is still flaky for this. Thanks.


- Guangya


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


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



Re: Review Request 55909: Added a function for available agent resources in the allocator.

2017-01-25 Thread Guangya Liu

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


Ship it!




Ship It!

- Guangya Liu


On 一月 25, 2017, 2:31 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55909/
> ---
> 
> (Updated 一月 25, 2017, 2:31 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 9b66c23f26b37c02ed850c06c4518ea99078b02d 
>   src/master/allocator/mesos/hierarchical.cpp 
> c2211be7458755aeb91ef078e4bfe92ac474044a 
> 
> Diff: https://reviews.apache.org/r/55909/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>



Re: Review Request 55908: Made a function static in the allocator.

2017-01-25 Thread Guangya Liu

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


Ship it!




Ship It!

- Guangya Liu


On 一月 25, 2017, 2:25 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55908/
> ---
> 
> (Updated 一月 25, 2017, 2:25 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 9b66c23f26b37c02ed850c06c4518ea99078b02d 
> 
> Diff: https://reviews.apache.org/r/55908/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>



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

2017-01-25 Thread Guangya Liu

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




src/tests/hierarchical_allocator_tests.cpp (line 1870)
<https://reviews.apache.org/r/55868/#comment234549>

Any reason you want to update here?


- Guangya Liu


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



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

2017-01-25 Thread Guangya Liu

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




src/tests/hierarchical_allocator_tests.cpp 
<https://reviews.apache.org/r/55868/#comment234528>

I saw that you removed all of this `ASSERT_EQ` in the following tests, any 
reason you want do this?



src/tests/hierarchical_allocator_tests.cpp (line 1072)
<https://reviews.apache.org/r/55868/#comment234530>

How about move this right before `unreserved` under #1089?



src/tests/hierarchical_allocator_tests.cpp (lines 1315 - 1316)
<https://reviews.apache.org/r/55868/#comment234540>

How about udpate the comments here as well to hightlight `update`?

```
// Now recover the amount of `update` resources which is equal to
// `slave.resources()` and expect the next allocation equal to
// `slave.resources()`.
```



src/tests/hierarchical_allocator_tests.cpp (line 1316)
<https://reviews.apache.org/r/55868/#comment234539>

s/to equal/equal to



src/tests/hierarchical_allocator_tests.cpp (lines 1436 - 1438)
<https://reviews.apache.org/r/55868/#comment234541>

How about move this right before the `update` used right before #1447 as 
before?



src/tests/hierarchical_allocator_tests.cpp (line 1543)
<https://reviews.apache.org/r/55868/#comment234543>

Why not put this right before #1507 and use `allocation` for #1511 and 
#1522 as

```
AWAIT_EXPECT_EQ(expected, allocation);
```

Ditto for the following places.



src/tests/hierarchical_allocator_tests.cpp (line 4262)
<https://reviews.apache.org/r/55868/#comment234547>

s/allocations/offers


- Guangya Liu


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



Re: Review Request 55824: Updated hierarchical allocator tests to use -> operator.

2017-01-25 Thread Guangya Liu

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


Ship it!




I doubled checked that all of `.get().` has been replaced, but it would be 
great if we can put this clean up at the front of this patch chain.

- Guangya Liu


On 一月 24, 2017, 2:29 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55824/
> ---
> 
> (Updated 一月 24, 2017, 2:29 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5 
> 
> Diff: https://reviews.apache.org/r/55824/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>



Re: Review Request 55866: Added CHECK failure logging to the sorter.

2017-01-25 Thread Guangya Liu

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


Ship it!




Ship It!

- Guangya Liu


On 一月 24, 2017, 12:28 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55866/
> ---
> 
> (Updated 一月 24, 2017, 12:28 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 4ff153963229f2ecc73e05ba80f895c23e6a870e 
> 
> Diff: https://reviews.apache.org/r/55866/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>



Re: Review Request 55829: Updated resources quantity stripping to strip AllocationInfo.

2017-01-25 Thread Guangya Liu

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


Ship it!




Ship It!

- Guangya Liu


On 一月 23, 2017, 2:09 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55829/
> ---
> 
> (Updated 一月 23, 2017, 2:09 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-6965
> https://issues.apache.org/jira/browse/MESOS-6965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `Resources::createStrippedScalarQuantity()` attempts to
> create a notion of a "quantity" of resources. In order to do this,
> all distinguishing metadata between resources of the same name are
> stripped. This currently includes, disk metadata, reservation
> metadata (only for dynamic reservations), and shared resource
> metadata. To maintain the notion of a quantity, this patch also
> strips the allocation metadata.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412 
>   include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1 
>   src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45 
>   src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68 
>   src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267 
> 
> Diff: https://reviews.apache.org/r/55829/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>



Re: Review Request 54836: Added helpers to allocate / unallocate Resources.

2017-01-25 Thread Guangya Liu


> On 一月 25, 2017, 9:54 a.m., Michael Park wrote:
> > include/mesos/resources.hpp, lines 332-339
> > <https://reviews.apache.org/r/54836/diff/3/?file=1611725#file1611725line332>
> >
> > I would've expected:
> > ```cpp
> > void allocate(const std::string& role);
> > void unallocate();
> > ```
> > 
> > Could you help me understand what the `bool` from `unallocate` is for?
> > 
> > If it's simply to indicate whether the function had any effects or
> > not, `allocate` could also return a `bool`, which returns `false`
> > when the resources were already allocated for the specified role.
> 
> Guangya Liu wrote:
> For `allocate`, seems the question here is same as mine above: In which 
> case shall we need to overwrite the role for allocation?

@mpark, I think that Ben already posted some comments here for why return 
`bool` for `unallocate` at https://reviews.apache.org/r/54836/#comment230598


- Guangya


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


On 一月 23, 2017, 1:55 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54836/
> -----------
> 
> (Updated 一月 23, 2017, 1:55 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helpers to allocate / unallocate Resources.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412 
>   include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1 
>   src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45 
>   src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68 
>   src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267 
> 
> Diff: https://reviews.apache.org/r/54836/diff/
> 
> 
> Testing
> ---
> 
> Updated the existing allocation test to incorporate the new helper.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>



Re: Review Request 55825: Augmented printing of Resources to include AllocationInfo.

2017-01-25 Thread Guangya Liu

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




src/tests/resources_tests.cpp (line 822)
<https://reviews.apache.org/r/55825/#comment234330>

How about not clear this but keep the `allocated` label for all of the test 
or just add this to the end of this test after line 883? This can convine us 
that the `allocated` label works for all kind of resources.


- Guangya Liu


On 一月 23, 2017, 2:04 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55825/
> ---
> 
> (Updated 一月 23, 2017, 2:04 a.m.)
> 
> 
> Review request for mesos, Jay Guo, Guangya Liu, and Michael Park.
> 
> 
> Bugs: MESOS-6970
> https://issues.apache.org/jira/browse/MESOS-6970
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignoring whether there is a disk info, the format is as follows:
> name(reservedrole[, principal[, labels]])(allocatedrole):value
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45 
>   src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68 
>   src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267 
> 
> Diff: https://reviews.apache.org/r/55825/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>



Re: Review Request 54836: Added helpers to allocate / unallocate Resources.

2017-01-25 Thread Guangya Liu

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




src/tests/resources_tests.cpp (line 2759)
<https://reviews.apache.org/r/54836/#comment234329>

How about 

```
Resources cpus2 = cpus1;
```

Ditto for others.


- Guangya Liu


On 一月 23, 2017, 1:55 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54836/
> ---
> 
> (Updated 一月 23, 2017, 1:55 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helpers to allocate / unallocate Resources.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412 
>   include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1 
>   src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45 
>   src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68 
>   src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267 
> 
> Diff: https://reviews.apache.org/r/54836/diff/
> 
> 
> Testing
> ---
> 
> Updated the existing allocation test to incorporate the new helper.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>



Re: Review Request 54836: Added helpers to allocate / unallocate Resources.

2017-01-25 Thread Guangya Liu


> On 一月 25, 2017, 9:54 a.m., Michael Park wrote:
> > include/mesos/resources.hpp, lines 332-339
> > <https://reviews.apache.org/r/54836/diff/3/?file=1611725#file1611725line332>
> >
> > I would've expected:
> > ```cpp
> > void allocate(const std::string& role);
> > void unallocate();
> > ```
> > 
> > Could you help me understand what the `bool` from `unallocate` is for?
> > 
> > If it's simply to indicate whether the function had any effects or
> > not, `allocate` could also return a `bool`, which returns `false`
> > when the resources were already allocated for the specified role.

For `allocate`, seems the question here is same as mine above: In which case 
shall we need to overwrite the role for allocation?


- Guangya


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


On 一月 23, 2017, 1:55 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54836/
> -----------
> 
> (Updated 一月 23, 2017, 1:55 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helpers to allocate / unallocate Resources.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412 
>   include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1 
>   src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45 
>   src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68 
>   src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267 
> 
> Diff: https://reviews.apache.org/r/54836/diff/
> 
> 
> Testing
> ---
> 
> Updated the existing allocation test to incorporate the new helper.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>



Re: Review Request 54836: Added helpers to allocate / unallocate Resources.

2017-01-25 Thread Guangya Liu


> On 十二月 18, 2016, 1:10 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 1062
> > <https://reviews.apache.org/r/54836/diff/1/?file=1588580#file1588580line1062>
> >
> > Shall we add a `CHECK` here to make sure this resource was not 
> > allocated to any role?
> 
> Benjamin Mahler wrote:
> We could, the alternative that I went for was to have callers (the 
> allocator) CHECK this invariant if they care about it, since it seems 
> specific to the way the allocator works.
> 
> Guangya Liu wrote:
> I saw that you have updated the comments of `allocate` by allowing 
> existing allocation info be overwitten by a new role, why do we allow this? 
> Which secnario need this operation?
> 
> Benjamin Mahler wrote:
> It's not clear if we have a scenario that needs the ability to overwrite, 
> it's just the behavior I would intuit, much like set_role overwrites if there 
> is a role already set.

Still have one question: For which case shall we need to overwrite the role? 
Ideally, the role info should be cleared when `recoverResources`, so if there 
are still roles when `allocate`, we should report an error?


- Guangya


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


On 一月 23, 2017, 1:55 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54836/
> ---
> 
> (Updated 一月 23, 2017, 1:55 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helpers to allocate / unallocate Resources.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412 
>   include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1 
>   src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45 
>   src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68 
>   src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267 
> 
> Diff: https://reviews.apache.org/r/54836/diff/
> 
> 
> Testing
> ---
> 
> Updated the existing allocation test to incorporate the new helper.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>



Re: Review Request 55893: Fixed OversubscriptionTest.RescindRevocableOfferWithIncreasedRevocable.

2017-01-25 Thread Guangya Liu

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



@Yan, I posted some comments at https://reviews.apache.org/r/51027/ for this 
issue with some comments as:

```
Jacob, regaring the test failure of 
OversubscriptionTest.RescindRevocableOfferWithIncreasedRevocable, I think that 
you can disable it as DISABLED_RescindRevocableOfferWithIncreasedRevocable in 
your patch, and I will fix this in /r/51621

I found that with current logic, it is difficult to handle this case, as we 
cannot make sure the complete order for allocator->updateSlave and 
allocator->recoverResources, if allocator->recoverResources finished first, 
then the offer will have 3 REV resources; if allocator->updateSlave finished 
first, then the offer will have 2 REV resources.

With /r/51621 , after we enable recoverResources allocate resources, we can 
always make sure we get 3 REV resources in the offer, so I propose that you 
disable the test case first and I will fix this after we enable allocate 
resources when recoverResoures.
```

As I will have a patch for `allocate resources when recoverResoures`, so how 
about `disable` this test first and then I can fix this when my patch ready in 
https://reviews.apache.org/r/51621/ ?

- Guangya Liu


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



Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

2017-01-14 Thread Guangya Liu

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




src/master/master.cpp (lines 2473 - 2506)
<https://reviews.apache.org/r/55271/#comment232920>

Can you move this right after 2448 to group the logic related with role?



src/master/master.cpp (line 2490)
<https://reviews.apache.org/r/55271/#comment232921>

I think that you may want to 
```
#include 
```



src/tests/master_validation_tests.cpp (line 2616)
<https://reviews.apache.org/r/55271/#comment232922>

How about s/RejectRolesChange/RejectRolesChangeWithMutiRole


- Guangya Liu


On 一月 12, 2017, 3:32 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> ---
> 
> (Updated 一月 12, 2017, 3:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
> https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
>   src/tests/master_validation_tests.cpp 
> e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> ---
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 55381: Added test for framework upgrading to multi-role capability.

2017-01-12 Thread Guangya Liu

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




src/tests/master_validation_tests.cpp (line 2559)
<https://reviews.apache.org/r/55381/#comment232613>

s/UpgradeToMultirole/UpgradeToMultiRole


- Guangya Liu


On 一月 11, 2017, 10:37 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55381/
> ---
> 
> (Updated 一月 11, 2017, 10:37 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6900
> https://issues.apache.org/jira/browse/MESOS-6900
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_validation_tests.cpp 
> e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55381/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 55381: Added test for framework upgrading to multi-role capability.

2017-01-12 Thread Guangya Liu

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


Fix it, then Ship it!




Ship It!


src/tests/master_validation_tests.cpp (lines 2593 - 2596)
<https://reviews.apache.org/r/55381/#comment232608>

How about following to avoid a new line between for `add_capabilities`?

```
  frameworkInfo.add_roles(frameworkInfo.role());
  frameworkInfo.clear_role();
  frameworkInfo.add_capabilities()->set_type(
  FrameworkInfo::Capability::MULTI_ROLE);
```



src/tests/master_validation_tests.cpp (lines 2604 - 2605)
<https://reviews.apache.org/r/55381/#comment232606>

new line here


- Guangya Liu


On 一月 11, 2017, 10:37 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55381/
> ---
> 
> (Updated 一月 11, 2017, 10:37 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6900
> https://issues.apache.org/jira/browse/MESOS-6900
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_validation_tests.cpp 
> e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55381/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 55445: Augmented a fault_tolerance_test to cover update of role.

2017-01-12 Thread Guangya Liu

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


Ship it!




It would be great to add sth in `Testing Done` section to verify that the test 
cases you updated still works well.

- Guangya Liu


On 一月 12, 2017, 8:56 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55445/
> ---
> 
> (Updated 一月 12, 2017, 8:56 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In currently implementation, update of role during framework failover
> is ignored. This behavior should be reflected in test.
> 
> 
> Diffs
> -
> 
>   src/tests/fault_tolerance_tests.cpp 
> 05937a917a2c175aa53b52488febb7cfd8400a13 
> 
> Diff: https://reviews.apache.org/r/55445/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jay Guo
> 
>



Re: Review Request 54926: Augmented a fault_tolerance_test to cover update of role.

2017-01-12 Thread Guangya Liu

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



Can you please abandon this? Seems duplicate with 
https://reviews.apache.org/r/55445/

- Guangya Liu


On 十二月 21, 2016, 3:06 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54926/
> ---
> 
> (Updated 十二月 21, 2016, 3:06 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In currently implementation, update of role during framework
> failover is ignored. This behavior should be reflected in test.
> 
> 
> Diffs
> -
> 
>   src/tests/fault_tolerance_tests.cpp 
> 05937a917a2c175aa53b52488febb7cfd8400a13 
> 
> Diff: https://reviews.apache.org/r/54926/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>



Re: Review Request 54926: Augmented a fault_tolerance_test to cover update of role.

2017-01-12 Thread Guangya Liu

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


Ship it!




Ship It!

- Guangya Liu


On 十二月 21, 2016, 3:06 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54926/
> ---
> 
> (Updated 十二月 21, 2016, 3:06 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In currently implementation, update of role during framework
> failover is ignored. This behavior should be reflected in test.
> 
> 
> Diffs
> -
> 
>   src/tests/fault_tolerance_tests.cpp 
> 05937a917a2c175aa53b52488febb7cfd8400a13 
> 
> Diff: https://reviews.apache.org/r/54926/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>



Re: Review Request 54365: Fixed indentation of a function argument in master.cpp.

2017-01-09 Thread Guangya Liu


> On 一月 10, 2017, 7:34 a.m., Guangya Liu wrote:
> > Ship It!

I have committed this already, but seems do not have permission to close this 
review, will close this when got permission.

commit 653fe55b3f2e6cd76567945dbbec4a84c03f13c2
Author: Jay Guo <guojiannan1...@gmail.com>
Date:   Tue Jan 10 15:32:05 2017 +0800

Fixed indentation of a function argument in master.cpp.

Fixed indentation of a function argument in master.cpp.

Review: https://reviews.apache.org/r/54365/


- Guangya


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


On 十二月 9, 2016, 9:38 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54365/
> ---
> 
> (Updated 十二月 9, 2016, 9:38 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed indentation of a function argument in master.cpp.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 353e6ea802e197b4456c1647f78d9984a50f1c9d 
> 
> Diff: https://reviews.apache.org/r/54365/diff/
> 
> 
> Testing
> ---
> 
> line 3902 should be aligned with other arguments.
> 
> 
> Thanks,
> 
> Jay Guo
> 
>



Re: Review Request 54365: Fixed indentation of a function argument in master.cpp.

2017-01-09 Thread Guangya Liu

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


Ship it!




Ship It!

- Guangya Liu


On 十二月 9, 2016, 9:38 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54365/
> ---
> 
> (Updated 十二月 9, 2016, 9:38 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed indentation of a function argument in master.cpp.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 353e6ea802e197b4456c1647f78d9984a50f1c9d 
> 
> Diff: https://reviews.apache.org/r/54365/diff/
> 
> 
> Testing
> ---
> 
> line 3902 should be aligned with other arguments.
> 
> 
> Thanks,
> 
> Jay Guo
> 
>



Re: Review Request 54898: Added a CHECK in updateFrameworkInfo.

2017-01-07 Thread Guangya Liu

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


Ship it!




Ship It!

- Guangya Liu


On 十二月 20, 2016, 3:06 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54898/
> ---
> 
> (Updated 十二月 20, 2016, 3:06 p.m.)
> 
> 
> Review request for mesos, Guangya Liu and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a CHECK in updateFrameworkInfo.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 89b3c394b268a8645885412aeb19980db8d73c69 
> 
> Diff: https://reviews.apache.org/r/54898/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>



Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

2017-01-07 Thread Guangya Liu

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



You may also want a test case in 
https://github.com/apache/mesos/blob/master/src/tests/master_validation_tests.cpp


src/master/master.cpp (lines 2400 - 2418)
<https://reviews.apache.org/r/55271/#comment232041>

I think that you may also want to apply this to 
https://github.com/apache/mesos/blob/master/src/master/master.cpp#L2535 

```
void Master::subscribe(
const UPID& from,
const scheduler::Call::Subscribe& subscribe)
```


- Guangya Liu


On 一月 6, 2017, 7:07 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> ---
> 
> (Updated 一月 6, 2017, 7:07 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
> https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 11c34a048586d30c6ac67be8638ed8fa81cc3f1f 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> ---
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 52738: Used `inspectImage` to get pulled image.

2017-01-07 Thread Guangya Liu

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

(Updated 一月 7, 2017, 11:51 a.m.)


Review request for mesos, Benjamin Mahler and Kevin Klues.


Repository: mesos


Description
---

After image pull finished, use `inspectImage` to get
the returned image value.


Diffs
-

  src/docker/docker.cpp 472cb1b4dc2b0ac65721c732fca8ec70a7470f47 

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


Testing (updated)
---

make
make check

Also test with a real cases.

1) Remove busybox:latest 
2) Launch a task using busybox:latest , succeed.
3) Check agent log, it is using `inspectImage` to inspect the pulled image as 
`I0107 19:21:33.227638 19845 docker.cpp:1219] Running docker -H 
unix:///var/run/docker.sock inspect busybox:latest`.
```
I0107 19:20:46.989816 19851 docker.cpp:1126] Starting container 
'fa2bff2b-2c48-4afd-842d-cc742bc44afd' for task 'test_mesos' (and executor 
'test_mesos') of framework 3ba2d123-524c-4d44-b6cf-5a440c3286be-
I0107 19:20:46.991299 19846 fetcher.cpp:349] Starting to fetch URIs for 
container: fa2bff2b-2c48-4afd-842d-cc742bc44afd, directory: 
/tmp/mesos/slave/slaves/3ba2d123-524c-4d44-b6cf-5a440c3286be-S0/frameworks/3ba2d123-524c-4d44-b6cf-5a440c3286be-/executors/test_mesos/runs/fa2bff2b-2c48-4afd-842d-cc742bc44afd
I0107 19:20:46.993584 19846 docker.cpp:1519] Running docker -H 
unix:///var/run/docker.sock inspect busybox:latest
I0107 19:20:47.033046 19850 docker.cpp:1593] Running docker -H 
unix:///var/run/docker.sock pull busybox:latest
I0107 19:20:55.868794 19848 slave.cpp:5589] Querying resource estimator for 
oversubscribable resources
I0107 19:20:55.869504 19848 slave.cpp:5603] Received oversubscribable resources 
{} from the resource estimator
I0107 19:20:56.662844 19846 slave.cpp:4275] Received ping from 
slave-observer(1)@192.168.56.12:5050
I0107 19:21:10.871165 19850 slave.cpp:5589] Querying resource estimator for 
oversubscribable resources
I0107 19:21:10.871886 19850 slave.cpp:5603] Received oversubscribable resources 
{} from the resource estimator
I0107 19:21:11.664295 19849 slave.cpp:4275] Received ping from 
slave-observer(1)@192.168.56.12:5050
I0107 19:21:25.873539 19848 slave.cpp:5589] Querying resource estimator for 
oversubscribable resources
I0107 19:21:25.873878 19848 slave.cpp:5603] Received oversubscribable resources 
{} from the resource estimator
I0107 19:21:26.665603 19845 slave.cpp:4275] Received ping from 
slave-observer(1)@192.168.56.12:5050
I0107 19:21:33.227638 19845 docker.cpp:1219] Running docker -H 
unix:///var/run/docker.sock inspect busybox:latest
I0107 19:21:33.335652 19852 docker.cpp:445] Docker pull busybox:latest completed
I0107 19:21:33.337988 19852 docker.cpp:1480] Launching 'mesos-docker-executor' 
with flags 
'--container="mesos-3ba2d123-524c-4d44-b6cf-5a440c3286be-S0.fa2bff2b-2c48-4afd-842d-cc742bc44afd"
 --docker="docker" --docker_socket="/var/run/docker.sock" --help="false" 
--initialize_driver_logging="true" 
--launcher_dir="/root/src/mesos/m1/mesos/build/src" --logbufsecs="0" 
--logging_level="INFO" --mapped_directory="/mnt/mesos/sandbox" --quiet="false" 
--sandbox_directory="/tmp/mesos/slave/slaves/3ba2d123-524c-4d44-b6cf-5a440c3286be-S0/frameworks/3ba2d123-524c-4d44-b6cf-5a440c3286be-/executors/test_mesos/runs/fa2bff2b-2c48-4afd-842d-cc742bc44afd"
 --stop_timeout="0ns"'
I0107 19:21:33.402930 19850 slave.cpp:3320] Got registration for executor 
'test_mesos' of framework 3ba2d123-524c-4d44-b6cf-5a440c3286be- from 
executor(1)@192.168.56.12:46096
I0107 19:21:33.404538 19850 docker.cpp:1574] Ignoring updating container 
fa2bff2b-2c48-4afd-842d-cc742bc44afd because resources passed to update are 
identical to existing resources
I0107 19:21:33.405146 19850 slave.cpp:2265] Sending queued task 'test_mesos' to 
executor 'test_mesos' of framework 3ba2d123-524c-4d44-b6cf-5a440c3286be- at 
executor(1)@192.168.56.12:46096
I0107 19:21:34.016574 19847 slave.cpp:3752] Handling status update TASK_RUNNING 
(UUID: c606b507-14e8-41e8-a1d4-b5f34db4fda1) for task test_mesos of framework 
3ba2d123-524c-4d44-b6cf-5a440c3286be- from executor(1)@192.168.56.12:46096
I0107 19:21:34.018025 19846 status_update_manager.cpp:323] Received status 
update TASK_RUNNING (UUID: c606b507-14e8-41e8-a1d4-b5f34db4fda1) for task 
test_mesos of framework 3ba2d123-524c-4d44-b6cf-5a440c3286be-
```


Thanks,

Guangya Liu



Re: Review Request 52666: Added support for `docker inspect image` in docker containerizer.

2017-01-07 Thread Guangya Liu

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

(Updated 一月 7, 2017, 11:47 a.m.)


Review request for mesos, Benjamin Mahler and Kevin Klues.


Changes
---

Rebase


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


Repository: mesos


Description
---

Added support for `docker inspect image` in docker containerizer.


Diffs (updated)
-

  src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
  src/docker/docker.cpp 472cb1b4dc2b0ac65721c732fca8ec70a7470f47 
  src/tests/containerizer/docker_tests.cpp 
9667d434486c1832f180a297a39a3d5dae6a26bd 

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


Testing
---

make
make check

```
./bin/mesos-tests.sh --gtest_filter="DockerTest.ROOT_DOCKER_Inspect*"
[==] Running 2 tests from 1 test case.
[--] Global test environment set-up.
[--] 2 tests from DockerTest
[ RUN  ] DockerTest.ROOT_DOCKER_InspectExistImage
[   OK ] DockerTest.ROOT_DOCKER_InspectExistImage (6769 ms)
[ RUN  ] DockerTest.ROOT_DOCKER_InspectNonExistImage
[   OK ] DockerTest.ROOT_DOCKER_InspectNonExistImage (314 ms)
[--] 2 tests from DockerTest (7083 ms total)

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


Thanks,

Guangya Liu



Re: Review Request 52727: Added `Labels` to docker image.

2017-01-07 Thread Guangya Liu

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

(Updated 一月 7, 2017, 11:33 a.m.)


Review request for mesos, Benjamin Mahler and Kevin Klues.


Changes
---

rebase


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


Repository: mesos


Description
---

Added `Labels` to docker image.


Diffs (updated)
-

  src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
  src/docker/docker.cpp 472cb1b4dc2b0ac65721c732fca8ec70a7470f47 
  src/tests/containerizer/docker_tests.cpp 
9667d434486c1832f180a297a39a3d5dae6a26bd 

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


Testing
---

make
make check

```
./bin/mesos-tests.sh  --gtest_filter="DockerImageTest.ParseInspectonImage" 
--verbose
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from DockerImageTest
[ RUN  ] DockerImageTest.ParseInspectonImage
[   OK ] DockerImageTest.ParseInspectonImage (4 ms)
[--] 1 test from DockerImageTest (4 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (21 ms total)
[  PASSED  ] 1 test.
```


Thanks,

Guangya Liu



Re: Review Request 52680: Used full docker image name to force pull a iamge.

2017-01-07 Thread Guangya Liu

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

(Updated 一月 7, 2017, 11:29 a.m.)


Review request for mesos, Jie Yu and Timothy Chen.


Changes
---

Rebase


Repository: mesos


Description
---

When force pull docker image, we should use the full docker image
name to avoid pulling down the repository.

Seems the latest docker client will add `latest` automatically
for now but I think that we still need to keep the logic in case
someone using old docker client.


Diffs (updated)
-

  src/docker/docker.cpp 472cb1b4dc2b0ac65721c732fca8ec70a7470f47 

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


Testing
---

make
make check


Thanks,

Guangya Liu



Re: Review Request 52738: Used `inspectImage` to get pulled image.

2017-01-07 Thread Guangya Liu

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

(Updated 一月 7, 2017, 10:15 a.m.)


Review request for mesos, Benjamin Mahler and Kevin Klues.


Summary (updated)
-

Used `inspectImage` to get pulled image.


Repository: mesos


Description
---

After image pull finished, use `inspectImage` to get
the returned image value.


Diffs
-

  src/docker/docker.cpp 472cb1b4dc2b0ac65721c732fca8ec70a7470f47 

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


Testing
---

make
make check

Also test with a real cases.

1) Remove busybox:latest 
2) Launch a task using busybox:latest , succeed.
3) Check agent log, there are log messages of pull docker image.
```
I1011 16:28:14.538455 16307 docker.cpp:1014] Starting container 
'0727d4b2-37ee-4aa6-8c15-7c3eac1fbdc7' for task 'test' (and executor 'test') of 
framework 6f14d15e-4963-4984-bc55-91fe21b63560-0009
I1011 16:28:14.539734 16307 docker.cpp:1470] Running docker -H 
unix:///var/run/docker.sock inspect busybox:latest
I1011 16:28:14.649885 16304 docker.cpp:1544] Running docker -H 
unix:///var/run/docker.sock pull busybox:latest
I1011 16:28:22.898789 16304 slave.cpp:5448] Querying resource estimator for 
oversubscribable resources
I1011 16:28:22.899137 16304 slave.cpp:5462] Received oversubscribable resources 
{} from the resource estimator
I1011 16:28:23.695930 16305 slave.cpp:4128] Received ping from 
slave-observer(2)@192.168.56.12:5050
I1011 16:28:30.308148 16306 docker.cpp:1470] Running docker -H 
unix:///var/run/docker.sock inspect busybox:latest
I1011 16:28:30.416584 16302 docker.cpp:422] Docker pull busybox:latest completed
I1011 16:28:30.418618 16302 docker.cpp:1322] Launching 'mesos-docker-executor' 
with flags 
'--container="mesos-6f14d15e-4963-4984-bc55-91fe21b63560-S1.0727d4b2-37ee-4aa6-8c15-7c3eac1fbdc7"
 --docker="docker" --docker_socket="/var/run/docker.sock" --help="false" 
--initialize_driver_logging="true" 
--launcher_dir="/root/src/mesos/m1/mesos/build/src" --logbufsecs="0" 
--logging_level="INFO" --mapped_directory="/mnt/mesos/sandbox" --quiet="false" 
--sandbox_directory="/tmp/mesos/slaves/6f14d15e-4963-4984-bc55-91fe21b63560-S1/frameworks/6f14d15e-4963-4984-bc55-91fe21b63560-0009/executors/test/runs/0727d4b2-37ee-4aa6-8c15-7c3eac1fbdc7"
 --stop_timeout="0ns"'
```


Thanks,

Guangya Liu



Re: Review Request 52738: Ued `inspectImage` to get pulled image.

2017-01-07 Thread Guangya Liu

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

(Updated 一月 7, 2017, 10:14 a.m.)


Review request for mesos, Benjamin Mahler and Kevin Klues.


Changes
---

Rebase


Summary (updated)
-

Ued `inspectImage` to get pulled image.


Repository: mesos


Description (updated)
---

After image pull finished, use `inspectImage` to get
the returned image value.


Diffs (updated)
-

  src/docker/docker.cpp 472cb1b4dc2b0ac65721c732fca8ec70a7470f47 

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


Testing
---

make
make check

Also test with a real cases.

1) Remove busybox:latest 
2) Launch a task using busybox:latest , succeed.
3) Check agent log, there are log messages of pull docker image.
```
I1011 16:28:14.538455 16307 docker.cpp:1014] Starting container 
'0727d4b2-37ee-4aa6-8c15-7c3eac1fbdc7' for task 'test' (and executor 'test') of 
framework 6f14d15e-4963-4984-bc55-91fe21b63560-0009
I1011 16:28:14.539734 16307 docker.cpp:1470] Running docker -H 
unix:///var/run/docker.sock inspect busybox:latest
I1011 16:28:14.649885 16304 docker.cpp:1544] Running docker -H 
unix:///var/run/docker.sock pull busybox:latest
I1011 16:28:22.898789 16304 slave.cpp:5448] Querying resource estimator for 
oversubscribable resources
I1011 16:28:22.899137 16304 slave.cpp:5462] Received oversubscribable resources 
{} from the resource estimator
I1011 16:28:23.695930 16305 slave.cpp:4128] Received ping from 
slave-observer(2)@192.168.56.12:5050
I1011 16:28:30.308148 16306 docker.cpp:1470] Running docker -H 
unix:///var/run/docker.sock inspect busybox:latest
I1011 16:28:30.416584 16302 docker.cpp:422] Docker pull busybox:latest completed
I1011 16:28:30.418618 16302 docker.cpp:1322] Launching 'mesos-docker-executor' 
with flags 
'--container="mesos-6f14d15e-4963-4984-bc55-91fe21b63560-S1.0727d4b2-37ee-4aa6-8c15-7c3eac1fbdc7"
 --docker="docker" --docker_socket="/var/run/docker.sock" --help="false" 
--initialize_driver_logging="true" 
--launcher_dir="/root/src/mesos/m1/mesos/build/src" --logbufsecs="0" 
--logging_level="INFO" --mapped_directory="/mnt/mesos/sandbox" --quiet="false" 
--sandbox_directory="/tmp/mesos/slaves/6f14d15e-4963-4984-bc55-91fe21b63560-S1/frameworks/6f14d15e-4963-4984-bc55-91fe21b63560-0009/executors/test/runs/0727d4b2-37ee-4aa6-8c15-7c3eac1fbdc7"
 --stop_timeout="0ns"'
```


Thanks,

Guangya Liu



Re: Review Request 52728: Renamed `inspect` to `inspectContainer`.

2017-01-07 Thread Guangya Liu

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

(Updated 一月 7, 2017, 10:13 a.m.)


Review request for mesos, Benjamin Mahler and Kevin Klues.


Changes
---

Rebase


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


Repository: mesos


Description
---

We introduced `inspectImage` for docker containerizer, and it is
better rename current `inspect` to `inspectContainer` to keep
consistent.


Diffs (updated)
-

  src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
  src/docker/docker.cpp 472cb1b4dc2b0ac65721c732fca8ec70a7470f47 
  src/docker/executor.cpp 9b5c469e2d0f33e228ec746711e6bc6ed352cbc7 
  src/slave/constants.hpp 1159ac3b1f8797a847a6f5679045434064a519e9 
  src/slave/containerizer/docker.cpp 7a8a7271b54af0b4dcdae7a2aa8a90a8a7d05fd6 
  src/tests/containerizer/docker_containerizer_tests.cpp 
4e3b67bbb989f9084dfdf4970839956dcb0caa0e 
  src/tests/containerizer/docker_tests.cpp 
9667d434486c1832f180a297a39a3d5dae6a26bd 
  src/tests/mock_docker.hpp 829a760d54ad8c7b88256ae5df4c88c9fb18df71 
  src/tests/mock_docker.cpp 02b6065a01e7e52b0edb38676dfb1bb475584502 

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


Testing
---

make
make check


Thanks,

Guangya Liu



Re: Review Request 55021: Added a framework capabilities struct.

2016-12-28 Thread Guangya Liu


> On 十二月 28, 2016, 9:24 a.m., Guangya Liu wrote:
> > src/common/protobuf_utils.hpp, lines 238-243
> > <https://reviews.apache.org/r/55021/diff/2/?file=1592796#file1592796line238>
> >
> > I'd like we keep a comment for each of the capability here and also 
> > keep the variable same as before.
> > ```
> >   // Whether the framework desires revocable resources.
> >   bool revocable = false;
> >   
> >   // Whether the framework can receive TASK_KILLING TaskState
> >   // when a task is being killed.
> >   bool taskKillingAware = false;
> >   
> >   // Whether the framework is aware of GPU resources. See
> >   // the documentation for the GPU_RESOURCES Capability.
> >   bool gpuAware = false;
> >   
> >   // Whether the framework desires shared resources.
> >   bool shared = false;
> >   
> >   // Whether the framework is prepared to handle the following
> >   // TaskStates: TASK_UNREACHABLE, TASK_DROPPED, TASK_GONE,
> >   // TASK_GONE_BY_OPERATOR, and TASK_UNKNOWN, and whether the
> >   // framework will assume responsibility for managing
> >   // partitioned tasks that reregister with the master.
> >   bool partitionAware = false;
> >  
> >   // Whether the framework support `multi-tenancy`.
> >   bool multiRole = false;
> > ```
> 
> Jay Guo wrote:
> previous naming seems more reasonable from literal perspective but as the 
> # of caps grows, it looks more straightforward to me to keep the consistency. 
> what do you think?

I have no comments for this but it is better leave some comments for each 
capability as before.


- Guangya


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


On 十二月 29, 2016, 1:37 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55021/
> ---
> 
> (Updated 十二月 29, 2016, 1:37 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Master, agent and allocator structs could use this stuct to easily
> deal with Framework Capabilities.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp f225bb0f5196f7f4b12be25828361a38e1d42e60 
>   src/tests/protobuf_utils_tests.cpp 4ce952a7d3901a4362aa69f199b135b53d14a723 
> 
> Diff: https://reviews.apache.org/r/55021/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>



Re: Review Request 55021: Added a framework capabilities struct.

2016-12-28 Thread Guangya Liu

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




src/common/protobuf_utils.hpp (line 209)
<https://reviews.apache.org/r/55021/#comment231313>

You may need a default constructor for `Capabilities` here, otherwise, the 
`hierarhical.cpp` will be failed when construct the `Framework`.



src/common/protobuf_utils.hpp (line 212)
<https://reviews.apache.org/r/55021/#comment231311>

s/cap/capability

Mesos perfer using long name which is easy to understand.



src/common/protobuf_utils.hpp (lines 238 - 243)
<https://reviews.apache.org/r/55021/#comment231314>

I'd like we keep a comment for each of the capability here and also keep 
the variable same as before.
```
  // Whether the framework desires revocable resources.
  bool revocable = false;
  
  // Whether the framework can receive TASK_KILLING TaskState
  // when a task is being killed.
  bool taskKillingAware = false;
  
  // Whether the framework is aware of GPU resources. See
  // the documentation for the GPU_RESOURCES Capability.
  bool gpuAware = false;
  
  // Whether the framework desires shared resources.
  bool shared = false;
  
  // Whether the framework is prepared to handle the following
  // TaskStates: TASK_UNREACHABLE, TASK_DROPPED, TASK_GONE,
  // TASK_GONE_BY_OPERATOR, and TASK_UNKNOWN, and whether the
  // framework will assume responsibility for managing
  // partitioned tasks that reregister with the master.
  bool partitionAware = false;
 
  // Whether the framework support `multi-tenancy`.
  bool multiRole = false;
```



src/tests/protobuf_utils_tests.cpp (lines 78 - 83)
<https://reviews.apache.org/r/55021/#comment231315>

Group the TRUE and FALSE cases, also the variable names may need some 
update accoding to my above comments.

```
  ASSERT_TRUE(capabilities.revocableResources);
  ASSERT_TRUE(capabilities.partitionAware);
  ASSERT_TRUE(capabilities.gpuResources);
  
  ASSERT_FALSE(capabilities.sharedResources);
  ASSERT_FALSE(capabilities.taskKillingState);
  ASSERT_FALSE(capabilities.multiRole);
```


- Guangya Liu


On 十二月 28, 2016, 8:32 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55021/
> ---
> 
> (Updated 十二月 28, 2016, 8:32 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Master, agent and allocator structs could use this stuct to easily
> deal with Framework Capabilities.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp f225bb0f5196f7f4b12be25828361a38e1d42e60 
>   src/tests/protobuf_utils_tests.cpp 4ce952a7d3901a4362aa69f199b135b53d14a723 
> 
> Diff: https://reviews.apache.org/r/55021/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>



Re: Review Request 54836: Added helpers to allocate / unallocate Resources.

2016-12-18 Thread Guangya Liu


> On 十二月 18, 2016, 1:10 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 1075
> > <https://reviews.apache.org/r/54836/diff/1/?file=1588580#file1588580line1075>
> >
> > How about put this in the `if` block?
> 
> Benjamin Mahler wrote:
> Then this wouldn't clear the allocation?

I mean the following, can this work?

```
if (resource_.resource.has_allocation_info()) {
  resource_.resource.clear_allocation_info();
  unallocated = true;
}
```


> On 十二月 18, 2016, 1:10 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 1062
> > <https://reviews.apache.org/r/54836/diff/1/?file=1588580#file1588580line1062>
> >
> > Shall we add a `CHECK` here to make sure this resource was not 
> > allocated to any role?
> 
> Benjamin Mahler wrote:
> We could, the alternative that I went for was to have callers (the 
> allocator) CHECK this invariant if they care about it, since it seems 
> specific to the way the allocator works.

I saw that you have updated the comments of `allocate` by allowing existing 
allocation info be overwitten by a new role, why do we allow this? Which 
secnario need this operation?


- Guangya


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


On 十二月 18, 2016, 4:36 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54836/
> ---
> 
> (Updated 十二月 18, 2016, 4:36 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helpers to allocate / unallocate Resources.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412 
>   include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1 
>   src/common/resources.cpp 7dbeefc8fc159f28dc1542ad6a761213c65ff226 
>   src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68 
>   src/v1/resources.cpp 859f7388035b855bf0cb60d5cbe746e6871bf833 
> 
> Diff: https://reviews.apache.org/r/54836/diff/
> 
> 
> Testing
> ---
> 
> Updated the existing allocation test to incorporate the new helper.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>



Re: Review Request 54837: Added a CHECK when adding a framework in the allocator.

2016-12-17 Thread Guangya Liu

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


Ship it!




Ship It!

- Guangya Liu


On 十二月 17, 2016, 10:50 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54837/
> ---
> 
> (Updated 十二月 17, 2016, 10:50 p.m.)
> 
> 
> Review request for mesos, Jay Guo and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a CHECK when adding a framework in the allocator.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> a8cbc8db3d1319718765783827f8be1981b56f04 
> 
> Diff: https://reviews.apache.org/r/54837/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>



Re: Review Request 54836: Added helpers to allocate / unallocate Resources.

2016-12-17 Thread Guangya Liu

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




include/mesos/resources.hpp (lines 335 - 336)
<https://reviews.apache.org/r/54836/#comment230600>

Can you please elaborate on this? I think that we do not want to allocate a 
resource again if it was already allocated to some other roles? even with 
optimistic offer?



include/mesos/resources.hpp (line 339)
<https://reviews.apache.org/r/54836/#comment230598>

What is the use of the return value here, can you please add more comments 
here for its usage? I think that the `unallocate()` will be called by 
`recoverResources`, and what is the problem if `unalocate` just return void for 
such case?



src/common/resources.cpp (line 1062)
<https://reviews.apache.org/r/54836/#comment230599>

Shall we add a `CHECK` here to make sure this resource was not allocated to 
any role?



src/common/resources.cpp (line 1075)
<https://reviews.apache.org/r/54836/#comment230597>

How about put this in the `if` block?


- Guangya Liu


On 十二月 17, 2016, 10:50 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54836/
> ---
> 
> (Updated 十二月 17, 2016, 10:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helpers to allocate / unallocate Resources.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412 
>   include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1 
>   src/common/resources.cpp 16f6de6741b548a8a9fff5b70447b1a312f10efe 
>   src/tests/resources_tests.cpp c0ff9777ceb1731086179e2966b07c46d035c110 
>   src/v1/resources.cpp a1adf1b27964bf6a89d97715cff05878d8c6cae9 
> 
> Diff: https://reviews.apache.org/r/54836/diff/
> 
> 
> Testing
> ---
> 
> Updated the existing allocation test to incorporate the new helper.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>



Re: Review Request 54842: Updated Resources to handle Resource.AllocationInfo.

2016-12-17 Thread Guangya Liu

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


Ship it!




Ship It!

- Guangya Liu


On 十二月 17, 2016, 10:50 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54842/
> ---
> 
> (Updated 十二月 17, 2016, 10:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With the addition of `Resource.AllocationInfo`, we must prevent the
> loss of allocation information when performing addition and
> subtraction of resources.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 16f6de6741b548a8a9fff5b70447b1a312f10efe 
>   src/tests/resources_tests.cpp c0ff9777ceb1731086179e2966b07c46d035c110 
>   src/v1/resources.cpp a1adf1b27964bf6a89d97715cff05878d8c6cae9 
> 
> Diff: https://reviews.apache.org/r/54842/diff/
> 
> 
> Testing
> ---
> 
> Added tests.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>



Re: Review Request 54830: Added helper function to get per-role resource allocations.

2016-12-17 Thread Guangya Liu


> On 十二月 17, 2016, 8:07 p.m., Benjamin Mahler wrote:
> > src/tests/resources_tests.cpp, lines 2765-2769
> > <https://reviews.apache.org/r/54830/diff/3/?file=1588522#file1588522line2765>
> >
> > Hm.. I didn't follow this assumption, I guess when you're saying "using 
> > star role" you're referring to unreserved resources?

Yes, I mean unreserved resource here.


- Guangya


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


On 十二月 17, 2016, 1:24 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54830/
> ---
> 
> (Updated 十二月 17, 2016, 1:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper function to get per-role resource allocations.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp f569c931ff7db8d51dfd7c96f4f2addab05df85d 
>   include/mesos/v1/resources.hpp f60ab794a0c7c24885c49cc47b798c363e3279e7 
>   src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f 
>   src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87 
>   src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9 
> 
> Diff: https://reviews.apache.org/r/54830/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> ./bin/mesos-tests.sh  --gtest_filter="AllocatedResourcesTest.Allocations" 
> --verbose
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from AllocatedResourcesTest
> [ RUN  ] AllocatedResourcesTest.Allocations
> [   OK ] AllocatedResourcesTest.Allocations (0 ms)
> [--] 1 test from AllocatedResourcesTest (0 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (17 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>



Re: Review Request 54830: Added helper function to get per-role resource allocations.

2016-12-17 Thread Guangya Liu

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

(Updated 十二月 17, 2016, 1:24 p.m.)


Review request for mesos, Benjamin Mahler and Jay Guo.


Repository: mesos


Description
---

Added helper function to get per-role resource allocations.


Diffs (updated)
-

  include/mesos/resources.hpp f569c931ff7db8d51dfd7c96f4f2addab05df85d 
  include/mesos/v1/resources.hpp f60ab794a0c7c24885c49cc47b798c363e3279e7 
  src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f 
  src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87 
  src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9 

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


Testing
---

make
make check

```
./bin/mesos-tests.sh  --gtest_filter="AllocatedResourcesTest.Allocations" 
--verbose
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from AllocatedResourcesTest
[ RUN  ] AllocatedResourcesTest.Allocations
[   OK ] AllocatedResourcesTest.Allocations (0 ms)
[--] 1 test from AllocatedResourcesTest (0 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (17 ms total)
[  PASSED  ] 1 test.
```


Thanks,

Guangya Liu



Re: Review Request 54830: Added helper function to get per-role resource allocations.

2016-12-17 Thread Guangya Liu


> On 十二月 17, 2016, 10:19 a.m., Jay Guo wrote:
> > src/common/resources.cpp, lines 1082-1083
> > <https://reviews.apache.org/r/54830/diff/2/?file=1588444#file1588444line1082>
> >
> > Maybe "We do not expect allocated and unallocated resource objects 
> > being stored within the same `Resources` object."?

I updated this to "We do not expect unallocated resource objects being stored 
within the this `Resources` object."


- Guangya


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


On 十二月 17, 2016, 4:28 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54830/
> ---
> 
> (Updated 十二月 17, 2016, 4:28 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper function to get per-role resource allocations.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp f569c931ff7db8d51dfd7c96f4f2addab05df85d 
>   include/mesos/v1/resources.hpp f60ab794a0c7c24885c49cc47b798c363e3279e7 
>   src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f 
>   src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87 
>   src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9 
> 
> Diff: https://reviews.apache.org/r/54830/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> ./bin/mesos-tests.sh  --gtest_filter="AllocatedResourcesTest.Allocations" 
> --verbose
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from AllocatedResourcesTest
> [ RUN  ] AllocatedResourcesTest.Allocations
> [   OK ] AllocatedResourcesTest.Allocations (0 ms)
> [------] 1 test from AllocatedResourcesTest (0 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (17 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>



  1   2   3   4   5   6   7   8   9   10   >