-
Notifications
You must be signed in to change notification settings - Fork 32
block component: introduce wincode #615
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
Conversation
0c03888 to
cd6a2f7
Compare
cd6a2f7 to
ec10bcf
Compare
cpubot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few suggestions, and a question about LengthPrefixed. Will have more follow-up after I understand LengthPrefixed -- it probably shouldn't use SeqLen.
6683bdd to
b554bed
Compare
cpubot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
#### Problem and Summary of Changes PR #615 introduced `block_component_v2.rs`, which: - Is identical to `block_component.rs` from a functionality standpoint (demonstrated via tests) - Is written via `wincode`, resulting in far cleaner logic This PR: - Moves the contents of `block_component_v2.rs` into `block_component.rs`, removing the old `BlockComponent` implementation - Updates call-sites (all cosmetic changes)
Problem and Summary of Changes
block_component.rsis getting hard to manage. Let's usewincodeto solve this.Notes:
block_component_v2.rsnow has close to zero custom serde code, and is much simpler to maintain. We have tests to validate that behavior identically matches that ofblock_component.rs.Once this lands, we'll entirely remove
block_component.rs.This PR does not use
wincodeto serdeEntryBatch-es just yet. We'll do this as soon as we upstream toagave. I tried cherry-picking the PRs to getEntryto implementSchemaRead/SchemaWrite, but ran into some versioning problems. Could probably get this to work with some effort, but there's no point, since we'll be upstreaming soon anyways.