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

Rust: properly handle messages with log_time 0 #930

Merged
merged 4 commits into from
Jul 27, 2023

Conversation

mrkline
Copy link
Contributor

@mrkline mrkline commented Jul 25, 2023

Update the Writer tracking of message bounds to include log_time 0 as a valid time for bounds.

Additionally removes duplicated stats tracking data in the Chunk and overall MCAP writer.

Fix #924

@mrkline mrkline changed the title Log time0 Rust: properly handle messages with log_time 0 Jul 25, 2023
@mrkline mrkline marked this pull request as ready for review July 25, 2023 20:05
@defunctzombie
Copy link
Contributor

Not sure how the tests are organized but is there an existing test for checking this behavior? Or is this code tested with the conformance tests?

@@ -7,7 +7,7 @@ categories = [ "science::robotics", "compression" ]
repository = "https://github.com/foxglove/mcap"
documentation = "https://docs.rs/mcap"
readme = "README.md"
version = "0.7.0"
version = "0.7.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Will we need to do anything to release this version to crates?

Copy link
Contributor

Choose a reason for hiding this comment

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

The only previous instance of a "release" I've found is this tag: https://github.com/foxglove/mcap/releases/tag/releases%2Frust%2Fv0.6.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@james-rms is the owner of the crate (see https://crates.io/crates/mcap) so he'll have to push an update with cargo publish unless someone has his credentials sitting around. 😛

@mrkline
Copy link
Contributor Author

mrkline commented Jul 27, 2023

The existing unit tests should verify that file contents for the test outputs haven't changed (see https://github.com/foxglove/mcap/tree/main/rust/tests/data). A specific test that confirms #924 is gone wouldn't be a bad idea; I can add one later tonight.

@mrkline
Copy link
Contributor Author

mrkline commented Jul 27, 2023

Test added. Looks like there's some spurious failure about an API token in the docs-deploy CI step.

@defunctzombie defunctzombie merged commit cb36b5c into foxglove:main Jul 27, 2023
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

rust writer does not track log_time 0 in message_start_time
2 participants