[compression] Enable compression through ros2bag cli by zmichaels11 · Pull Request #263 · ros2/rosbag2

zmichaels11 · 2020-01-22T17:05:33Z

Changes

Add --compression-mode and --compression-format arguments for ros2bag
rosbag2_transport instantiates SequentialCompressionReader or SequentialCompressionWriter if both compression-mode and compression-format are specified.

Testing

Add e2e test to verify bagfiles are compressed
Add e2e test to verify compressed bagfiles can be read

rosbag2_compression/CMakeLists.txt

piraka9011

Some minor changes but LGTM.

rosbag2_transport/include/rosbag2_transport/record_options.hpp

rosbag2_transport/src/rosbag2_transport/rosbag2_transport_python.cpp

dabonnie

Other than some nits seems fine.

zmichaels11 · 2020-01-24T00:31:10Z

Rebased onto master and resolved redundancy issue.

zmichaels11 · 2020-01-24T16:21:21Z

Any comments?
@Karsten1987
@thomas-moulard
@mm318
@emersonknapp

I'd like to merge this tonight if possible.

thomas-moulard · 2020-01-24T17:15:00Z

rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp

+  // TODO(zmichaels11): Remove when stop_execution properly SIGINT on Windows.
+  // This is required since stop_execution hard kills the proces on Windows,
+  // which prevents the metadata from being written.


Does this mean this feature is broken on Windows? Did we test this on this platform?

thomas-moulard · 2020-01-24T17:18:14Z

Somehow I can't approve this PR, but this LGTM. Let's make sure we test rosbag2 on windows and OS X too. The TODO in the test is worrying.

thomas-moulard

See previous comment.

zmichaels11 · 2020-01-24T20:06:32Z

@ros2/aws-oncall - please run this CI job
Gist: https://gist.githubusercontent.com/zmichaels11/0f8a5a8cf63f22aced353085c523f9ef/raw/77155bc93aa546d2e8c8b901f8d13e98785a4e42/ros2.repos
BUILD args: --packages-up-to rosbag2_tests rosbag2_compression rosbag2_transport rosbag2_cpp
TEST args: --packages-select rosbag2_tests rosbag2_compression rosbag2_transport rosbag2_cpp
Job: ci_launcher

zmichaels11 · 2020-01-24T20:08:01Z

Linux
Linux-aarch64
macOS
Windows

zmichaels11 · 2020-01-25T21:19:59Z

This was failing earlier due to me configuring the build wrong.
It looks like its passing now. I'm going to merge this tonight if all CI is green.

zmichaels11 · 2020-01-27T19:29:58Z

The e2e tests on windows are failing due to the SIGINT workaround for Windows.
The last bagfile is compressed in the dtor. Because of this, it gets skipped in record_end_to_end_test_with_zstd_file_compression. I'm going to rewrite that test to ensure the bagfile splits at least once and verify that the first bagfile is compressed properly.

Similarly, record_end_to_end_with_splitting_bagsize_split_is_at_least_specified_size is unable to pass since the last bagfile is not compressed. I'll relax this test and skip checking that the last bagfile is < uncompressed size.

Karsten1987

I do approve as I think the tests cover the new functionality (please make sure CI turns out to be green).
But I am getting a bit concerned about this windows workaround as well, as we copied this one all over the place now within the end to end tests.

rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp

zmichaels11 · 2020-01-28T00:32:54Z

Linux
Linux-aarch64
macOS
Windows

zmichaels11 · 2020-02-06T00:08:38Z

Ran successfully on local machine.

Windows
Edit: fixed linters
Windows

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

zmichaels11 · 2020-02-06T18:33:41Z

This is working now. I'm going to refactor the work done into single-focus PRs
Edit: Refactored into:

[compression] Improve logging in rosbag2_compression #287 - Improve logging in rosbag2_compression
[compression] Close storage before compression #284 - Close storage before compression

This PR should be rebased onto master once both of those PRs are merged in.

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

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

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

rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp

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

piraka9011 · 2020-02-07T19:44:07Z

Kicking off CI after rebase:

Linux
Linux-aarch64
macOS
Windows
Windows-container

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

zmichaels11 · 2020-02-07T21:42:44Z

It looks OSX test is failing due to the test fixture trying to access SQLite mid-compression.
Rewrote that test to not depend on querying SQLite while publishing.

Linux
Linux-aarch64
macOS

Edit: It looks like its passing. Rerunning CI on Linux only since the test is not included in Windows.

zmichaels11 force-pushed the compression-cli branch 2 times, most recently from a6c2213 to db94ab0 Compare January 22, 2020 17:43

zmichaels11 marked this pull request as ready for review January 22, 2020 18:07

zmichaels11 requested review from dabonnie, emersonknapp, mm318, piraka9011 and thomas-moulard January 22, 2020 18:07

zmichaels11 mentioned this pull request Jan 22, 2020

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

Closed

2 tasks

piraka9011 reviewed Jan 23, 2020

