> On Dec. 2, 2019, 11:46 a.m., Andrei Sekretenko wrote: > > 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.)
Yeah this came up when I originally was prototyping it. We pretty much have to use it, unfortunately. Note that we're already using it, e.g. `WireFormatLite::MakeTag(...)`. I suspect they will keep things inlined exposed in headers, and therefore as part of the installation, and visible for us to use. If not, we'll have to jump to a lower level of code or write our own little wrappers for said lower level code. > On Dec. 2, 2019, 11:46 a.m., Andrei Sekretenko wrote: > > src/common/http.hpp > > Lines 184 (patched) > > <https://reviews.apache.org/r/71823/diff/1/?file=2179379#file2179379line184> > > > > 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.) > If you intend this as something more general, then why not > src/common/protobuf_utils or some other location with more general-purpose > utilites? Good point, that seems like a better home. (And yes, I would expect this to be used in other places (e.g. the agent or common http.cpp files)). > 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? It's modeled after WireFormatLite which is a class with all statics, wanted to make this look like our extension of it (i.e. same calling style). Renamed it to WireFormatLite2 to make it a bit more clear that it's not "wrapping" something, but instead just our extension of these helpers. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71823/#review218874 ----------------------------------------------------------- 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 > >