You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrei Sekretenko <as...@mesosphere.io> on 2019/12/02 11:46:52 UTC
Re: Review Request 71823: Simplified V0 -> V1 direct serialization
logic.
-----------------------------------------------------------
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
>
>
Re: Review Request 71823: Simplified V0 -> V1 direct serialization
logic.
Posted by Benjamin Mahler <bm...@apache.org>.
> 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
>
>