Re: Review Request 36677: Introduced 'recordio' encoding facilities to stout.
On July 23, 2015, 11:52 p.m., Benjamin Hindman wrote: 3rdparty/libprocess/3rdparty/stout/tests/recordio_tests.cpp, line 32 https://reviews.apache.org/r/36677/diff/3/?file=1019202#file1019202line32 Why not add these to try.hpp like we did with Option? Ben Mahler wrote: Decided to punt because I'm not sure whether equality should include the error message equality, thoughts? Hmm, good question. I might start by not testing for error message equality to deter that use case, and see if it becomes necessary later. Either way, a comment above these operators as to why we're keeping them here instead of pulling them to try.hpp would be very helpful for a future code reader! - Benjamin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36677/#review92812 --- On July 22, 2015, 11:24 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36677/ --- (Updated July 22, 2015, 11:24 p.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, and Jie Yu. Bugs: MESOS-3067 https://issues.apache.org/jira/browse/MESOS-3067 Repository: mesos Description --- Note that most Record-IO encodings are used for file I/O and consequently use a fixed-size header to encode the record length. However, decoding a base-10 integer is more straightforward to implement in most languages, and so this was chosen instead. (Note that the Twitter streaming API uses the same technique for portability). Diffs - 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 2394b95462182273464f0847f416ad83c3b64485 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 8c75f6b28c18596018eaefe427b238424aae2fd9 3rdparty/libprocess/3rdparty/stout/include/stout/recordio.hpp PRE-CREATION 3rdparty/libprocess/3rdparty/stout/tests/recordio_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36677/diff/ Testing --- Added tests. Thanks, Ben Mahler
Re: Review Request 36677: Introduced 'recordio' encoding facilities to stout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36677/#review92626 --- nice tests! 3rdparty/libprocess/3rdparty/stout/include/stout/recordio.hpp (lines 54 - 61) https://reviews.apache.org/r/36677/#comment146834 Add comments! 3rdparty/libprocess/3rdparty/stout/include/stout/recordio.hpp (lines 72 - 73) https://reviews.apache.org/r/36677/#comment146836 Comment please. 3rdparty/libprocess/3rdparty/stout/include/stout/recordio.hpp (line 83) https://reviews.apache.org/r/36677/#comment146837 Can you add a comment about whether data can contain a partial record? - Vinod Kone On July 22, 2015, 6:33 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36677/ --- (Updated July 22, 2015, 6:33 a.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, and Jie Yu. Bugs: MESOS-3067 https://issues.apache.org/jira/browse/MESOS-3067 Repository: mesos Description --- Note that most Record-IO encodings are used for file I/O and consequently use a fixed-size header to encode the record length. However, decoding a base-10 integer is more straightforward to implement in most languages, and so this was chosen instead. (Note that the Twitter streaming API uses the same technique for portability). Diffs - 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 2394b95462182273464f0847f416ad83c3b64485 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 8c75f6b28c18596018eaefe427b238424aae2fd9 3rdparty/libprocess/3rdparty/stout/include/stout/recordio.hpp PRE-CREATION 3rdparty/libprocess/3rdparty/stout/tests/recordio_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36677/diff/ Testing --- Added tests. Thanks, Ben Mahler
Re: Review Request 36677: Introduced 'recordio' encoding facilities to stout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36677/ --- (Updated July 22, 2015, 6:33 a.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, and Jie Yu. Changes --- Removed libprocess code. Bugs: MESOS-3067 https://issues.apache.org/jira/browse/MESOS-3067 Repository: mesos Description --- Note that most Record-IO encodings are used for file I/O and consequently use a fixed-size header to encode the record length. However, decoding a base-10 integer is more straightforward to implement in most languages, and so this was chosen instead. (Note that the Twitter streaming API uses the same technique for portability). Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 2394b95462182273464f0847f416ad83c3b64485 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 8c75f6b28c18596018eaefe427b238424aae2fd9 3rdparty/libprocess/3rdparty/stout/include/stout/recordio.hpp PRE-CREATION 3rdparty/libprocess/3rdparty/stout/tests/recordio_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36677/diff/ Testing --- Added tests. Thanks, Ben Mahler
Re: Review Request 36677: Introduced 'recordio' encoding facilities to stout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36677/#review92556 --- Bad patch! Reviews applied: [36677] Failed command: ./support/apply-review.sh -n -r 36677 Error: 2015-07-22 06:28:31 URL:https://reviews.apache.org/r/36677/diff/raw/ [10398/10398] - 36677.patch [1] Successfully applied: Introduced 'recordio' encoding facilities to stout. Note that most Record-IO encodings are used for file I/O and consequently use a fixed-size header to encode the record length. However, decoding a base-10 integer is more straightforward to implement in most languages, and so this was chosen instead. (Note that the Twitter streaming API uses the same technique for portability). Review: https://reviews.apache.org/r/36677 Checking 3 files using filter --filter=-,+build/class,+build/deprecated,+build/endif_comment,+readability/todo,+readability/namespace,+runtime/vlog,+whitespace/blank_line,+whitespace/comma,+whitespace/end_of_line,+whitespace/ending_newline,+whitespace/forcolon,+whitespace/indent,+whitespace/line_length,+whitespace/operators,+whitespace/semicolon,+whitespace/tab,+whitespace/todo Total errors found: 0 ERROR: Commit spanning multiple projects. Please use separate commits for mesos, libprocess and stout. Paths grouped by project: stout: 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 3rdparty/libprocess/3rdparty/stout/include/stout/recordio.hpp 3rdparty/libprocess/3rdparty/stout/tests/recordio_tests.cpp libprocess: 3rdparty/libprocess/3rdparty/Makefile.am Failed to commit patch - Mesos ReviewBot On July 22, 2015, 6:09 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36677/ --- (Updated July 22, 2015, 6:09 a.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, and Jie Yu. Bugs: MESOS-3067 https://issues.apache.org/jira/browse/MESOS-3067 Repository: mesos Description --- Note that most Record-IO encodings are used for file I/O and consequently use a fixed-size header to encode the record length. However, decoding a base-10 integer is more straightforward to implement in most languages, and so this was chosen instead. (Note that the Twitter streaming API uses the same technique for portability). Diffs - 3rdparty/libprocess/3rdparty/Makefile.am 856c2b289451fd404b97285b825e72913feb2f04 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 2394b95462182273464f0847f416ad83c3b64485 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 8c75f6b28c18596018eaefe427b238424aae2fd9 3rdparty/libprocess/3rdparty/stout/include/stout/recordio.hpp PRE-CREATION 3rdparty/libprocess/3rdparty/stout/tests/recordio_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36677/diff/ Testing --- Added tests. Thanks, Ben Mahler
Re: Review Request 36677: Introduced 'recordio' encoding facilities to stout.
On July 22, 2015, 6:46 p.m., Vinod Kone wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/recordio.hpp, lines 54-61 https://reviews.apache.org/r/36677/diff/2/?file=1018410#file1018410line54 Add comments! Added a javadoc at the top of encoder and decoder to capture that these wrap individual record encoding / decoding. On July 22, 2015, 6:46 p.m., Vinod Kone wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/recordio.hpp, line 83 https://reviews.apache.org/r/36677/diff/2/?file=1018410#file1018410line83 Can you add a comment about whether data can contain a partial record? Re-phrased the documentation to make this more clear. - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36677/#review92626 --- On July 22, 2015, 6:33 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36677/ --- (Updated July 22, 2015, 6:33 a.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, and Jie Yu. Bugs: MESOS-3067 https://issues.apache.org/jira/browse/MESOS-3067 Repository: mesos Description --- Note that most Record-IO encodings are used for file I/O and consequently use a fixed-size header to encode the record length. However, decoding a base-10 integer is more straightforward to implement in most languages, and so this was chosen instead. (Note that the Twitter streaming API uses the same technique for portability). Diffs - 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 2394b95462182273464f0847f416ad83c3b64485 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 8c75f6b28c18596018eaefe427b238424aae2fd9 3rdparty/libprocess/3rdparty/stout/include/stout/recordio.hpp PRE-CREATION 3rdparty/libprocess/3rdparty/stout/tests/recordio_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36677/diff/ Testing --- Added tests. Thanks, Ben Mahler
Re: Review Request 36677: Introduced 'recordio' encoding facilities to stout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36677/#review92690 --- I'll add a note about completion semantics (surface an error when the stream ends with a partial record). This is not immediately useful but note that it is provided by joyent's http parser: https://github.com/joyent/http-parser (search for EOF). 3rdparty/libprocess/3rdparty/stout/include/stout/recordio.hpp (line 29) https://reviews.apache.org/r/36677/#comment146918 I'll add a TODO here that it would be nice to compose asynchronous streams of data, e.g.: Streamstring | DecoderEvent - StreamEvent But we don't have the necessary abstractions yet in libprocess. 3rdparty/libprocess/3rdparty/stout/include/stout/recordio.hpp (line 156) https://reviews.apache.org/r/36677/#comment146917 Add a note about memory allocation here, the string will retain its maximum size after a .clear() call: http://stackoverflow.com/q/25974085 - Ben Mahler On July 22, 2015, 11:24 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36677/ --- (Updated July 22, 2015, 11:24 p.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, and Jie Yu. Bugs: MESOS-3067 https://issues.apache.org/jira/browse/MESOS-3067 Repository: mesos Description --- Note that most Record-IO encodings are used for file I/O and consequently use a fixed-size header to encode the record length. However, decoding a base-10 integer is more straightforward to implement in most languages, and so this was chosen instead. (Note that the Twitter streaming API uses the same technique for portability). Diffs - 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 2394b95462182273464f0847f416ad83c3b64485 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 8c75f6b28c18596018eaefe427b238424aae2fd9 3rdparty/libprocess/3rdparty/stout/include/stout/recordio.hpp PRE-CREATION 3rdparty/libprocess/3rdparty/stout/tests/recordio_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36677/diff/ Testing --- Added tests. Thanks, Ben Mahler
