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] Add a SequentialCompressionReader #258

Merged
merged 4 commits into from
Jan 17, 2020
Merged

[compression] Add a SequentialCompressionReader #258

merged 4 commits into from
Jan 17, 2020

Conversation

piraka9011
Copy link
Contributor

Implements a SequentialCompressionReader that uses zstd to both decompress files and read them sequentially.

Depends on #257.

@piraka9011 piraka9011 changed the title Add a SequentialCompressionReader [WIP] Add a SequentialCompressionReader Jan 15, 2020
@piraka9011 piraka9011 force-pushed the sc-reader branch 3 times, most recently from cb7ed83 to fdf09f2 Compare January 16, 2020 16:56
@piraka9011 piraka9011 changed the title [WIP] Add a SequentialCompressionReader Add a SequentialCompressionReader Jan 16, 2020
@piraka9011 piraka9011 marked this pull request as ready for review January 16, 2020 16:56
emersonknapp
emersonknapp reviewed Jan 16, 2020
View reviewed changes
Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

This looks file overall, but I'm concerned about the amount of copied code - what do you think?

rosbag2_compression/include/rosbag2_compression/sequential_compression_reader.hpp
*
* \return true if there are still files to read in the list
*/
virtual bool has_next_file() const;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this is only the second time we're writing this pattern, but the sequential file URI logic is a decent amount of copy-pasted code. It may be worth a refactor to delegate functionality to a common sequential file handler.

Copy link
Contributor Author

@piraka9011 piraka9011 Jan 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I agree that it's a lot of similar code.
The problem with the sequential reader is that some of the functionality is private and not exposed in its public API. Moreover that's for another PR.

If we find ourselves doing this again though, then yes a refactor is in order.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Rule of 3 is my point exactly. And I would not suggest trying to use the other thing's private API. I would suggest pulling all the sequential file handling logic out into a totally separate thing that both use

Copy link
Contributor

Choose a reason for hiding this comment

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

Splitting SequentialReader may be a step in resolving Issue #213 since SequentialCompressionReader requires a metadata file.

I think there will be refactoring in the future where SequentialReader may split again into a specialized form for ROS1 bagfile support. If that happens, I think it would be more beneficial to refactor then.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why can't this class inherit from the SequentialReader?

@piraka9011
Copy link
Contributor Author

piraka9011 commented Jan 17, 2020 •
edited
Loading

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

@zmichaels11
Add a SequentialCompressionReader
236827c 
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
thomas-moulard
thomas-moulard approved these changes Jan 17, 2020
View reviewed changes
Copy link
Contributor

@thomas-moulard thomas-moulard left a comment

Choose a reason for hiding this comment

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

mainly LGTM, take a look at my comment on the decompressor.

mm318
mm318 reviewed Jan 17, 2020
View reviewed changes
rosbag2_compression/src/rosbag2_compression/sequential_compression_reader.cpp

void SequentialCompressionReader::set_current_file(const std::string & file)
{
*current_file_iterator_ = file;
Copy link
Member

Choose a reason for hiding this comment

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

I think it's much better to keep an index into file_paths_ than to keep an iterator. An iterator is just as bad as a raw pointer, subject to invalidation. While it's easy to compare the iterator to file_paths_.end(), it's hard to check if an iterator is indeed somewhere between file_paths_.begin() and file_paths_.end().

@zmichaels11
Apply suggestions
58c7629 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Copy link
Contributor

@ros2/aws-oncall please rerun CI

@piraka9011
Copy link
Contributor Author

piraka9011 commented Jan 17, 2020 •
edited by zmichaels11
Loading

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

@zmichaels11 zmichaels11 mentioned this pull request Jan 17, 2020
Merged
@zmichaels11
Copy link
Contributor

All CI is green.
@Karsten1987 any last minute comments before I go ahead and merge?

@zmichaels11 zmichaels11 mentioned this pull request Jan 17, 2020
Merged
Karsten1987
Karsten1987 reviewed Jan 17, 2020
View reviewed changes
Copy link
Collaborator

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

It's mainly new code, so it looks okay to me.

I wonder though why you couldn't inherit from the SequentialReader class to reduce code duplication.

rosbag2_compression/include/rosbag2_compression/sequential_compression_reader.hpp
*
* \return true if there are still files to read in the list
*/
virtual bool has_next_file() const;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why can't this class inherit from the SequentialReader?

@zmichaels11
Add new lines
e1c3900 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@piraka9011
Copy link
Contributor Author

Why can't this class inherit from the SequentialReader?

There's some private methods that need to be updated and/or refactored to support compression.
That's something we can address when we work on aws-roadmap#154

@zmichaels11
Replace asserts with throws
82f0ba1 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11 zmichaels11 merged commit 690d1e7 into ros2:master Jan 17, 2020
@zmichaels11 zmichaels11 deleted the sc-reader branch January 17, 2020 23:28
@zmichaels11 zmichaels11 changed the title Add a SequentialCompressionReader [compression] Add a SequentialCompressionReader Jan 22, 2020
@zmichaels11 zmichaels11 mentioned this pull request Jan 22, 2020
Closed
2 tasks
@jhdcs jhdcs mentioned this pull request Jan 14, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emersonknapp emersonknapp emersonknapp left review comments

@Karsten1987 Karsten1987 Karsten1987 left review comments

@mm318 mm318 mm318 left review comments

@zmichaels11 zmichaels11 zmichaels11 approved these changes

@thomas-moulard thomas-moulard thomas-moulard approved these changes

@dabonnie dabonnie Awaiting requested review from dabonnie

@prajakta-gokhale prajakta-gokhale Awaiting requested review from prajakta-gokhale

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

Successfully merging this pull request may close these issues.
6 participants
@piraka9011 @zmichaels11 @thomas-moulard @emersonknapp @Karsten1987 @mm318