Skip to content

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

Merged
piraka9011 merged 7 commits intoros2:masterfrom
aws-ros-dev:compression/reader
Jan 10, 2020
Merged

[Compression - 8] Enable reader to read from compressed files/messages#246
piraka9011 merged 7 commits intoros2:masterfrom
aws-ros-dev:compression/reader

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
zmichaels11
zmichaels11 suggested changes Jan 7, 2020
View reviewed changes
@piraka9011 piraka9011 removed the request for review from Karsten1987 January 7, 2020 17:47
dabonnie
dabonnie reviewed Jan 7, 2020
View reviewed changes
@piraka9011
Copy link
Contributor Author

piraka9011 commented Jan 7, 2020

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
zmichaels11
zmichaels11 suggested changes Jan 8, 2020
View reviewed changes
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
Constness and counters
b7912ab 
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
zmichaels11
zmichaels11 suggested changes Jan 9, 2020
View reviewed changes
Simplify and enhance tests
3ce2779 
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
@piraka9011
Copy link
Contributor Author

piraka9011 commented Jan 9, 2020

@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

prajakta-gokhale commented Jan 9, 2020

  • 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

piraka9011 commented Jan 9, 2020

@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!

@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

piraka9011 commented Jan 15, 2020

@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

dirk-thomas commented Jan 15, 2020

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

@Karsten1987 Karsten1987 Awaiting requested review from Karsten1987

3 more reviewers

@zmichaels11 zmichaels11 zmichaels11 approved these changes

@dabonnie dabonnie dabonnie approved these changes

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

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

needs review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@piraka9011 @prajakta-gokhale @dirk-thomas @zmichaels11 @dabonnie