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] Close storage before compression #284

Merged
merged 2 commits into from
Feb 7, 2020
Merged

[compression] Close storage before compression #284

merged 2 commits into from
Feb 7, 2020

Conversation

piraka9011
Copy link
Contributor

@piraka9011 piraka9011 commented Feb 4, 2020 •
edited by zmichaels11
Loading

Changes

  • Close the storage plugin before compressing it. While it is legal to compress an open storage plugin, it will result in only the data synced to disc being compressed.
  • This work is in relationship to reducing flakiness in rosbag2_tests record end-to-end test.

Note

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

thomas-moulard
thomas-moulard suggested changes Feb 4, 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.

Let's make sure this is the right logical approach as we discussed offline.

@piraka9011
Copy link
Contributor Author

Investigating a better approach to closing the bag file since this may result in an extra bag file generated if there are no more new messages.

@piraka9011 piraka9011 force-pushed the clean-compression branch 2 times, most recently from 25cbdc5 to d56476f Compare February 6, 2020 00:14
@zmichaels11
Close storage before compression
7fe19fa 
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11 zmichaels11 changed the title Close storage before compression [compression] Close storage before compression Feb 6, 2020
@zmichaels11
Copy link
Contributor

I've verified in #263 that this approach does fix a bug visible in Windows.
The changes done are:

  • Close storage plugin before compressing. This order of operations needs to be explicit due to logic for closing the last bag file within the dtor
  • Only compress if the bag file exists and is not empty

@zmichaels11
Only compress if the bag file exists and has content
d834a85 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Copy link
Contributor

@ros2/aws-oncall - please run this CI job
Gist: https://gist.githubusercontent.com/zmichaels11/7a6c7e7de66e7a449e703611310fb4ad/raw/b4dffe80154096d58dcb84458282f6d70ec2b676/ros2.repos
BUILD args: --packages-up-to rosbag2_compression
TEST args: --packages-select rosbag2_compression
Job: ci_launcher

@zmichaels11 zmichaels11 mentioned this pull request Feb 7, 2020
Merged
@emersonknapp
Copy link
Collaborator

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

thomas-moulard
thomas-moulard approved these changes Feb 7, 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.

LGTM

@zmichaels11 zmichaels11 merged commit baca364 into ros2:master Feb 7, 2020
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

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

@emersonknapp emersonknapp emersonknapp approved these changes

@Karsten1987 Karsten1987 Awaiting requested review from Karsten1987

@dabonnie dabonnie Awaiting requested review from dabonnie

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@piraka9011 @zmichaels11 @emersonknapp @thomas-moulard @dabonnie