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 71827: Improved operator api subscribe initial payload performance.
Date Tue, 03 Dec 2019 05:07:37 GMT


> On Dec. 2, 2019, 1:23 p.m., Andrei Sekretenko wrote:
> > src/master/http.cpp
> > Lines 444 (patched)
> > <https://reviews.apache.org/r/71827/diff/1/?file=2179402#file2179402line444>
> >
> >     Shouldn't we delete `_getState`, `_getFrameworks` and so on altogether? 
> >     
> >     They are dead code now: not used anywhere, not covered by tests and, essentialy,
not maintained. I don't think their correctness can be relied upon after this patch.
> >     
> >     Probably they should be removed in a separate next patch so that if we need
them back someday, the first step to getting them back will be reverting that patch.
> >     
> >     And the comments will need to be adjusted to not refer to them as well.

Good observation!

When writing the code, these were useful for debugging to compare the serialized bytes against
the expected serialized bytes (turns out the only bug was my lack of calling Trim() which
left trailing garbage bytes at the end of the string).

We should consider the case where we have a serialization bug, and we need to investigate
it. Having the expected vs actual will be helpful, but we won't have that if we remove the
object creation functions. If it's just as simple as a field being missing, then that's pretty
easy (probably just forgot to write it). But if it's more like the data has some garbage in
it, that may be harder to debug especially without the actual vs expected bytes handy. Thoughts
on this?

Another thing I was considering (but couldn't see a clean way to do) was to have our test
suite somehow assert that the two paths produce identical results, e.g.:

 - If running within tests, have the endpoint handler run both paths and CHECK they produce
the same serialized bytes.
 - Or, introduce some test visible function in the master that could get called at some point
in any test lifecycle (e.g. cleanup, or periodically) to assert that both produce equal results.

They both seemed pretty heavy to leave around in the code, and I became less paranoid about
serialization bugs, so I decided not to do these.


- Benjamin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71827/#review218878
-----------------------------------------------------------


On Nov. 26, 2019, 9:31 p.m., Benjamin Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71827/
> -----------------------------------------------------------
> 
> (Updated Nov. 26, 2019, 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 uses the same approach for other GET_ calls in MESOS-10026
> of directly serializing from in-memory state, rather than building
> up the temporary object and evolving it.
> 
> There is currently no benchmark but the improvement should closely
> resemble that of the GET_STATE call, for example:
> 
>     Before:
>     v0 '/state' response took 6.55 secs
>     v1 'GetState' application/x-protobuf response took 24.08 secs
>     v1 'GetState' application/json response took 22.76 secs
> 
>     After:
>     v0 '/state' response took 8.00 secs
>     v1 'GetState' application/x-protobuf response took 5.73 secs
>     v1 'GetState' application/json response took 9.62 secs
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp e03655863ea2d4e4464b3d14b359de3d7f059778 
>   src/master/master.hpp 93630421d58e6fd26566e81a23cd910957795665 
> 
> 
> Diff: https://reviews.apache.org/r/71827/diff/1/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>


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