Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use encode_varint to encode vector length in DefaultAsyncBuffer and RecordSets. #752

Merged
merged 1 commit into from
Feb 9, 2021

Conversation

simlay
Copy link
Contributor

@simlay simlay commented Feb 5, 2021

Closes #706.

@simlay simlay force-pushed the use-varint-for-default-async-buffer branch from a71529d to ca1f709 Compare February 6, 2021 00:17
@simlay simlay marked this pull request as ready for review February 8, 2021 03:04
@simlay simlay requested a review from sehz February 8, 2021 03:06
@sehz
Copy link
Contributor

sehz commented Feb 8, 2021

rebase

#[test]
fn test_decode_batch_truncation() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I deleted the truncation stuff because it seems that it's a bad use case to take the bytes and modify them directy. Thoughts @sehz?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL. That is to simulate where SPU truncates output bytes in order to comply with max bytes per response. This is critical test to demonstrate receivers can recover much of batches even thought some of the batches are incomplete.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of the reason I removed it was because I couldn't figure out how to change the truncation bytes. Given that the SPU truncates it, that'll also have to change (probably why the build is currently failing) so should we keep the encoder changes to RecordSet or not?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make it changes only to DefaultAsyncBuffer

@simlay simlay force-pushed the use-varint-for-default-async-buffer branch from 7080646 to c2ede31 Compare February 8, 2021 03:08
@simlay simlay changed the title Initial stuff for varint work Use encode_varint to encode vector length in DefaultAsyncBuffer and RecordSets. Feb 8, 2021
#[test]
fn test_decode_batch_truncation() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL. That is to simulate where SPU truncates output bytes in order to comply with max bytes per response. This is critical test to demonstrate receivers can recover much of batches even thought some of the batches are incomplete.

@simlay
Copy link
Contributor Author

simlay commented Feb 8, 2021

LOL. That is to simulate where SPU truncates output bytes in order to comply with max bytes per response. This is critical test to demonstrate receivers can recover much of batches even thought some of the batches are incomplete.

Well then that clears up the importance of it.

@simlay simlay force-pushed the use-varint-for-default-async-buffer branch from c2ede31 to 8915767 Compare February 8, 2021 17:20
@simlay simlay requested a review from sehz February 8, 2021 17:37
@simlay simlay force-pushed the use-varint-for-default-async-buffer branch from 8915767 to da4453d Compare February 8, 2021 17:54
Copy link
Contributor

@sehz sehz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crate version need to be updated as well min platform version

@simlay
Copy link
Contributor Author

simlay commented Feb 8, 2021

crate version need to be updated as well min platform version

I'm pretty sure I bumped all the right versions. It's kind of a breaking change to all things depending on the dataplane-protocol crate which means there's a breaking change to just about every crate in the project so that's nice.

- Outdated Show resolved Hide resolved
@simlay simlay force-pushed the use-varint-for-default-async-buffer branch 6 times, most recently from 704f955 to 66d7e4e Compare February 9, 2021 01:58
@simlay simlay force-pushed the use-varint-for-default-async-buffer branch from 66d7e4e to a5906d3 Compare February 9, 2021 02:03
@sehz sehz merged commit 073a9e5 into infinyon:master Feb 9, 2021
simlay added a commit that referenced this pull request Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants