You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Mahler <bm...@apache.org> on 2020/01/30 21:31:38 UTC
Review Request 72066: Improved performance of v1 agent operator API
GET_TASKS call.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72066/
-----------------------------------------------------------
Review request for mesos, Andrei Sekretenko and Greg Mann.
Bugs: MESOS-10026
https://issues.apache.org/jira/browse/MESOS-10026
Repository: mesos
Description
-------
This follow the same approach used for the master's v1 calls:
https://github.com/apache/mesos/commit/d7dd4d0e8493331d7b7a21b504eb
https://github.com/apache/mesos/commit/3dda3622f5ed01e8c132dc5ca594
That is, serializing directly to protobuf or json from the in-memory
v0 state.
Diffs
-----
src/slave/http.hpp 0afdad9479f0cc2c94452b6b1f2289dd6ea01494
src/slave/http.cpp 04ad0d816618a1880913857a6f0ff38c4643c488
Diff: https://reviews.apache.org/r/72066/diff/1/
Testing
-------
make check
Thanks,
Benjamin Mahler
Re: Review Request 72066: Improved performance of v1 agent operator
API GET_TASKS call.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Feb. 24, 2020, 7:28 p.m., Andrei Sekretenko wrote:
> > src/slave/http.cpp
> > Lines 2325-2351 (patched)
> > <https://reviews.apache.org/r/72066/diff/1/?file=2210099#file2210099line2325>
> >
> > Similarly to r72065 - is it possible to factor this out to make clear that framework/executor lists in json and protobuf code are composed in indentical way?
Yes, we definitely have a lot of these at this point! I will look into cleaning this up after these patches. It also makes me wonder if the existing code makes sense. For example, if I'm viewing executors, should the (intermediate) framework list construction go through VIEW_FRAMEWORK approval? What would it mean if I could VIEW_EXECUTOR but I can't VIEW_FRAMEWORK for its framework? Should I be able to view the executor in that case? Not sure.. but it has implications on the refactoring.
> On Feb. 24, 2020, 7:28 p.m., Andrei Sekretenko wrote:
> > src/slave/http.cpp
> > Lines 2332 (patched)
> > <https://reviews.apache.org/r/72066/diff/1/?file=2210099#file2210099line2332>
> >
> > newline?
I cut the newlines out to make it clearer that the block was building the vector, on all of these similar cases. This seemed clearer, or a lambda could be used to better scope it, but ultimately we'll probably add a helper for this.
> On Feb. 24, 2020, 7:28 p.m., Andrei Sekretenko wrote:
> > src/slave/http.cpp
> > Lines 2379 (patched)
> > <https://reviews.apache.org/r/72066/diff/1/?file=2210099#file2210099line2379>
> >
> > Given that our styleguide builds on top of Google's C++ styleguide, shouldn't `using` be preferred over `typedef` for type aliases?
yes probably! we started using that at the top of files in lieu of typedefs but we haven't made use of them for the foreach workaround, I will leave as is since I'm just moving the code but would be nice to sweep the code base for this change!
> On Feb. 24, 2020, 7:28 p.m., Andrei Sekretenko wrote:
> > src/slave/http.cpp
> > Lines 2445-2446 (patched)
> > <https://reviews.apache.org/r/72066/diff/1/?file=2210099#file2210099line2445>
> >
> > CHECK_NOTNULL when calling `approved()`?
I will actually remove these, we're inconsistently using them and it's a pain to actually use them before every pointer de-reference in all of this code.
> On Feb. 24, 2020, 7:28 p.m., Andrei Sekretenko wrote:
> > src/slave/http.cpp
> > Lines 2461 (patched)
> > <https://reviews.apache.org/r/72066/diff/1/?file=2210099#file2210099line2461>
> >
> > Technically, shared_ptr can also store nullptr - why have CHECK_NOTNULL in the loop before and don't have it here?
See comment above about the inconsistency.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72066/#review219626
-----------------------------------------------------------
On Jan. 30, 2020, 9:31 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72066/
> -----------------------------------------------------------
>
> (Updated Jan. 30, 2020, 9:31 p.m.)
>
>
> Review request for mesos, Andrei Sekretenko and Greg Mann.
>
>
> Bugs: MESOS-10026
> https://issues.apache.org/jira/browse/MESOS-10026
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This follow the same approach used for the master's v1 calls:
>
> https://github.com/apache/mesos/commit/d7dd4d0e8493331d7b7a21b504eb
> https://github.com/apache/mesos/commit/3dda3622f5ed01e8c132dc5ca594
>
> That is, serializing directly to protobuf or json from the in-memory
> v0 state.
>
>
> Diffs
> -----
>
> src/slave/http.hpp 0afdad9479f0cc2c94452b6b1f2289dd6ea01494
> src/slave/http.cpp 04ad0d816618a1880913857a6f0ff38c4643c488
>
>
> Diff: https://reviews.apache.org/r/72066/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 72066: Improved performance of v1 agent operator
API GET_TASKS call.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72066/#review219626
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/http.cpp
Lines 2325-2351 (patched)
<https://reviews.apache.org/r/72066/#comment307833>
Similarly to r72065 - is it possible to factor this out to make clear that framework/executor lists in json and protobuf code are composed in indentical way?
src/slave/http.cpp
Lines 2332 (patched)
<https://reviews.apache.org/r/72066/#comment307838>
newline?
src/slave/http.cpp
Lines 2379 (patched)
<https://reviews.apache.org/r/72066/#comment307834>
Given that our styleguide builds on top of Google's C++ styleguide, shouldn't `using` be preferred over `typedef` for type aliases?
src/slave/http.cpp
Lines 2428 (patched)
<https://reviews.apache.org/r/72066/#comment307835>
`const Task*` here and below?
src/slave/http.cpp
Lines 2445-2446 (patched)
<https://reviews.apache.org/r/72066/#comment307836>
CHECK_NOTNULL when calling `approved()`?
src/slave/http.cpp
Lines 2461 (patched)
<https://reviews.apache.org/r/72066/#comment307837>
Technically, shared_ptr can also store nullptr - why have CHECK_NOTNULL in the loop before and don't have it here?
- Andrei Sekretenko
On Jan. 30, 2020, 9:31 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72066/
> -----------------------------------------------------------
>
> (Updated Jan. 30, 2020, 9:31 p.m.)
>
>
> Review request for mesos, Andrei Sekretenko and Greg Mann.
>
>
> Bugs: MESOS-10026
> https://issues.apache.org/jira/browse/MESOS-10026
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This follow the same approach used for the master's v1 calls:
>
> https://github.com/apache/mesos/commit/d7dd4d0e8493331d7b7a21b504eb
> https://github.com/apache/mesos/commit/3dda3622f5ed01e8c132dc5ca594
>
> That is, serializing directly to protobuf or json from the in-memory
> v0 state.
>
>
> Diffs
> -----
>
> src/slave/http.hpp 0afdad9479f0cc2c94452b6b1f2289dd6ea01494
> src/slave/http.cpp 04ad0d816618a1880913857a6f0ff38c4643c488
>
>
> Diff: https://reviews.apache.org/r/72066/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>