Re: Review Request 70169: Refactored SLRP to use `ServiceManager`.

2019-03-22 Thread Mesos Reviewbot Windows 

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



FAIL: Failed to apply the current review.

Failed command: `python.exe .\support\apply-reviews.py -n -r 70169`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2996/mesos-review-70169

Relevant logs:

- 
[apply-review-70169.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2996/mesos-review-70169/logs/apply-review-70169.log):

```
error: patch failed: src/resource_provider/storage/provider.cpp:41
error: src/resource_provider/storage/provider.cpp: patch does not apply
error: patch failed: src/resource_provider/storage/provider_process.hpp:17
error: src/resource_provider/storage/provider_process.hpp: patch does not apply
```

- Mesos Reviewbot Windows


On March 20, 2019, 6:03 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70169/
> ---
> 
> (Updated March 20, 2019, 6:03 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9632
> https://issues.apache.org/jira/browse/MESOS-9632
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored SLRP to use `ServiceManager`.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> fea623c292158deb1b4b4b9ab1ac208031471519 
>   src/resource_provider/storage/provider_process.hpp 
> a5536b3d735e01eb1c4dc52d0602d973155f3c93 
> 
> 
> Diff: https://reviews.apache.org/r/70169/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70169: Refactored SLRP to use `ServiceManager`.

2019-03-20 Thread Benjamin Bannier 

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


Fix it, then Ship it!




Looking good. Let's squash this with https://reviews.apache.org/r/70168/.


src/resource_provider/storage/provider.cpp
Line 1 (original), 1 (patched)


Please squash this whole patch with https://reviews.apache.org/r/70168/ as 
we are moving code from here to there.


- Benjamin Bannier


On March 20, 2019, 7:03 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70169/
> ---
> 
> (Updated March 20, 2019, 7:03 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9632
> https://issues.apache.org/jira/browse/MESOS-9632
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored SLRP to use `ServiceManager`.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> fea623c292158deb1b4b4b9ab1ac208031471519 
>   src/resource_provider/storage/provider_process.hpp 
> a5536b3d735e01eb1c4dc52d0602d973155f3c93 
> 
> 
> Diff: https://reviews.apache.org/r/70169/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70169: Refactored SLRP to use `ServiceManager`.

2019-03-20 Thread Chun-Hung Hsiao 

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

(Updated March 20, 2019, 6:03 a.m.)


Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.


Changes
---

Addressed Benjamin's comments.


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


Repository: mesos


Description
---

Refactored SLRP to use `ServiceManager`.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
fea623c292158deb1b4b4b9ab1ac208031471519 
  src/resource_provider/storage/provider_process.hpp 
a5536b3d735e01eb1c4dc52d0602d973155f3c93 


Diff: https://reviews.apache.org/r/70169/diff/3/

Changes: https://reviews.apache.org/r/70169/diff/2-3/


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 70169: Refactored SLRP to use `ServiceManager`.

2019-03-19 Thread Chun-Hung Hsiao 


> On March 19, 2019, 1:12 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Line 3734 (original), 3140 (patched)
> > 
> >
> > Preexisting, but isn't this already covered by above 
> > `operations_dropped` counter for `UNKNOWN`?

No. The previous `switch` trick would skip `UNKNOWN`, as we don't want to 
create `operations/unknown/pending`, `operations/unknown/finished` and 
`operations/unknown/failed`. Dropping.

I'm planning to do a small cleanup by, e.g., introducing 
`isSupportedOperation`, and replace the `switch` trick with a loop that uses 
the helpers provided by protobuf.


> On March 19, 2019, 1:12 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider_process.hpp
> > Line 369 (original), 344 (patched)
> > 
> >
> > Preexisting condition, but should we make this class non-copyable?

It already is as the copy ctor and assignment have been marked as deleted. 
Dropping.


> On March 19, 2019, 1:12 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider_process.hpp
> > Line 402 (original), 373 (patched)
> > 
> >
> > Composition instead of inheritance seems to work just fine here, let's 
> > do that instead.

Yeah both work. The inheritance approach would enable us to use 
`metrics.csi_plugin_container_termination` instead of 
`metrics.csiPluginMetrics.container_termination`. WDYT?

Also let's move the discussion to r/70245.


- Chun-Hung


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


On March 12, 2019, 8:43 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70169/
> ---
> 
> (Updated March 12, 2019, 8:43 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9632
> https://issues.apache.org/jira/browse/MESOS-9632
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored SLRP to use `ServiceManager`.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> fea623c292158deb1b4b4b9ab1ac208031471519 
>   src/resource_provider/storage/provider_process.hpp 
> a5536b3d735e01eb1c4dc52d0602d973155f3c93 
> 
> 
> Diff: https://reviews.apache.org/r/70169/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70169: Refactored SLRP to use `ServiceManager`.

2019-03-19 Thread Benjamin Bannier 

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



Reviewed simultaneously with https://reviews.apache.org/r/70168/, suggest to 
merge the trivial code movements in a single patch. Please see comment there as 
well.


src/resource_provider/storage/provider.cpp
Line 3734 (original), 3140 (patched)


Preexisting, but isn't this already covered by above `operations_dropped` 
counter for `UNKNOWN`?



src/resource_provider/storage/provider_process.hpp
Line 122 (original), 119 (patched)


Preexisting, but I don't understand the value this comment adds to this 
decl. If anything, we could make the parameter `const` when actually defining 
this function, but it seems leaky to expose this in the declaration.



src/resource_provider/storage/provider_process.hpp
Line 369 (original), 344 (patched)


Preexisting condition, but should we make this class non-copyable?



src/resource_provider/storage/provider_process.hpp
Line 402 (original), 373 (patched)


Composition instead of inheritance seems to work just fine here, let's do 
that instead.


- Benjamin Bannier


On March 12, 2019, 9:43 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70169/
> ---
> 
> (Updated March 12, 2019, 9:43 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9632
> https://issues.apache.org/jira/browse/MESOS-9632
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored SLRP to use `ServiceManager`.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> fea623c292158deb1b4b4b9ab1ac208031471519 
>   src/resource_provider/storage/provider_process.hpp 
> a5536b3d735e01eb1c4dc52d0602d973155f3c93 
> 
> 
> Diff: https://reviews.apache.org/r/70169/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70169: Refactored SLRP to use `ServiceManager`.

2019-03-13 Thread Mesos Reviewbot Windows 

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['70168', '70169']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2941/mesos-review-70169

Relevant logs:

- 
[mesos-tests.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2941/mesos-review-70169/logs/mesos-tests.log):

```
I0313 21:49:52.153338 13108 master.cpp:11596] Removing task 
501f2cba-a103-4807-a1b3-9b82533c60ef with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework d2ab3f23-c49e-452a-8df7-23361e63b23f- on 
agent d2ab3f23-c49e-452a-8df7-23361e63b23f-S0 at slave(494)@192.10.1.7:56848 
(windows-03.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0313 21:49:52.157333 13108 master.cpp:1295] Agent 
d2ab3f23-c49e-452a-8df7-23361e63b23f-S0 at slave(494)@192.10.1.7:56848 
(windows-03.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net) disconnected
I0313 21:49:52.157333 13108 master.cpp:3330] Disconnecting agent 
d2ab3f23-c49e-452a-8df7-23361e63b23f-S0 at slave(494)@192.10.1.7:56848 
(windows-03.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0313 21:49:52.157333 13108 master.cpp:3349] Deactivating agent 
d2ab3f23-c49e-452a-8df7-23361e63b23f-S0 at slave(494)@192.10.1.7:56848 
(windows-03.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0313 21:49:52.158335  8956 hierarchical.cpp:390] Removed framework 
d2ab3f23-c49e-452a-8df7-23361e63b23f-
I0313 21:49:52.158335  8956 hierarchical.cpp:827] Agent 
d2ab3f23-c49e-452a-8df7-23361e63b23f-S0 deactivated
I0313 21:49:52.158335  9620 containerizer.cpp:2576] Destroying container 
b8b9d05c-3a90-4839-b308-7fa95fe55840 in RUNNING state
I0313 21:49:52.158335  9620 containerizer.cpp:3278] Transitioning the state of 
container b8b9d05c-3a90-4839-b308-7fa95fe55840 from RUNNING to DESTROYING
I0313 21:49:52.159339  9620 launcher.cpp:161] Asked to destroy container 
b8b9d05c-3a90-4839-b308-7fa95fe55840
W0313 21:49:52.160332 15040 process.cpp:1423] Failed to recv on socket 
WindowsFD::Type::SOCKET=10028 to peer '192.10.1.7:59186': IO failed with error 
code: The specified network name is no longer available.

W0313 21:49:52.161321 15040 process.cpp:838] Failed to recv on socket 
WindowsFD::Type::SOCKET=10148 to peer '192.10.1.7:59187': IO failed with error 
code: The spe[   OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (690 
ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (706 ms total)

[--] Global test environment tear-down
[==] 1115 tests from 106 test cases ran. (502409 ms total)
[  PASSED  ] 1114 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] DockerFetcherPluginTest.INTERNET_CURL_FetchImage

 1 FAILED TEST
  YOU HAVE 232 DISABLED TESTS

cified network name is no longer available.

I0313 21:49:52.238680  8956 containerizer.cpp:3117] Container 
b8b9d05c-3a90-4839-b308-7fa95fe55840 has exited
I0313 21:49:52.267634  9620 master.cpp:1135] Master terminating
I0313 21:49:52.268743  9568 hierarchical.cpp:678] Removed agent 
d2ab3f23-c49e-452a-8df7-23361e63b23f-S0
I0313 21:49:52.54 15040 process.cpp:927] Stopped the socket accept loop
```

- Mesos Reviewbot Windows


On March 12, 2019, 8:43 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70169/
> ---
> 
> (Updated March 12, 2019, 8:43 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-9632
> https://issues.apache.org/jira/browse/MESOS-9632
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored SLRP to use `ServiceManager`.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> fea623c292158deb1b4b4b9ab1ac208031471519 
>   src/resource_provider/storage/provider_process.hpp 
> a5536b3d735e01eb1c4dc52d0602d973155f3c93 
> 
> 
> Diff: https://reviews.apache.org/r/70169/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70169: Refactored SLRP to use `ServiceManager`.

2019-03-12 Thread Chun-Hung Hsiao 

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

(Updated March 12, 2019, 8:43 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Specified the set of services in SLRP.


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


Repository: mesos


Description
---

Refactored SLRP to use `ServiceManager`.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
fea623c292158deb1b4b4b9ab1ac208031471519 
  src/resource_provider/storage/provider_process.hpp 
a5536b3d735e01eb1c4dc52d0602d973155f3c93 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 70169: Refactored SLRP to use `ServiceManager`.

2019-03-08 Thread Mesos Reviewbot 

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



Patch looks great!

Reviews applied: [70168, 70169]

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 March 8, 2019, 7:29 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70169/
> ---
> 
> (Updated March 8, 2019, 7:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-9632
> https://issues.apache.org/jira/browse/MESOS-9632
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored SLRP to use `ServiceManager`.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> fea623c292158deb1b4b4b9ab1ac208031471519 
>   src/resource_provider/storage/provider_process.hpp 
> a5536b3d735e01eb1c4dc52d0602d973155f3c93 
> 
> 
> Diff: https://reviews.apache.org/r/70169/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70169: Refactored SLRP to use `ServiceManager`.

2019-03-08 Thread Mesos Reviewbot 

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



Patch looks great!

Reviews applied: [70168, 70169]

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 March 8, 2019, 7:29 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70169/
> ---
> 
> (Updated March 8, 2019, 7:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-9632
> https://issues.apache.org/jira/browse/MESOS-9632
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored SLRP to use `ServiceManager`.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> fea623c292158deb1b4b4b9ab1ac208031471519 
>   src/resource_provider/storage/provider_process.hpp 
> a5536b3d735e01eb1c4dc52d0602d973155f3c93 
> 
> 
> Diff: https://reviews.apache.org/r/70169/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 70169: Refactored SLRP to use `ServiceManager`.

2019-03-08 Thread Chun-Hung Hsiao 

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

Review request for mesos, Benjamin Bannier and Jie Yu.


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


Repository: mesos


Description
---

Refactored SLRP to use `ServiceManager`.


Diffs
-

  src/resource_provider/storage/provider.cpp 
fea623c292158deb1b4b4b9ab1ac208031471519 
  src/resource_provider/storage/provider_process.hpp 
a5536b3d735e01eb1c4dc52d0602d973155f3c93 


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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao