mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 72066: Improved performance of v1 agent operator API GET_TASKS call.
Date Tue, 25 Feb 2020 02:08:19 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message