View reviewed changes

rosbag2_compression/CMakeLists.txt Show resolved Hide resolved

piraka9011 approved these changes Jan 23, 2020

View reviewed changes

zmichaels11 requested a review from Karsten1987 January 23, 2020 17:47

dabonnie reviewed Jan 23, 2020

View reviewed changes

rosbag2_transport/include/rosbag2_transport/record_options.hpp Outdated Show resolved Hide resolved

dabonnie reviewed Jan 24, 2020

View reviewed changes

rosbag2_transport/src/rosbag2_transport/rosbag2_transport_python.cpp Show resolved Hide resolved

dabonnie reviewed Jan 24, 2020

View reviewed changes

rosbag2_transport/src/rosbag2_transport/rosbag2_transport_python.cpp Show resolved Hide resolved

dabonnie approved these changes Jan 24, 2020

View reviewed changes

zmichaels11 force-pushed the compression-cli branch 2 times, most recently from 96deb6d to 089c971 Compare January 24, 2020 00:30

thomas-moulard reviewed Jan 24, 2020

View reviewed changes

thomas-moulard approved these changes Jan 24, 2020

View reviewed changes

Karsten1987 approved these changes Jan 28, 2020

View reviewed changes

rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp Outdated Show resolved Hide resolved

rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp Outdated Show resolved Hide resolved

zmichaels11 force-pushed the compression-cli branch from 37fd6a2 to bbbb91b Compare January 28, 2020 16:42

Reset storage on bag split before compressing

b02e45a

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

piraka9011 force-pushed the compression-cli branch from 27e6746 to b02e45a Compare February 6, 2020 01:04

zmichaels11 force-pushed the compression-cli branch from a301c3d to 2b88fa0 Compare February 6, 2020 16:58

Fix linting

c7d3b94

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

zmichaels11 force-pushed the compression-cli branch from 2b88fa0 to c7d3b94 Compare February 6, 2020 17:46

zmichaels11 mentioned this pull request Feb 6, 2020

[compression] Improve logging in rosbag2_compression #287

Merged

zmichaels11 mentioned this pull request Feb 6, 2020

[compression] Close storage before compression #284

Merged

Anas Abou Allaban and others added 12 commits February 7, 2020 10:57

Add compression flags to CLI

9d251f2

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

ament export rosbag2_compression

5f7c960

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

Add e2e test for recording compressed bagfiles

3d5d868

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

Add e2e test verifying compressed bagfiles are readable

c2a81e1

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

Remove redundant initialization of Reader and Writer

fc3b6aa

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

Disable e2e compression test on Windows that depends heavily on metadata

dbc986b

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

Update test to skip last split on windows

f71737e

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

Add log statements to expects

77c1a28

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

Assert only that bagfile split with compression

1cf8d38

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

Rebase onto master

7d932e1

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

Fix linting

db778a8

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

Fix merge conflicts, linting, and rebase

4a9feb3

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

piraka9011 force-pushed the compression-cli branch from 3d714d1 to 4a9feb3 Compare February 7, 2020 19:28

zmichaels11 commented Feb 7, 2020

View reviewed changes

rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp Show resolved Hide resolved

Remove default value in record options and add wait_for_metadata

a8b7e3d

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

piraka9011 force-pushed the compression-cli branch from ca9cbcf to a8b7e3d Compare February 7, 2020 19:41

Rewrite record_end_to_end_test_with_zstd_file_compression

8abeadb

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

piraka9011 merged commit 714d4c8 into ros2:master Feb 7, 2020

zmichaels11 deleted the compression-cli branch February 7, 2020 22:48

Conversation

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

Uh oh!

Changes

Testing

Uh oh!

Uh oh!

piraka9011 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dabonnie left a comment

Choose a reason for hiding this comment

Uh oh!

zmichaels11 commented Jan 24, 2020

Uh oh!

zmichaels11 commented Jan 24, 2020

Uh oh!

thomas-moulard Jan 24, 2020

Choose a reason for hiding this comment

Uh oh!

thomas-moulard commented Jan 24, 2020

Uh oh!

thomas-moulard left a comment

Choose a reason for hiding this comment

Uh oh!

zmichaels11 commented Jan 24, 2020

Uh oh!

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

Uh oh!

Uh oh!

zmichaels11 commented Jan 25, 2020

Uh oh!

zmichaels11 commented Jan 27, 2020

Uh oh!

Karsten1987 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

zmichaels11 commented Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

zmichaels11 commented Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zmichaels11 commented Jan 22, 2020 •

edited

Loading

zmichaels11 commented Jan 24, 2020 •

edited

Loading

zmichaels11 commented Jan 28, 2020 •

edited

Loading

zmichaels11 commented Feb 6, 2020 •

edited

Loading

zmichaels11 commented Feb 6, 2020 •

edited by piraka9011

Loading

piraka9011 commented Feb 7, 2020 •

edited by zmichaels11

Loading

zmichaels11 commented Feb 7, 2020 •

edited

Loading