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

Move get_storage_identifier and get_bagfile_size #209

Merged
merged 2 commits into from
Dec 3, 2019
Merged

Move get_storage_identifier and get_bagfile_size #209

merged 2 commits into from
Dec 3, 2019

Conversation

zmichaels11
Copy link
Contributor

@zmichaels11 zmichaels11 commented Nov 18, 2019 •
edited
Loading

Resolves Issue: #197

Changes

  • Move get_storage_identifier to BaseInfoInterface
  • Move get_bagfile_size to BaseInfoInterface

Notes

  • This change does not require changes to rosbag2_bag_v2

Testing

  • Ran colcon build and test on subprojects of rosbag2
  • Ran colcon build and test on rosbag2_bag_v2

@zmichaels11
[WIP] Move get_storage_identifier and get_bagfile_size
01d1bfb 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11 zmichaels11 force-pushed the io-interface-cleanup/move-get_bagfile_size-get_storage_identifier branch from 5c7c723 to 01d1bfb Compare November 18, 2019 22:57
@zmichaels11 zmichaels11 mentioned this pull request Nov 18, 2019
Closed
6 tasks
@zmichaels11
Copy link
Contributor Author

This should resolve the failed builds on rosbag2_bag_v2.
Test failures are due to changes in SequentialReader in rosbag2.

@zmichaels11 zmichaels11 marked this pull request as ready for review November 19, 2019 00:19
@zmichaels11 zmichaels11 changed the title [WIP] Move get_storage_identifier and get_bagfile_size Move get_storage_identifier and get_bagfile_size Nov 19, 2019
@zmichaels11 zmichaels11 changed the title Move get_storage_identifier and get_bagfile_size [WIP] Move get_storage_identifier and get_bagfile_size Nov 19, 2019
@zmichaels11
Remove trailing spaces
bd0b4e4 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11 zmichaels11 changed the title [WIP] Move get_storage_identifier and get_bagfile_size Move get_storage_identifier and get_bagfile_size Nov 21, 2019
@zmichaels11
Copy link
Contributor Author

zmichaels11 commented Nov 21, 2019 •
edited
Loading

This PR is ready for review.
It should not be merged until the required API changes are introduced into rosbag2_bag_v2

@zmichaels11
Copy link
Contributor Author

Built rosbag2_bag_v2 against this branch and confirmed no regression.
This PR is ready for review and can be merged.

Karsten1987
Karsten1987 reviewed Nov 25, 2019
View reviewed changes
rosbag2_storage/include/rosbag2_storage/storage_interfaces/base_info_interface.hpp
Comment on lines +43 to +47
/**
* Returns the size of the bagfile.
* \returns the size of the bagfile in bytes.
*/
virtual uint64_t get_bagfile_size() const = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

doesn't #197 state to have bagfile size part of the IO interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I accidentally re-worded the issue and flipped the function migration.
I'll re-review this work in a few hours

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update:
The correct wordage should be: "get_bagfile_size and get_storage_identifier should be moved to BaseInfoInterface".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update:
Corrected wordage in issue

piraka9011
piraka9011 approved these changes Dec 2, 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.

thomas-moulard
thomas-moulard approved these changes Dec 2, 2019
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.

This LGTM w/ CI.
Let's wait for @Karsten1987 input as this is an architectural change.

Karsten1987
Karsten1987 approved these changes Dec 2, 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.

looks good to me to. Please run CI on it.

@zmichaels11
Copy link
Contributor Author

@prajakta-gokhale please run CI:
Gist: https://gist.githubusercontent.com/zmichaels11/509a30b66376c16a5e097a30735e9656/raw/9b3d2ded44337cf42f7abc67515c0c03a8b6bdf8/ros2.repos
BUILD args: --packages-up-to rosbag2 rosbag2_storage rosbag2_storage_default_plugins rosbag2_transport rosbag2_tests
TEST args: --packages-select rosbag2 rosbag2_storage rosbag2_storage_default_plugins rosbag2_transport rosbag2_tests
Job: ci_launcher

@prajakta-gokhale
Copy link

prajakta-gokhale commented Dec 2, 2019 •
edited
Loading

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

Rebuild:

  • macOS Build Status

@prajakta-gokhale
Copy link

The MacOS failure seems unrelated to this change, probably related to infrastructure issues. (some MacOS nightlies are also failing like this)

@thomas-moulard
Copy link
Contributor

@prajakta-gokhale Let's retry once? @nuclearsandwich FYI

@prajakta-gokhale
Copy link

Passed on retry! 🎊

@prajakta-gokhale prajakta-gokhale merged commit 6cf47b2 into ros2:master Dec 3, 2019
@zmichaels11 zmichaels11 deleted the io-interface-cleanup/move-get_bagfile_size-get_storage_identifier branch December 3, 2019 00:29
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 approved these changes

@Karsten1987 Karsten1987 Karsten1987 approved these changes

@piraka9011 piraka9011 piraka9011 approved these changes

@dabonnie dabonnie dabonnie 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.
6 participants
@zmichaels11 @prajakta-gokhale @thomas-moulard @Karsten1987 @piraka9011 @dabonnie