-
Notifications
You must be signed in to change notification settings - Fork 37
refactor to rewrite shard header with footer_len set to 0 #551
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
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.
LGTM, minor comment.
Maybe add a test?
|
P.S. Also tested with this version that uploads work and CAS does accept this |
|
|
||
| use super::*; | ||
|
|
||
| fn test_read_shard_to_bytes_remove_footer() -> Result<()> { |
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.
Missing the #[test] attribute
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.
LGTM!
related to #547
When xet-core uploads the shard the header field where the footer length is specified is set to 200 where it should be 0 according to the specification.
Note: this value is ignored by the server today, but ideally we would set this right since it can be useful to know if there is a footer on the shard when reading the shard as a whole in a non-streaming fashion.