You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Mahler <bm...@apache.org> on 2019/11/11 23:59:41 UTC
Review Request 71748: Support jsonifying v0 protobuf to v1 protobuf.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71748/
-----------------------------------------------------------
Review request for mesos, Andrei Sekretenko, Greg Mann, and Meng Zhu.
Bugs: MESOS-10026
https://issues.apache.org/jira/browse/MESOS-10026
Repository: mesos
Description
-------
This allows us to jsonify a v0 protobuf directly to a v1 protobuf
efficiently, with no need to `evolve()` the message (which is rather
expensive).
The way this works is by converting all "slave" and "SLAVE" strings
in fields and enum values, respectively, to "agent" and "AGENT".
Our current v0 to v1 conversion for the v1 operator API simply
serializes the v0 message and de-serializes into a v1 message, which
means all field tags and message structures are the same, except
for field names. The only difference with field names is the use
of "agent" in place of "slave".
Diffs
-----
src/common/http.hpp 062586c0a5a339c7e63c89a3a893ae015d3fd26e
src/common/http.cpp b79074f823d3bce2a15736c0ef4739ad13db8d9c
Diff: https://reviews.apache.org/r/71748/diff/1/
Testing
-------
Added a test in the subsequent patch.
Thanks,
Benjamin Mahler
Re: Review Request 71748: Support jsonifying v0 protobuf to v1
protobuf.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Nov. 19, 2019, 5:29 p.m., Andrei Sekretenko wrote:
> > src/common/http.cpp
> > Lines 177 (patched)
> > <https://reviews.apache.org/r/71748/diff/1/?file=2172059#file2172059line177>
> >
> > Is there any reason not to file a ticket for that and link it here?
> >
> > This patch unblocks a way to usable V1, but is *really* brittle (for example, I had to run diff on several pairs of `.proto`s and two C++ excerpts to review this). We must want to replace it with a permanent solution ;)
We have https://issues.apache.org/jira/browse/MESOS-8163 but sounds good to separate out the idea of holding an up-to-date v1 state object in memory, since that can happen without necessarily using a state actor to serve it. Filed https://issues.apache.org/jira/browse/MESOS-10040 and linked them up!
I will note that ticket in this patch.
> On Nov. 19, 2019, 5:29 p.m., Andrei Sekretenko wrote:
> > src/common/http.cpp
> > Lines 253 (patched)
> > <https://reviews.apache.org/r/71748/diff/1/?file=2172059#file2172059line253>
> >
> > There are at least three differences from the stout version that seem to be not covered by any Mesos test, including the new one you are adding in r71749.
> >
> > How about covering this one (and also repeated ENUM + map/singular MESSAGE below) in r71749?
> >
> >
> >
> > P.S. I'm quite surprised to see that our V1 API coverage is that poor... probably this only affects JSON?
I ended up just adding a .proto file (and pre-compiled, so no build process complication) in the next patch.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71748/#review218676
-----------------------------------------------------------
On Nov. 11, 2019, 11:59 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71748/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2019, 11:59 p.m.)
>
>
> Review request for mesos, Andrei Sekretenko, Greg Mann, and Meng Zhu.
>
>
> Bugs: MESOS-10026
> https://issues.apache.org/jira/browse/MESOS-10026
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This allows us to jsonify a v0 protobuf directly to a v1 protobuf
> efficiently, with no need to `evolve()` the message (which is rather
> expensive).
>
> The way this works is by converting all "slave" and "SLAVE" strings
> in fields and enum values, respectively, to "agent" and "AGENT".
>
> Our current v0 to v1 conversion for the v1 operator API simply
> serializes the v0 message and de-serializes into a v1 message, which
> means all field tags and message structures are the same, except
> for field names. The only difference with field names is the use
> of "agent" in place of "slave".
>
>
> Diffs
> -----
>
> src/common/http.hpp 062586c0a5a339c7e63c89a3a893ae015d3fd26e
> src/common/http.cpp b79074f823d3bce2a15736c0ef4739ad13db8d9c
>
>
> Diff: https://reviews.apache.org/r/71748/diff/1/
>
>
> Testing
> -------
>
> Added a test in the subsequent patch.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 71748: Support jsonifying v0 protobuf to v1
protobuf.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71748/#review218676
-----------------------------------------------------------
Fix it, then Ship it!
src/common/http.cpp
Lines 177 (patched)
<https://reviews.apache.org/r/71748/#comment306541>
Is there any reason not to file a ticket for that and link it here?
This patch unblocks a way to usable V1, but is *really* brittle (for example, I had to run diff on several pairs of `.proto`s and two C++ excerpts to review this). We must want to replace it with a permanent solution ;)
src/common/http.cpp
Lines 253 (patched)
<https://reviews.apache.org/r/71748/#comment306542>
There are at least three differences from the stout version that seem to be not covered by any Mesos test, including the new one you are adding in r71749.
How about covering this one (and also repeated ENUM + map/singular MESSAGE below) in r71749?
P.S. I'm quite surprised to see that our V1 API coverage is that poor... probably this only affects JSON?
- Andrei Sekretenko
On Nov. 11, 2019, 11:59 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71748/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2019, 11:59 p.m.)
>
>
> Review request for mesos, Andrei Sekretenko, Greg Mann, and Meng Zhu.
>
>
> Bugs: MESOS-10026
> https://issues.apache.org/jira/browse/MESOS-10026
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This allows us to jsonify a v0 protobuf directly to a v1 protobuf
> efficiently, with no need to `evolve()` the message (which is rather
> expensive).
>
> The way this works is by converting all "slave" and "SLAVE" strings
> in fields and enum values, respectively, to "agent" and "AGENT".
>
> Our current v0 to v1 conversion for the v1 operator API simply
> serializes the v0 message and de-serializes into a v1 message, which
> means all field tags and message structures are the same, except
> for field names. The only difference with field names is the use
> of "agent" in place of "slave".
>
>
> Diffs
> -----
>
> src/common/http.hpp 062586c0a5a339c7e63c89a3a893ae015d3fd26e
> src/common/http.cpp b79074f823d3bce2a15736c0ef4739ad13db8d9c
>
>
> Diff: https://reviews.apache.org/r/71748/diff/1/
>
>
> Testing
> -------
>
> Added a test in the subsequent patch.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 71748: Support jsonifying v0 protobuf to v1
protobuf.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71748/
-----------------------------------------------------------
(Updated Nov. 22, 2019, 6:16 p.m.)
Review request for mesos, Andrei Sekretenko, Greg Mann, and Meng Zhu.
Changes
-------
Split out the lower and upper cases.
Bugs: MESOS-10026
https://issues.apache.org/jira/browse/MESOS-10026
Repository: mesos
Description
-------
This allows us to jsonify a v0 protobuf directly to a v1 protobuf
efficiently, with no need to `evolve()` the message (which is rather
expensive).
The way this works is by converting all "slave" and "SLAVE" strings
in fields and enum values, respectively, to "agent" and "AGENT".
Our current v0 to v1 conversion for the v1 operator API simply
serializes the v0 message and de-serializes into a v1 message, which
means all field tags and message structures are the same, except
for field names. The only difference with field names is the use
of "agent" in place of "slave".
Diffs (updated)
-----
src/common/http.hpp 062586c0a5a339c7e63c89a3a893ae015d3fd26e
src/common/http.cpp b79074f823d3bce2a15736c0ef4739ad13db8d9c
Diff: https://reviews.apache.org/r/71748/diff/2/
Changes: https://reviews.apache.org/r/71748/diff/1-2/
Testing
-------
Added a test in the subsequent patch.
Thanks,
Benjamin Mahler