Re: Review Request 71480: Fixed a bug for non-partition-aware schedulers.

2019-09-12 Thread Mesos Reviewbot 

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



Patch looks great!

Reviews applied: [71480]

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

- Mesos Reviewbot


On Sept. 12, 2019, 11:18 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71480/
> ---
> 
> (Updated Sept. 12, 2019, 11:18 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Joseph Wu, and Vinod Kone.
> 
> 
> Bugs: MESOS-9965
> https://issues.apache.org/jira/browse/MESOS-9965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the agent would send task status updates with the state
> TASK_GONE_BY_OPERATOR to all schedulers when an agent was drained
> with the `mark_gone` parameter set to `true`.
> 
> This patch updates this code to ensure that TASK_GONE_BY_OPERATOR
> is only sent to partition-aware schedulers.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 4e9365669395c89202961ca80d41cd92346d23d2 
>   src/tests/slave_tests.cpp 02b65a925a52d09b0a9183d3288b9ab363b98bc5 
> 
> 
> Diff: https://reviews.apache.org/r/71480/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 71479: Added a test to ensure resources are recovered during agent removal.

2019-09-12 Thread Mesos Reviewbot 

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



Patch looks great!

Reviews applied: [71459, 71460, 71476, 71479]

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

- Mesos Reviewbot


On Sept. 12, 2019, 10:51 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71479/
> ---
> 
> (Updated Sept. 12, 2019, 10:51 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
> 
> 
> Bugs: MESOS-621
> https://issues.apache.org/jira/browse/MESOS-621
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test to ensure resources are recovered during agent removal.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 5f2e2b259af9920133dd22a5b03774d7035821dc 
> 
> 
> Diff: https://reviews.apache.org/r/71479/diff/1/
> 
> 
> Testing
> ---
> 
> Failed on master.
> Pass after r/71476
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 71481: Added some extra ASSERTs to a role test.

2019-09-12 Thread Meng Zhu 

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


Ship it!




Ship It!

- Meng Zhu


On Sept. 12, 2019, 5:48 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71481/
> ---
> 
> (Updated Sept. 12, 2019, 5:48 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Greg Mann, and Meng Zhu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These asserts technically aren't required, since subsequent
> awaiting for offers assumes that the agents are created successfully.
> However, it seems useful to fail a bit earlier if the agent creation
> happens to fail.
> 
> 
> Diffs
> -
> 
>   src/tests/role_tests.cpp d6cc31bf89954c5a440142b378560cdc0286df23 
> 
> 
> Diff: https://reviews.apache.org/r/71481/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 71481: Added some extra ASSERTs to a role test.

2019-09-12 Thread Joseph Wu 

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

Review request for mesos, Benjamin Mahler, Greg Mann, and Meng Zhu.


Repository: mesos


Description
---

These asserts technically aren't required, since subsequent
awaiting for offers assumes that the agents are created successfully.
However, it seems useful to fail a bit earlier if the agent creation
happens to fail.


Diffs
-

  src/tests/role_tests.cpp d6cc31bf89954c5a440142b378560cdc0286df23 


Diff: https://reviews.apache.org/r/71481/diff/1/


Testing
---

make check


Thanks,

Joseph Wu

Re: Review Request 71480: Fixed a bug for non-partition-aware schedulers.

2019-09-12 Thread Joseph Wu 

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


Ship it!




Ship It!

- Joseph Wu


On Sept. 12, 2019, 4:18 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71480/
> ---
> 
> (Updated Sept. 12, 2019, 4:18 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Joseph Wu, and Vinod Kone.
> 
> 
> Bugs: MESOS-9965
> https://issues.apache.org/jira/browse/MESOS-9965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the agent would send task status updates with the state
> TASK_GONE_BY_OPERATOR to all schedulers when an agent was drained
> with the `mark_gone` parameter set to `true`.
> 
> This patch updates this code to ensure that TASK_GONE_BY_OPERATOR
> is only sent to partition-aware schedulers.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 4e9365669395c89202961ca80d41cd92346d23d2 
>   src/tests/slave_tests.cpp 02b65a925a52d09b0a9183d3288b9ab363b98bc5 
> 
> 
> Diff: https://reviews.apache.org/r/71480/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 71480: Fixed a bug for non-partition-aware schedulers.

2019-09-12 Thread Greg Mann 

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

Review request for mesos, Gilbert Song, Joseph Wu, and Vinod Kone.


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


Repository: mesos


Description
---

Previously, the agent would send task status updates with the state
TASK_GONE_BY_OPERATOR to all schedulers when an agent was drained
with the `mark_gone` parameter set to `true`.

This patch updates this code to ensure that TASK_GONE_BY_OPERATOR
is only sent to partition-aware schedulers.


Diffs
-

  src/slave/slave.cpp 4e9365669395c89202961ca80d41cd92346d23d2 
  src/tests/slave_tests.cpp 02b65a925a52d09b0a9183d3288b9ab363b98bc5 


Diff: https://reviews.apache.org/r/71480/diff/1/


Testing
---

`make check`


Thanks,

Greg Mann

Review Request 71479: Added a test to ensure resources are recovered during agent removal.

2019-09-12 Thread Meng Zhu 

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

Review request for mesos.


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


Repository: mesos


Description
---

Added a test to ensure resources are recovered during agent removal.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
5f2e2b259af9920133dd22a5b03774d7035821dc 


Diff: https://reviews.apache.org/r/71479/diff/1/


Testing
---

Failed on master.
Pass after r/71476


Thanks,

Meng Zhu

Review Request 71476: Simplified recover resources when removing frameworks or agents.

2019-09-12 Thread Meng Zhu 

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

Review request for mesos, Andrei Sekretenko and Benjamin Mahler.


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


Repository: mesos


Description
---

Simplified recover resources when removing frameworks or agents.


Diffs
-

  src/master/allocator/mesos/hierarchical.hpp 
5ea37912dadded47eaa2d354889c95533b191c59 
  src/master/allocator/mesos/hierarchical.cpp 
256fdce61ccfb768605cac1579c9a6632cd26fec 
  src/master/master.cpp a2c289a8b6e30eac690f77c7fc1b12b16f62f541 


Diff: https://reviews.apache.org/r/71476/diff/1/


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 71478: Windows: Moved definition out of inline function call.

2019-09-12 Thread Mesos Reviewbot 

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



Patch looks great!

Reviews applied: [71478]

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

- Mesos Reviewbot


On Sept. 12, 2019, 7:35 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71478/
> ---
> 
> (Updated Sept. 12, 2019, 7:35 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MSVC does not deal with #ifdefs from inside function calls.
> So here, the `#if defined(...)` was taken literally and is
> considered a syntax error by MSVC.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> 5854711971c9ebc4d676edc43af5ab5cfd5ea4c6 
> 
> 
> Diff: https://reviews.apache.org/r/71478/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 71478: Windows: Moved definition out of inline function call.

2019-09-12 Thread Joseph Wu 

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

Review request for mesos, Greg Mann and Till Toenshoff.


Repository: mesos


Description
---

MSVC does not deal with #ifdefs from inside function calls.
So here, the `#if defined(...)` was taken literally and is
considered a syntax error by MSVC.


Diffs
-

  3rdparty/libprocess/src/openssl.cpp 5854711971c9ebc4d676edc43af5ab5cfd5ea4c6 


Diff: https://reviews.apache.org/r/71478/diff/1/


Testing
---

make check


Thanks,

Joseph Wu

Re: Review Request 71440: Improved allocator inverse offer test.

2019-09-12 Thread Mesos Reviewbot 

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



Patch looks great!

Reviews applied: [68507, 68508, 71428, 71440]

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

- Mesos Reviewbot


On Sept. 5, 2019, 9:12 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71440/
> ---
> 
> (Updated Sept. 5, 2019, 9:12 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The test is augmented to also check that a framework that
> declined offers from an agent will not get inverse offers
> for that agent.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 2c1d0fe2e3ac08e6443cf8348bb72ac7dae454a8 
> 
> 
> Diff: https://reviews.apache.org/r/71440/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 71445: Removed race in `StorageLocalResourceProviderTest.Update`.

2019-09-12 Thread Mesos Reviewbot 

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



Patch looks great!

Reviews applied: [71445]

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

- Mesos Reviewbot


On Sept. 12, 2019, 9:01 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71445/
> ---
> 
> (Updated Sept. 12, 2019, 9:01 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The agent might take some time before registering with the master which
> could have lead to unexpected retries in this test which the clock was
> resumed. This patch simplifies the test so we do not rely on
> `UpdateSlaveMessage` anymore.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 089aa9787a66d737267179ad461be0c0a99d5c63 
> 
> 
> Diff: https://reviews.apache.org/r/71445/diff/2/
> 
> 
> Testing
> ---
> 
> * `make check`
> * ran test fixed here in repetition under external stress. Previous failure 
> rate ~6%, no failures in >5k runs after
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71460: Tracked offered and allocated resources in the role tree.

2019-09-12 Thread Andrei Sekretenko 

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


Fix it, then Ship it!





src/master/allocator/mesos/hierarchical.cpp
Lines 430 (patched)


Stale comment?


- Andrei Sekretenko


On Sept. 11, 2019, 10:52 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71460/
> ---
> 
> (Updated Sept. 11, 2019, 10:52 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This helpers simplify the quota tracking logic and also paves
> the way to reduce duplicated states in the sorter.
> 
> Also documented that shared resources must be uniquely
> identifiable.
> 
> Small performance degradation when making allocations due to
> duplicated map construction in `(un)trackAllocatedResources`.
> This will be removed once embeded the sorter in the role tree.
> 
> Benchmark `LargeAndSmallQuota/2`:
> 
> Master:
> 
> Added 3000 agents in 80.648188ms
> Added 3000 frameworks in 19.7006984secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks,
> with drf sorter
> Made 3500 allocations in 16.044274434secs
> Made 0 allocation in 14.476429451secs
> 
> Master + this patch:
> Added 3000 agents in 80.110817ms
> Added 3000 frameworks in 17.25974094secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks,
> with drf sorter
> Made 3500 allocations in 16.91971379secs
> Made 0 allocation in 13.784476154secs
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8fd838e96228daacb0a1c0f4890fd3630841ab52 
>   include/mesos/v1/mesos.proto da19256cbb53215dc6f51d809ad214683ea153cf 
>   src/master/allocator/mesos/hierarchical.hpp 
> 48ba399e12129b3348b5c43b793f3428acf26d4e 
>   src/master/allocator/mesos/hierarchical.cpp 
> 187de173ee2f563e2ff0f7e5c96695dd94a73970 
> 
> 
> Diff: https://reviews.apache.org/r/71460/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> Benchmark result in the description
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 71445: Removed race in `StorageLocalResourceProviderTest.Update`.

2019-09-12 Thread Jan Schlicht 

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


Ship it!




Ship It!

- Jan Schlicht


On Sept. 12, 2019, 11:01 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71445/
> ---
> 
> (Updated Sept. 12, 2019, 11:01 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The agent might take some time before registering with the master which
> could have lead to unexpected retries in this test which the clock was
> resumed. This patch simplifies the test so we do not rely on
> `UpdateSlaveMessage` anymore.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 089aa9787a66d737267179ad461be0c0a99d5c63 
> 
> 
> Diff: https://reviews.apache.org/r/71445/diff/2/
> 
> 
> Testing
> ---
> 
> * `make check`
> * ran test fixed here in repetition under external stress. Previous failure 
> rate ~6%, no failures in >5k runs after
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71445: Removed race in `StorageLocalResourceProviderTest.Update`.

2019-09-12 Thread Benjamin Bannier 

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

(Updated Sept. 12, 2019, 11:01 a.m.)


Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.


Changes
---

Simplify the test even more


Repository: mesos


Description
---

The agent might take some time before registering with the master which
could have lead to unexpected retries in this test which the clock was
resumed. This patch simplifies the test so we do not rely on
`UpdateSlaveMessage` anymore.


Diffs (updated)
-

  src/tests/storage_local_resource_provider_tests.cpp 
089aa9787a66d737267179ad461be0c0a99d5c63 


Diff: https://reviews.apache.org/r/71445/diff/2/

Changes: https://reviews.apache.org/r/71445/diff/1-2/


Testing
---

* `make check`
* ran test fixed here in repetition under external stress. Previous failure 
rate ~6%, no failures in >5k runs after


Thanks,

Benjamin Bannier