[compression] Add a SequentialCompressionReader by piraka9011 · Pull Request #258 · ros2/rosbag2

piraka9011 · 2020-01-15T20:55:08Z

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

Depends on #257.

rosbag2_compression/include/rosbag2_compression/sequential_compression_reader.hpp

rosbag2_compression/src/rosbag2_compression/sequential_compression_reader.cpp

rosbag2_compression/test/rosbag2_compression/test_sequential_compression_reader.cpp

emersonknapp

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

emersonknapp · 2020-01-16T21:40:38Z

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;


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.

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.

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

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.

Why can't this class inherit from the SequentialReader?

piraka9011 · 2020-01-17T16:24:27Z

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Anas Abou Allaban <allabana@amazon.com>

thomas-moulard

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

rosbag2_compression/src/rosbag2_compression/sequential_compression_reader.cpp

rosbag2_compression/include/rosbag2_compression/sequential_compression_reader.hpp

rosbag2_compression/src/rosbag2_compression/sequential_compression_reader.cpp

mm318 · 2020-01-17T18:28:28Z

rosbag2_compression/src/rosbag2_compression/sequential_compression_reader.cpp

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


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().

rosbag2_compression/src/rosbag2_compression/sequential_compression_reader.cpp

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

zmichaels11 · 2020-01-17T19:08:24Z

@ros2/aws-oncall please rerun CI

piraka9011 · 2020-01-17T19:10:45Z

Linux
Linux-aarch64
macOS
Windows

zmichaels11 · 2020-01-17T21:59:17Z

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

Karsten1987

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.

Karsten1987 · 2020-01-17T22:22:23Z

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;


Why can't this class inherit from the SequentialReader?

rosbag2_compression/src/rosbag2_compression/sequential_compression_reader.cpp

rosbag2_compression/test/rosbag2_compression/test_sequential_compression_reader.cpp

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

piraka9011 · 2020-01-17T22:51:49Z

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

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

piraka9011 changed the title ~~Add a SequentialCompressionReader~~ [WIP] Add a SequentialCompressionReader Jan 15, 2020

piraka9011 force-pushed the sc-reader branch 3 times, most recently from cb7ed83 to fdf09f2 Compare January 16, 2020 16:56

piraka9011 requested review from Karsten1987, dabonnie, prajakta-gokhale and zmichaels11 January 16, 2020 16:56

piraka9011 changed the title ~~[WIP] Add a SequentialCompressionReader~~ Add a SequentialCompressionReader Jan 16, 2020

piraka9011 marked this pull request as ready for review January 16, 2020 16:56

zmichaels11 suggested changes Jan 16, 2020

View reviewed changes

piraka9011 force-pushed the sc-reader branch from fdf09f2 to c3b07da Compare January 16, 2020 18:48

piraka9011 requested a review from zmichaels11 January 16, 2020 18:48

zmichaels11 approved these changes Jan 16, 2020

View reviewed changes

emersonknapp reviewed Jan 16, 2020

View reviewed changes

Add a SequentialCompressionReader

236827c

Signed-off-by: Anas Abou Allaban <allabana@amazon.com>

thomas-moulard approved these changes Jan 17, 2020

View reviewed changes

mm318 reviewed Jan 17, 2020

View reviewed changes

Apply suggestions

58c7629

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

zmichaels11 force-pushed the sc-reader branch from c3b07da to 58c7629 Compare January 17, 2020 19:07

zmichaels11 mentioned this pull request Jan 17, 2020

[compression] Add SequentialCompressionWriter #260

Merged

zmichaels11 mentioned this pull request Jan 17, 2020

Add splitting e2e tests #247

Merged

Karsten1987 reviewed Jan 17, 2020

View reviewed changes

Add new lines

e1c3900

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

Replace asserts with throws

82f0ba1

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

zmichaels11 force-pushed the sc-reader branch from 5b8feeb to 82f0ba1 Compare January 17, 2020 22:54

zmichaels11 merged commit 690d1e7 into ros2:master Jan 17, 2020

zmichaels11 deleted the sc-reader branch January 17, 2020 23:28

zmichaels11 changed the title ~~Add a SequentialCompressionReader~~ [compression] Add a SequentialCompressionReader Jan 22, 2020

zmichaels11 mentioned this pull request Jan 22, 2020

[Compression - 9] Enable writer to compress files/messages #250

Closed

2 tasks

jhdcs mentioned this pull request Jan 14, 2021

Introduce ros2 bag reindex #539

Closed

Conversation

piraka9011 commented Jan 15, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emersonknapp left a comment

Choose a reason for hiding this comment

Uh oh!

emersonknapp Jan 16, 2020

Choose a reason for hiding this comment

Uh oh!

piraka9011 Jan 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emersonknapp Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

zmichaels11 Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

Karsten1987 Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

piraka9011 commented Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomas-moulard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mm318 Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zmichaels11 commented Jan 17, 2020

Uh oh!

piraka9011 commented Jan 17, 2020 • edited by zmichaels11 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zmichaels11 commented Jan 17, 2020

Uh oh!

Karsten1987 left a comment

Choose a reason for hiding this comment

Uh oh!

Karsten1987 Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

piraka9011 commented Jan 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

piraka9011 Jan 16, 2020 •

edited

Loading

piraka9011 commented Jan 17, 2020 •

edited

Loading

piraka9011 commented Jan 17, 2020 •

edited by zmichaels11

Loading