You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2017/05/09 13:48:16 UTC
Review Request 59092: Added local resource provider driver.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59092/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier and Jan Schlicht.
Bugs: MESOS-7469
https://issues.apache.org/jira/browse/MESOS-7469
Repository: mesos
Description
-------
The local resource provider driver will be used by local resource
providers. It relies on the agent's connection to the master. The
agent will be responsible for forwarding Events and Calls by invoking
corresponding functions from this driver.
Diffs
-----
include/mesos/v1/resource_provider.hpp PRE-CREATION
include/mesos/v1/resource_provider/resource_provider.hpp 2b8c8afab852621fb49b132813d512d0c96bc68c
src/CMakeLists.txt 40d921ee7026f5ac47efbf0243e1cafab57825f9
src/Makefile.am 6bb81fd49b4564a0afa993b2cef6baa9d370ee7a
src/resource_provider/local_driver.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/59092/diff/1/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 59092: Added local resource provider driver.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59092/#review174404
-----------------------------------------------------------
include/mesos/v1/resource_provider.hpp
Lines 39 (patched)
<https://reviews.apache.org/r/59092/#comment247540>
s/alraedy/already/
src/Makefile.am
Lines 770 (patched)
<https://reviews.apache.org/r/59092/#comment247541>
This line should be put before the above line.
- Qian Zhang
On May 9, 2017, 9:48 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59092/
> -----------------------------------------------------------
>
> (Updated May 9, 2017, 9:48 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jan Schlicht.
>
>
> Bugs: MESOS-7469
> https://issues.apache.org/jira/browse/MESOS-7469
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The local resource provider driver will be used by local resource
> providers. It relies on the agent's connection to the master. The
> agent will be responsible for forwarding Events and Calls by invoking
> corresponding functions from this driver.
>
>
> Diffs
> -----
>
> include/mesos/v1/resource_provider.hpp PRE-CREATION
> include/mesos/v1/resource_provider/resource_provider.hpp 2b8c8afab852621fb49b132813d512d0c96bc68c
> src/CMakeLists.txt 40d921ee7026f5ac47efbf0243e1cafab57825f9
> src/Makefile.am 6bb81fd49b4564a0afa993b2cef6baa9d370ee7a
> src/resource_provider/local_driver.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59092/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 59092: Added local resource provider driver.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59092/#review174451
-----------------------------------------------------------
src/resource_provider/local_driver.cpp
Lines 19-25 (patched)
<https://reviews.apache.org/r/59092/#comment247622>
These should be sorted alphabetically. I.e. the `mesos` includes come first.
src/resource_provider/local_driver.cpp
Lines 27-38 (patched)
<https://reviews.apache.org/r/59092/#comment247625>
Sort these as well.
src/resource_provider/local_driver.cpp
Lines 66 (patched)
<https://reviews.apache.org/r/59092/#comment247627>
How about we add a log line here, to be able to debug these cases? E.g. `MesosSchedulerDriver` would `VLOG(1)` in such a case.
Here and below.
- Jan Schlicht
On May 9, 2017, 3:48 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59092/
> -----------------------------------------------------------
>
> (Updated May 9, 2017, 3:48 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jan Schlicht.
>
>
> Bugs: MESOS-7469
> https://issues.apache.org/jira/browse/MESOS-7469
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The local resource provider driver will be used by local resource
> providers. It relies on the agent's connection to the master. The
> agent will be responsible for forwarding Events and Calls by invoking
> corresponding functions from this driver.
>
>
> Diffs
> -----
>
> include/mesos/v1/resource_provider.hpp PRE-CREATION
> include/mesos/v1/resource_provider/resource_provider.hpp 2b8c8afab852621fb49b132813d512d0c96bc68c
> src/CMakeLists.txt 40d921ee7026f5ac47efbf0243e1cafab57825f9
> src/Makefile.am 6bb81fd49b4564a0afa993b2cef6baa9d370ee7a
> src/resource_provider/local_driver.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59092/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 59092: Added local resource provider driver.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59092/#review174305
-----------------------------------------------------------
include/mesos/v1/resource_provider.hpp
Lines 37 (patched)
<https://reviews.apache.org/r/59092/#comment247441>
Add a `virtual` dtr here.
include/mesos/v1/resource_provider.hpp
Lines 59-60 (patched)
<https://reviews.apache.org/r/59092/#comment247446>
Recently some people have used `override` to mark overridden functions instead of the purely documenting `virtual`. Up to you.
include/mesos/v1/resource_provider.hpp
Lines 69 (patched)
<https://reviews.apache.org/r/59092/#comment247442>
Any reason this needs to be potentially shareable? We typically seem to prefer owned semantics, how about a `unique_ptr<LocalDriverProcess>`?
src/resource_provider/local_driver.cpp
Lines 17 (patched)
<https://reviews.apache.org/r/59092/#comment247444>
unused?
src/resource_provider/local_driver.cpp
Lines 23 (patched)
<https://reviews.apache.org/r/59092/#comment247443>
unused.
src/resource_provider/local_driver.cpp
Lines 63 (patched)
<https://reviews.apache.org/r/59092/#comment247445>
This overload hides `ProcessBase::send`. We should either rename this function, or unhide the base class method by pulling it into the class explicitly with a using decl.
- Benjamin Bannier
On May 9, 2017, 3:48 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59092/
> -----------------------------------------------------------
>
> (Updated May 9, 2017, 3:48 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jan Schlicht.
>
>
> Bugs: MESOS-7469
> https://issues.apache.org/jira/browse/MESOS-7469
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The local resource provider driver will be used by local resource
> providers. It relies on the agent's connection to the master. The
> agent will be responsible for forwarding Events and Calls by invoking
> corresponding functions from this driver.
>
>
> Diffs
> -----
>
> include/mesos/v1/resource_provider.hpp PRE-CREATION
> include/mesos/v1/resource_provider/resource_provider.hpp 2b8c8afab852621fb49b132813d512d0c96bc68c
> src/CMakeLists.txt 40d921ee7026f5ac47efbf0243e1cafab57825f9
> src/Makefile.am 6bb81fd49b4564a0afa993b2cef6baa9d370ee7a
> src/resource_provider/local_driver.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59092/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>