You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2018/01/31 00:13:45 UTC
Re: Review Request 63991: Added helpers to create and forward offer
operation updates.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63991/
-----------------------------------------------------------
(Updated Jan. 31, 2018, 12:13 a.m.)
Review request for mesos, Benjamin Bannier, Gaston Kleiman, and Jie Yu.
Bugs: MESOS-8190
https://issues.apache.org/jira/browse/MESOS-8190
Repository: mesos
Description
-------
Added helpers to create and forward offer operation updates.
Diffs
-----
src/common/protobuf_utils.hpp b2aa3654db2fe7d7d9d275ded81c6d54244654ee
src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc
src/master/master.hpp 2a2e830354db4a2191fb8321beb8174b80f7ba7d
src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206
Diff: https://reviews.apache.org/r/63991/diff/1/
Testing
-------
make check
Thanks,
Greg Mann
Re: Review Request 63991: Added helpers to create and forward offer
operation updates.
Posted by Gaston Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63991/#review198557
-----------------------------------------------------------
Fix it, then Ship it!
src/common/protobuf_utils.hpp
Lines 167 (patched)
<https://reviews.apache.org/r/63991/#comment278751>
This function generates `UpdateOperationStatus` events for frameworks.
The master will only send an operation status update to a framework if `operationId` is set, so it should be a required field, i.e., `const OperationID& operationId`.
- Gaston Kleiman
On March 2, 2018, 12:06 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63991/
> -----------------------------------------------------------
>
> (Updated March 2, 2018, 12:06 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, and Jie Yu.
>
>
> Bugs: MESOS-8190
> https://issues.apache.org/jira/browse/MESOS-8190
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helpers to create and forward offer operation updates.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 9a940db188436b9c5a39b0637cb4c15ee2ab5266
> src/common/protobuf_utils.cpp f516091e9208552488c154f34adde6f20d3413bf
> src/master/master.hpp 1fadbe61f774e8ca9926af4e39b3d6c8e24fe5df
> src/master/master.cpp 9cea7bb6a6ee8bf5f4226d07111bcfa6f5d3a88c
>
>
> Diff: https://reviews.apache.org/r/63991/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 63991: Added helpers to create and forward offer
operation updates.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 14, 2018, 3:56 p.m., Benjamin Bannier wrote:
> > src/common/protobuf_utils.hpp
> > Lines 165-170 (patched)
> > <https://reviews.apache.org/r/63991/diff/4/?file=1974376#file1974376line165>
> >
> > This looks unusual when compared to other helpers in this file which usually have arguments modelled pretty closely after the fields of message they produce. The function names usually also pretty directly reflect what kind of message they produce. This helper OTOH only can create a single flavor of `Scheduler::Event`.
> >
> > Given that this function does not abstract away any logic and will be used just a single helper added in the next patch, I'd suggest to just inline the creation of `Scheduler::Event` there for now and not add this helper.
Yea I think you're right, we can just inline this.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63991/#review199169
-----------------------------------------------------------
On March 13, 2018, 4:33 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63991/
> -----------------------------------------------------------
>
> (Updated March 13, 2018, 4:33 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, and Jie Yu.
>
>
> Bugs: MESOS-8190
> https://issues.apache.org/jira/browse/MESOS-8190
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helpers to create and forward offer operation updates.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 2ef1c9a4c2986e6f254e28387635bc01bb0b3259
> src/common/protobuf_utils.cpp 9c5fb97afb58f98013b79f3cbaea7dacc3603271
> src/master/master.hpp 8bf2c763dafdb7df55c46a56f2ff66f2a951d947
> src/master/master.cpp 223ebf29ac4dd1dea9080e4bef4b2d4d064d847f
>
>
> Diff: https://reviews.apache.org/r/63991/diff/4/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 63991: Added helpers to create and forward offer
operation updates.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63991/#review199169
-----------------------------------------------------------
src/common/protobuf_utils.hpp
Lines 165-170 (patched)
<https://reviews.apache.org/r/63991/#comment279464>
This looks unusual when compared to other helpers in this file which usually have arguments modelled pretty closely after the fields of message they produce. The function names usually also pretty directly reflect what kind of message they produce. This helper OTOH only can create a single flavor of `Scheduler::Event`.
Given that this function does not abstract away any logic and will be used just a single helper added in the next patch, I'd suggest to just inline the creation of `Scheduler::Event` there for now and not add this helper.
src/master/master.hpp
Lines 913-920 (patched)
<https://reviews.apache.org/r/63991/#comment279466>
Do we plan to use this same helper to forward operation updates from agents and rps as well in the future? Wouldn't we want to pass in the updates instead of exploding fields into separate args?
src/master/master.cpp
Lines 7979-7985 (patched)
<https://reviews.apache.org/r/63991/#comment279467>
Shouldn't we add this operation into some operation status update manager?
One possible way for that would be to just accept an operation update as arg here and deal with the distinction between master-generated and other updates elsewhere, e.g., in https://reviews.apache.org/r/63992/.
- Benjamin Bannier
On March 13, 2018, 5:33 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63991/
> -----------------------------------------------------------
>
> (Updated March 13, 2018, 5:33 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, and Jie Yu.
>
>
> Bugs: MESOS-8190
> https://issues.apache.org/jira/browse/MESOS-8190
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helpers to create and forward offer operation updates.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 2ef1c9a4c2986e6f254e28387635bc01bb0b3259
> src/common/protobuf_utils.cpp 9c5fb97afb58f98013b79f3cbaea7dacc3603271
> src/master/master.hpp 8bf2c763dafdb7df55c46a56f2ff66f2a951d947
> src/master/master.cpp 223ebf29ac4dd1dea9080e4bef4b2d4d064d847f
>
>
> Diff: https://reviews.apache.org/r/63991/diff/4/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 63991: Added helpers to create and forward offer
operation updates.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63991/
-----------------------------------------------------------
(Updated March 13, 2018, 4:33 p.m.)
Review request for mesos, Benjamin Bannier, Gaston Kleiman, and Jie Yu.
Bugs: MESOS-8190
https://issues.apache.org/jira/browse/MESOS-8190
Repository: mesos
Description
-------
Added helpers to create and forward offer operation updates.
Diffs (updated)
-----
src/common/protobuf_utils.hpp 2ef1c9a4c2986e6f254e28387635bc01bb0b3259
src/common/protobuf_utils.cpp 9c5fb97afb58f98013b79f3cbaea7dacc3603271
src/master/master.hpp 8bf2c763dafdb7df55c46a56f2ff66f2a951d947
src/master/master.cpp 223ebf29ac4dd1dea9080e4bef4b2d4d064d847f
Diff: https://reviews.apache.org/r/63991/diff/4/
Changes: https://reviews.apache.org/r/63991/diff/3-4/
Testing
-------
make check
Thanks,
Greg Mann
Re: Review Request 63991: Added helpers to create and forward offer
operation updates.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63991/
-----------------------------------------------------------
(Updated March 2, 2018, 10:53 p.m.)
Review request for mesos, Benjamin Bannier, Gaston Kleiman, and Jie Yu.
Bugs: MESOS-8190
https://issues.apache.org/jira/browse/MESOS-8190
Repository: mesos
Description
-------
Added helpers to create and forward offer operation updates.
Diffs
-----
src/common/protobuf_utils.hpp 9a940db188436b9c5a39b0637cb4c15ee2ab5266
src/common/protobuf_utils.cpp f516091e9208552488c154f34adde6f20d3413bf
src/master/master.hpp 1fadbe61f774e8ca9926af4e39b3d6c8e24fe5df
src/master/master.cpp 9cea7bb6a6ee8bf5f4226d07111bcfa6f5d3a88c
Diff: https://reviews.apache.org/r/63991/diff/3/
Testing
-------
make check
Thanks,
Greg Mann
Re: Review Request 63991: Added helpers to create and forward offer
operation updates.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63991/
-----------------------------------------------------------
(Updated March 2, 2018, 10:52 p.m.)
Review request for mesos, Benjamin Bannier, Gaston Kleiman, and Jie Yu.
Changes
-------
Rebase.
Bugs: MESOS-8190
https://issues.apache.org/jira/browse/MESOS-8190
Repository: mesos
Description
-------
Added helpers to create and forward offer operation updates.
Diffs (updated)
-----
src/common/protobuf_utils.hpp 9a940db188436b9c5a39b0637cb4c15ee2ab5266
src/common/protobuf_utils.cpp f516091e9208552488c154f34adde6f20d3413bf
src/master/master.hpp 1fadbe61f774e8ca9926af4e39b3d6c8e24fe5df
src/master/master.cpp 9cea7bb6a6ee8bf5f4226d07111bcfa6f5d3a88c
Diff: https://reviews.apache.org/r/63991/diff/3/
Changes: https://reviews.apache.org/r/63991/diff/2-3/
Testing
-------
make check
Thanks,
Greg Mann
Re: Review Request 63991: Added helpers to create and forward offer
operation updates.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63991/
-----------------------------------------------------------
(Updated March 2, 2018, 8:06 a.m.)
Review request for mesos, Benjamin Bannier, Gaston Kleiman, and Jie Yu.
Changes
-------
Rebase.
Bugs: MESOS-8190
https://issues.apache.org/jira/browse/MESOS-8190
Repository: mesos
Description
-------
Added helpers to create and forward offer operation updates.
Diffs (updated)
-----
src/common/protobuf_utils.hpp 9a940db188436b9c5a39b0637cb4c15ee2ab5266
src/common/protobuf_utils.cpp f516091e9208552488c154f34adde6f20d3413bf
src/master/master.hpp 1fadbe61f774e8ca9926af4e39b3d6c8e24fe5df
src/master/master.cpp 9cea7bb6a6ee8bf5f4226d07111bcfa6f5d3a88c
Diff: https://reviews.apache.org/r/63991/diff/2/
Changes: https://reviews.apache.org/r/63991/diff/1-2/
Testing
-------
make check
Thanks,
Greg Mann