----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71823/#review218874 -----------------------------------------------------------
Fix it, then Ship it! Overall looks good and makes things much more readable! The only general concern that I see is the notice in the `wire_format_lite.h` that states that `WireFormatLite` "must not be called directly by clients." Are we OK with potentially breaking use with non-bundled protobuf in future? (Not sure how frequently Mesos is used with non-bundled protobuf and how broken working with non-bundled protobuf already is.) src/common/http.hpp Lines 184 (patched) <https://reviews.apache.org/r/71823/#comment306787> Hmm... seeing this wrapper here somewhat surprises me, what is the reason for placing it here? If this is an implementation detail of `Master::Http::getSomething()`, why not just put it into `http.cpp`? If you intend this as something more general, then why not `src/common/protobuf_utils` or some other location with more general-purpose utilites? If there really is a reason for this wrapper to be here, it is probably worth a brief comment (unless this reason will become obvious in some patches that will follow soon). Also, a class that has nothing except for a single static method and that is not used as a type name anywhere, makes me wonder why it is a class. Is it for readability purposes, or am I missing something? (I think it actually improves readability of `http.cpp`, but might confuse readers here. Probably worth a brief comment too.) - Andrei Sekretenko On Nov. 26, 2019, 9:06 p.m., Benjamin Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71823/ > ----------------------------------------------------------- > > (Updated Nov. 26, 2019, 9:06 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 > ------- > > There are higher level utility functions that can be used to make > the code simpler and less error prone. > > Also, while not a bug, we should be using WriteBytes instead of > WriteString for writing submessage bytes. > > > Diffs > ----- > > src/common/http.hpp b9ab561e11d52b776a283687b3a19aa1b7942522 > src/master/http.cpp e03655863ea2d4e4464b3d14b359de3d7f059778 > > > Diff: https://reviews.apache.org/r/71823/diff/1/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Mahler > >
