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/26 21:30:54 UTC
Review Request 71824: Updated stout recordio encoder/decoder to be
lower-level.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71824/
-----------------------------------------------------------
Review request for mesos, Andrei Sekretenko and Greg Mann.
Bugs: MESOS-10026
https://issues.apache.org/jira/browse/MESOS-10026
Repository: mesos
Description
-------
These previously were templated to operate against typed records,
which imposes a few limitations. Namely, the caller cannot encode
records of different types without using some wrapper type.
Similarly, on the decoding side if the caller expects different
types of records based on some higher level protocol, it needs to
use a wrapper type that just stores the record's bytes for the
caller to decode.
The actual motivation for this patch came from a case where the
caller already has the record in bytes when encoding. We could
use Encoder<string> but it imposes an extra copy of each record,
which is not so straightforward to eliminate while keeping the
interface simple and functional for generic T records.
So, this patch makes the recordio encoder/decoder operate on
records as bytes, and callers can do whatever they wish with
the bytes.
Diffs
-----
3rdparty/stout/include/stout/recordio.hpp 9d226c21ed4ba7f31acf8db817470085792aca45
3rdparty/stout/tests/recordio_tests.cpp 177939427a7ef5f15cac318a98e40cf77b0b0ada
Diff: https://reviews.apache.org/r/71824/diff/1/
Testing
-------
Needs the following patch to compile mesos.
Thanks,
Benjamin Mahler
Re: Review Request 71824: Updated stout recordio encoder/decoder to be
lower-level.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71824/#review218888
-----------------------------------------------------------
Ship it!
Ship It!
- Andrei Sekretenko
On Dec. 3, 2019, 5:25 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71824/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2019, 5:25 a.m.)
>
>
> Review request for mesos, Andrei Sekretenko and Greg Mann.
>
>
> Bugs: MESOS-10026
> https://issues.apache.org/jira/browse/MESOS-10026
>
>
> Repository: mesos
>
>
> Description
> -------
>
> These previously were templated to operate against typed records,
> which imposes a few limitations. Namely, the caller cannot encode
> records of different types without using some wrapper type.
> Similarly, on the decoding side if the caller expects different
> types of records based on some higher level protocol, it needs to
> use a wrapper type that just stores the record's bytes for the
> caller to decode.
>
> The actual motivation for this patch came from a case where the
> caller already has the record in bytes when encoding. We could
> use Encoder<string> but it imposes an extra copy of each record,
> which is not so straightforward to eliminate while keeping the
> interface simple and functional for generic T records.
>
> So, this patch makes the recordio encoder/decoder operate on
> records as bytes, and callers can do whatever they wish with
> the bytes.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/recordio.hpp 9d226c21ed4ba7f31acf8db817470085792aca45
> 3rdparty/stout/tests/recordio_tests.cpp 177939427a7ef5f15cac318a98e40cf77b0b0ada
>
>
> Diff: https://reviews.apache.org/r/71824/diff/2/
>
>
> Testing
> -------
>
> Needs the following patch to compile mesos.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 71824: Updated stout recordio encoder/decoder to be
lower-level.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71824/
-----------------------------------------------------------
(Updated Dec. 3, 2019, 5:25 a.m.)
Review request for mesos, Andrei Sekretenko and Greg Mann.
Changes
-------
* Moved from Encoder to stateless encode function.
Bugs: MESOS-10026
https://issues.apache.org/jira/browse/MESOS-10026
Repository: mesos
Description
-------
These previously were templated to operate against typed records,
which imposes a few limitations. Namely, the caller cannot encode
records of different types without using some wrapper type.
Similarly, on the decoding side if the caller expects different
types of records based on some higher level protocol, it needs to
use a wrapper type that just stores the record's bytes for the
caller to decode.
The actual motivation for this patch came from a case where the
caller already has the record in bytes when encoding. We could
use Encoder<string> but it imposes an extra copy of each record,
which is not so straightforward to eliminate while keeping the
interface simple and functional for generic T records.
So, this patch makes the recordio encoder/decoder operate on
records as bytes, and callers can do whatever they wish with
the bytes.
Diffs (updated)
-----
3rdparty/stout/include/stout/recordio.hpp 9d226c21ed4ba7f31acf8db817470085792aca45
3rdparty/stout/tests/recordio_tests.cpp 177939427a7ef5f15cac318a98e40cf77b0b0ada
Diff: https://reviews.apache.org/r/71824/diff/2/
Changes: https://reviews.apache.org/r/71824/diff/1-2/
Testing
-------
Needs the following patch to compile mesos.
Thanks,
Benjamin Mahler
Re: Review Request 71824: Updated stout recordio encoder/decoder to be
lower-level.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Dec. 2, 2019, 12:49 p.m., Andrei Sekretenko wrote:
> > 3rdparty/stout/include/stout/recordio.hpp
> > Line 82 (original)
> > <https://reviews.apache.org/r/71824/diff/1/?file=2179381#file2179381line82>
> >
> > Now that Encoder has no state, did you consider dropping `class Encoder` altogether so that it does not create an illusion (in the places where it is used) that there might be some stored state?
> >
> > I.e. how likely do you think it is that we are going to add state to Encoder when we implement the zero-copy TODO above?
> >
> > My impression is that after that TODO the RecordIO encoding will still need no state. Something like this:
> >
> > `
> > namespace recordio {
> >
> > template <class TWriter>
> > bool encode(std::string&& record, TWriter& writer) {
> > // Write two strings atomically so that records produced
> > // by concurrent encoders, if there are any, do not interleave.
> > return writer.write({stringify(record.size()) + "\n", std::move(record)});
> > }
> >
> > `
Good point, it crossed my mind, but I figured we would want a zero-copy class, much like we did with jsonify:
```
recordio::Writer writer(destination);
writer.write(std::move(record1));
writer.write(std::move(record2));
```
(rather than having to pass the same argument in every time, which seems tedious)
Anyway, I'll make it a stateless encode function for now, since that's all we need. And we can figure out zero-copy later.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71824/#review218876
-----------------------------------------------------------
On Nov. 26, 2019, 9:30 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71824/
> -----------------------------------------------------------
>
> (Updated Nov. 26, 2019, 9:30 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
> -------
>
> These previously were templated to operate against typed records,
> which imposes a few limitations. Namely, the caller cannot encode
> records of different types without using some wrapper type.
> Similarly, on the decoding side if the caller expects different
> types of records based on some higher level protocol, it needs to
> use a wrapper type that just stores the record's bytes for the
> caller to decode.
>
> The actual motivation for this patch came from a case where the
> caller already has the record in bytes when encoding. We could
> use Encoder<string> but it imposes an extra copy of each record,
> which is not so straightforward to eliminate while keeping the
> interface simple and functional for generic T records.
>
> So, this patch makes the recordio encoder/decoder operate on
> records as bytes, and callers can do whatever they wish with
> the bytes.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/recordio.hpp 9d226c21ed4ba7f31acf8db817470085792aca45
> 3rdparty/stout/tests/recordio_tests.cpp 177939427a7ef5f15cac318a98e40cf77b0b0ada
>
>
> Diff: https://reviews.apache.org/r/71824/diff/1/
>
>
> Testing
> -------
>
> Needs the following patch to compile mesos.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 71824: Updated stout recordio encoder/decoder to be
lower-level.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71824/#review218876
-----------------------------------------------------------
3rdparty/stout/include/stout/recordio.hpp
Line 82 (original)
<https://reviews.apache.org/r/71824/#comment306789>
Now that Encoder has no state, did you consider dropping `class Encoder` altogether so that it does not create an illusion (in the places where it is used) that there might be some stored state?
I.e. how likely do you think it is that we are going to add state to Encoder when we implement the zero-copy TODO above?
My impression is that after that TODO the RecordIO encoding will still need no state. Something like this:
`
namespace recordio {
template <class TWriter>
bool encode(std::string&& record, TWriter& writer) {
// Write two strings atomically so that records produced
// by concurrent encoders, if there are any, do not interleave.
return writer.write({stringify(record.size()) + "\n", std::move(record)});
}
`
- Andrei Sekretenko
On Nov. 26, 2019, 9:30 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71824/
> -----------------------------------------------------------
>
> (Updated Nov. 26, 2019, 9:30 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
> -------
>
> These previously were templated to operate against typed records,
> which imposes a few limitations. Namely, the caller cannot encode
> records of different types without using some wrapper type.
> Similarly, on the decoding side if the caller expects different
> types of records based on some higher level protocol, it needs to
> use a wrapper type that just stores the record's bytes for the
> caller to decode.
>
> The actual motivation for this patch came from a case where the
> caller already has the record in bytes when encoding. We could
> use Encoder<string> but it imposes an extra copy of each record,
> which is not so straightforward to eliminate while keeping the
> interface simple and functional for generic T records.
>
> So, this patch makes the recordio encoder/decoder operate on
> records as bytes, and callers can do whatever they wish with
> the bytes.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/recordio.hpp 9d226c21ed4ba7f31acf8db817470085792aca45
> 3rdparty/stout/tests/recordio_tests.cpp 177939427a7ef5f15cac318a98e40cf77b0b0ada
>
>
> Diff: https://reviews.apache.org/r/71824/diff/1/
>
>
> Testing
> -------
>
> Needs the following patch to compile mesos.
>
>
> Thanks,
>
> Benjamin Mahler
>
>