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