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 71824: Updated stout recordio encoder/decoder to be lower-level.
Date Tue, 03 Dec 2019 05:25:14 GMT


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


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