> 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. > > Benjamin Mahler wrote: > 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.
Hmm.. if we want to make use of old `_getState`, I would propose two options: 1. Add a flag to the GET_STATE call which would switch it to the old serialization path, and add a local test that will compare results of two GET_STATEs, old path vs new. This might be a good aid in case someone needs to debug protobuf serialization again. Can imagine this happending when updating the bundled protobuf. Also, this will somewhat alleviate the concern about the unmaintained `_getState` eventually becoming buggy and non-usable. The coverage of such test will be rather poor, though. 2. Add a master flag that, as you suggested, will make it run both paths and crash the master if the results are different. Executed on a testing cluster, this will give a much better coverage. The price will be introducing more complexity into GET_STATE. - Andrei ----------------------------------------------------------- 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 > >
