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