apollo_protobuf: add prost codec for length-delimited messages#11063
Conversation
b4c0020 to
fc2a8bb
Compare
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware made 4 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @noamsp-starkware and @ShahakShama).
crates/apollo_protobuf/src/codec_test.rs line 77 at r4 (raw file):
Previously, ShahakShama wrote…
No. I meant that i is equal to the last byte of a message
I really don't understand. Are you on the latest version of the PR? Can you give a psudo-code example?
crates/apollo_protobuf/src/codec_test.rs line 138 at r4 (raw file):
Previously, ShahakShama wrote…
Where is it?
I don't understand this? Is this a case that is not covered by the byte by byte implementation?
crates/apollo_protobuf/src/codec_test.rs line 38 at r5 (raw file):
Previously, ShahakShama wrote…
20ms is too much (think that if we have 10000 of tests which there's a chance we have, it will take 200 seconds to run the tests if the bar for all of them is 20ms)
- Reduce test_multiple_messages_byte_by_byte constant from 15 to let's say 10
- Remove the 10k case from test_encode_decode_multiple_messages_all_at_once::case_6
- Remove the 100k case from test_encode_decode_various_sizes
Done.
crates/apollo_protobuf/src/codec_test.rs line 56 at r6 (raw file):
Previously, ShahakShama wrote…
Don't have the same message all over the test. Also, consider a changing message len. For example, message len = i % 100
This change will probably require you to have a smaller num_messages, which is ok. I prefer an interesting test over a big test
Done.
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed 3 files and all commit messages, made 2 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noamsp-starkware and @sirandreww-starkware).
crates/apollo_protobuf/src/codec_test.rs line 77 at r4 (raw file):
Previously, sirandreww-starkware (Andrew Luka) wrote…
I really don't understand. Are you on the latest version of the PR? Can you give a psudo-code example?
Here's an ugly version of what I want. Please prettify it and add it
let mut expected_next_message_index_in_full_buf = sent_messages.first().unwrap().len();
for i in 0..total_len {
...
if partial_buf.is_empty() {
assert_eq!(i, expected_next_message_index_in_full_buf);
expected_next_message_index_in_full_buf += sent_messages[received_messages + 1].len();
...crates/apollo_protobuf/src/codec_test.rs line 138 at r4 (raw file):
Previously, sirandreww-starkware (Andrew Luka) wrote…
I don't understand this? Is this a case that is not covered by the byte by byte implementation?
Oh right
fc2a8bb to
1c7eae9
Compare
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noamsp-starkware and @ShahakShama).
crates/apollo_protobuf/src/codec_test.rs line 77 at r4 (raw file):
Previously, ShahakShama wrote…
Here's an ugly version of what I want. Please prettify it and add it
let mut expected_next_message_index_in_full_buf = sent_messages.first().unwrap().len(); for i in 0..total_len { ... if partial_buf.is_empty() { assert_eq!(i, expected_next_message_index_in_full_buf); expected_next_message_index_in_full_buf += sent_messages[received_messages + 1].len(); ...
Done.
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noamsp-starkware).
1c7eae9 to
b835990
Compare
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware partially reviewed 9 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noamsp-starkware).

No description provided.