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.

Sign up for GitHub

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

Jump to bottom

[Compression - 8] Enable reader to read from compressed files/messages #246

Merged
merged 7 commits into from
Jan 10, 2020
Merged

[Compression - 8] Enable reader to read from compressed files/messages #246

merged 7 commits into from
Jan 10, 2020

Conversation

piraka9011
Copy link
Contributor

@piraka9011 piraka9011 commented Jan 7, 2020 •
edited
Loading

This PR allows a Reader to decompress a file or serialized bag message by checking the bag file metadata.
It currently supports the Zstd decompressor only.
Creating a decompressor factory will be part of a later change.

Changes

  • Reader can now read from a compressed file or message.
  • Validate metadata keys for compression, supporting older metadata versions.
  • Unit test to check that compression occurs when specified in metadata.
  • Unit test to check that compression options can be converted b/w string/enum.

Part of aws-roadmap/#68.

@piraka9011 piraka9011 force-pushed the compression/reader branch 3 times, most recently from 7bad3bc to 8bb5ab2 Compare January 7, 2020 17:22
Enable reader to read from decompressed files & messages
4db4ef1 
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
Fix compression CMakeLists
b3be936 
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
@piraka9011 piraka9011 marked this pull request as ready for review January 7, 2020 17:31
@piraka9011 piraka9011 removed the request for review from Karsten1987 January 7, 2020 17:47
@piraka9011
Copy link
Contributor Author

Wondering if anyone had thoughts on making the new utility functions open_storage() and setup_compression() protected vs. private as well as naming.

@piraka9011 piraka9011 mentioned this pull request Jan 8, 2020
Closed
2 tasks
Add utility functions for reader, add tests for compression options, …
1d4f9ec 
…write docs, fix styles

Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
Add more unit tests, fix copyright years, nits
8bcd9a5 
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
dabonnie
dabonnie suggested changes Jan 9, 2020
View reviewed changes
rosbag2_cpp/src/rosbag2_cpp/readers/sequential_reader.cpp Show resolved Hide resolved
rosbag2_cpp/test/rosbag2_cpp/test_compression_options.cpp Outdated Show resolved Hide resolved
rosbag2_cpp/test/rosbag2_cpp/test_compression_options.cpp Outdated Show resolved Hide resolved
rosbag2_cpp/test/rosbag2_cpp/test_compression_options.cpp Outdated Show resolved Hide resolved
rosbag2_cpp/test/rosbag2_cpp/test_compression_options.cpp Outdated Show resolved Hide resolved
rosbag2_cpp/test/rosbag2_cpp/test_multifile_reader.cpp Outdated Show resolved Hide resolved
rosbag2_cpp/test/rosbag2_cpp/test_multifile_reader.cpp Outdated Show resolved Hide resolved
rosbag2_cpp/test/rosbag2_cpp/test_multifile_reader.cpp Outdated Show resolved Hide resolved
rosbag2_cpp/test/rosbag2_cpp/test_multifile_reader.cpp Show resolved Hide resolved
rosbag2_cpp/test/rosbag2_cpp/test_multifile_reader.cpp Show resolved Hide resolved
Constness and counters
b7912ab 
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
Simplify and enhance tests
3ce2779 
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
@piraka9011
Copy link
Contributor Author

@ros2/aws-oncall - please run this CI job
Gist: https://gist.githubusercontent.com/piraka9011/8028a6f7bc082936f2057ff5d3fba5c2/raw/fc3f28e4c525ba4f071965076204beaf54298942/ros2.repos
BUILD args: --packages-up-to rosbag2_cpp rosbag2_compression
TEST args: --packages-select rosbag2_cpp rosbag2_compression
Job: ci_launcher

@prajakta-gokhale
Copy link

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@piraka9011
Copy link
Contributor Author

piraka9011 commented Jan 9, 2020 •
edited
Loading

Linux and macOS are unstable due to an rcl issue.
Windows is complaining that it can't find rosbag2_cpp::compression_mode_to_string in test_multifile_reader.

Update:
Never forget your visibility macros for windows...

  • Windows Build Status

Fix windows build
1f9ab3e 
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
@piraka9011
Copy link
Contributor Author

@Karsten1987 could you PTAL?

prajakta-gokhale
prajakta-gokhale approved these changes Jan 10, 2020
View reviewed changes
Copy link

@prajakta-gokhale prajakta-gokhale left a comment •
edited
Loading

Choose a reason for hiding this comment

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

One minor nit. Looks good otherwise!

rosbag2_cpp/test/rosbag2_cpp/test_multifile_reader.cpp Show resolved Hide resolved
@piraka9011 piraka9011 merged commit bf04c04 into ros2:master Jan 10, 2020
@zmichaels11 zmichaels11 deleted the compression/reader branch January 10, 2020 18:27
@dirk-thomas
Copy link
Member

dirk-thomas commented Jan 15, 2020 •
edited
Loading

This PR introduced linter warnings which were even reported in the run CI build (see the latest Windows CI). Please fix this asap / by the end of day. (@ros2/aws-oncall FYI)

Also in the future do not merge PRs which clearly show regressions in the ran CI builds.

@piraka9011
Copy link
Contributor Author

@dirk-thomas Fix in #256.
For context, the build was unstable due to unrelated tests earlier so the uncrustify warnings were probably missed b/c of that.

@dirk-thomas
Copy link
Member

For context, the build was unstable due to unrelated tests earlier so the uncrustify warnings were probably missed b/c of that.

The Windows build had exact 4 failing tests - all related to this patch. So no unrelated tests obscuring the result in that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zmichaels11 zmichaels11 zmichaels11 approved these changes

@dabonnie dabonnie dabonnie approved these changes

@prajakta-gokhale prajakta-gokhale prajakta-gokhale approved these changes

@Karsten1987 Karsten1987 Awaiting requested review from Karsten1987

Assignees
No one assigned
Labels
needs review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@piraka9011 @prajakta-gokhale @dirk-thomas @zmichaels11 @dabonnie