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

Add bagfile splitting support to storage_options and Writer #182

Merged
merged 7 commits into from
Oct 21, 2019
Merged

Add bagfile splitting support to storage_options and Writer #182

merged 7 commits into from
Oct 21, 2019

Conversation

zmichaels11
Copy link
Contributor

@zmichaels11 zmichaels11 commented Oct 17, 2019 •
edited
Loading

Refactored changes in PR #171
This is part of an effort to rework PR #158 into multiple, smaller PRs.

Changes

  • Add max_bagfile_size to storage_options. This is used to indicate when a bagfile should be split. A default value of 0 indicates no splitting.
  • Add function should_split_bagfile to Writer. This will be used by Writer to determine when a bagfile should be split.

Issues

  • ros-security/aws-roadmap#11

piraka9011
piraka9011 approved these changes Oct 17, 2019
View reviewed changes
Copy link
Contributor

@piraka9011 piraka9011 left a comment

Choose a reason for hiding this comment

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

LGTM!

Note to add should_split_bagfile as a mocked method to the unit tests. (You probably already did that).

This was referenced Oct 17, 2019
Merged
Merged
Merged
@zmichaels11
Copy link
Contributor Author

zmichaels11 commented Oct 17, 2019 •
edited
Loading

@Karsten1987
also see PR #183 #184 #185

thomas-moulard
thomas-moulard reviewed Oct 18, 2019
View reviewed changes
Copy link
Contributor

@thomas-moulard thomas-moulard 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.

Could the new function be private or protected, if yes -> move to private, if no -> it needs a test.

rosbag2/include/rosbag2/writer.hpp Outdated Show resolved Hide resolved
Karsten1987
Karsten1987 reviewed Oct 18, 2019
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.

a few nitpicks on top of thomas' comments.

rosbag2/include/rosbag2/writer.hpp Outdated Show resolved Hide resolved
rosbag2/include/rosbag2/storage_options.hpp Show resolved Hide resolved
rosbag2/src/rosbag2/writer.cpp Show resolved Hide resolved
@zmichaels11 zmichaels11 mentioned this pull request Oct 18, 2019
Closed
2 tasks
@zmichaels11 zmichaels11 mentioned this pull request Oct 18, 2019
Closed
@Karsten1987
Copy link
Collaborator

This PR has to be rebased on top of master.

@zmichaels11
Add support for specifying max bagfile size in storage_options
d1f29f0 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Add helper functions in Writer required for bagfile splitting
45bcc3a 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Store max_bagfile_size when Writer is opened
42d5a93 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Uncrustify
dad39bd 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Apply suggestions from PR
f995257 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11 zmichaels11 force-pushed the splitting/storage-options-changes branch from d0f240a to f995257 Compare October 21, 2019 15:18
@zmichaels11
Copy link
Contributor Author

@Karsten1987 rebased

@Karsten1987
Copy link
Collaborator

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

@zmichaels11
Add ROSBAG2_STORAGE_PUBLIC to MAX_BAGFILE_SIZE_NO_SPLIT
32e5fa4 
This should fix the issue on Windows

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@Karsten1987
Copy link
Collaborator

new round of MS CI:
Build Status

@zmichaels11
Copy link
Contributor Author

new round of MS CI:
Build Status

we did it!

Karsten1987
Karsten1987 approved these changes Oct 21, 2019
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.

this looks good to me. Just a little nitpick about naming convention.

rosbag2/include/rosbag2/writer.hpp Outdated Show resolved Hide resolved
@zmichaels11
Renamed private function in Writer to not end in _
85cd54b 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@Karsten1987 Karsten1987 merged commit d659129 into ros2:master Oct 21, 2019
@Karsten1987
Copy link
Collaborator

thanks for iterating with me over this.

@zmichaels11 zmichaels11 deleted the splitting/storage-options-changes branch October 21, 2019 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomas-moulard thomas-moulard thomas-moulard left review comments

@Karsten1987 Karsten1987 Karsten1987 approved these changes

@piraka9011 piraka9011 piraka9011 approved these changes

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
4 participants
@zmichaels11 @Karsten1987 @thomas-moulard @piraka9011