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 71823: Simplified V0 -> V1 direct serialization logic.
Date Tue, 03 Dec 2019 04:13:47 GMT


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


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