You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chun-Hung Hsiao <ch...@apache.org> on 2019/03/08 19:29:23 UTC
Review Request 70169: Refactored SLRP to use `ServiceManager`.
-----------------------------------------------------------
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
Re: Review Request 70169: Refactored SLRP to use `ServiceManager`.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
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-0000 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-0000
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.566664 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`.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On March 19, 2019, 1:12 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Line 3734 (original), 3140 (patched)
> > <https://reviews.apache.org/r/70169/diff/2/?file=2131596#file2131596line3745>
> >
> > 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)
> > <https://reviews.apache.org/r/70169/diff/2/?file=2131597#file2131597line370>
> >
> > 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)
> > <https://reviews.apache.org/r/70169/diff/2/?file=2131597#file2131597line403>
> >
> > 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`.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/70169/#comment299829>
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)
<https://reviews.apache.org/r/70169/#comment299827>
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)
<https://reviews.apache.org/r/70169/#comment299826>
Preexisting condition, but should we make this class non-copyable?
src/resource_provider/storage/provider_process.hpp
Line 402 (original), 373 (patched)
<https://reviews.apache.org/r/70169/#comment299828>
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`.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/70169/#comment299899>
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`.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
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`.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
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`.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
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`.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 70169: Refactored SLRP to use `ServiceManager`.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
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
>
>