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

Bag rewriter (C++) #920

Merged
merged 4 commits into from
Dec 3, 2021
Merged

Bag rewriter (C++) #920

merged 4 commits into from
Dec 3, 2021

Conversation

emersonknapp
Copy link
Collaborator

@emersonknapp emersonknapp commented Nov 24, 2021 •
edited
Loading

Implements main functionality of #831
Depends on #924
Depends on #923

Adds a bag_rewrite API that takes Reader(s) for existing bags and writes all their messages to Writer(s) with specified StorageOptions and RecordOptions.

Does not expose the feature to Python/CLI - that is implemented in followup PR #921

@emersonknapp emersonknapp mentioned this pull request Nov 24, 2021
Merged
@emersonknapp emersonknapp force-pushed the emersonknapp/bag-rewriter branch 5 times, most recently from f7d2b83 to 5805c66 Compare November 25, 2021 10:34
@emersonknapp emersonknapp changed the title Implement bag_rewrite bag_rewrite C++ implementation Nov 25, 2021
@emersonknapp emersonknapp changed the title bag_rewrite C++ implementation bag_rewrite implementation Nov 25, 2021
@emersonknapp emersonknapp force-pushed the emersonknapp/bag-rewriter branch 2 times, most recently from 85aaa4e to b390a86 Compare November 30, 2021 01:01
@emersonknapp emersonknapp changed the title bag_rewrite implementation [WIP] bag_rewrite implementation Nov 30, 2021
@emersonknapp emersonknapp changed the base branch from master to emersonknapp/topic-filter-single-call November 30, 2021 01:03
@delete-merged-branch delete-merged-branch bot deleted the branch master November 30, 2021 19:26
@emersonknapp emersonknapp changed the base branch from emersonknapp/topic-filter-single-call to master November 30, 2021 19:27
@emersonknapp emersonknapp changed the title [WIP] bag_rewrite implementation bag_rewrite implementation Dec 2, 2021
@emersonknapp emersonknapp changed the title bag_rewrite implementation Bag rewriter (C++) Dec 2, 2021
@emersonknapp emersonknapp marked this pull request as ready for review December 2, 2021 03:46
@emersonknapp emersonknapp requested a review from a team as a code owner December 2, 2021 03:46
@emersonknapp emersonknapp requested review from MichaelOrlov, jhdcs and lihui815 and removed request for a team and MichaelOrlov December 2, 2021 03:46
@emersonknapp
Add C++ bag rewriter
d40ff41 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Add tests for rewriter
200ae66 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
jhdcs
jhdcs reviewed Dec 2, 2021
View reviewed changes
Copy link
Contributor

@jhdcs jhdcs left a comment

Choose a reason for hiding this comment

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

Only seeing one really small comment issue (apart from the build issue). Not sure if it's something we need to bother with, but thought I'd bring it up anyways.

rosbag2_transport/src/rosbag2_transport/bag_rewrite.cpp Show resolved Hide resolved
lihui815
lihui815 approved these changes Dec 2, 2021
View reviewed changes
Copy link
Contributor

@lihui815 lihui815 left a comment

Choose a reason for hiding this comment

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

looks reasonable to me. just one nit on documenting serialization format selection.

@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Dec 2, 2021 •
edited
Loading

Gist: https://gist.githubusercontent.com/emersonknapp/118ff3d83fe59438cad609306b4c102d/raw/8368bbbc8a74f487474f3474161702962fbd4cb6/ros2.repos
BUILD args: --packages-up-to rosbag2_transport rosbag2_tests rosbag2
TEST args: --packages-select rosbag2_transport rosbag2_tests rosbag2
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/9436

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

@emersonknapp
Fix linters and address review comments
e9c33e7 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Fix test dependencies and windows build
3e55113 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Dec 2, 2021 •
edited
Loading

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

@emersonknapp
Copy link
Collaborator Author

Noting here explicitly. The OSX CI build queue is backed up by ~12 hours right now because there is only a single executor online and some very long builds are stacked up. Will merge now, and if something is warning on OSX then I am happy to fix it then.

@emersonknapp emersonknapp merged commit 2abc509 into master Dec 3, 2021
@delete-merged-branch delete-merged-branch bot deleted the emersonknapp/bag-rewriter branch December 3, 2021 00:06
@clalancette
Copy link
Contributor

Noting here explicitly. The OSX CI build queue is backed up by ~12 hours right now because there is only a single executor online and some very long builds are stacked up.

Whoops. We were doing some network maintenance and we forgot to reenable them when that was finished. All 4 are back online now, FYI.

@emersonknapp
Copy link
Collaborator Author

Looks like the one from this morning failed on some networking issue, and the one from 3 hours ago got remove from the queue somehow (Jenkins doesn't have actual entities for pending builds, which drives me bananas - if you delete your build from the queue then everybody else's badges from behind you in the queue are wrong because the ID gets reassigned to the next queue item )

Rebuilding for just in case, though #921 will catch any problems

OSX: Build Status

Blast545
Blast545 reviewed Dec 3, 2021
View reviewed changes
rosbag2_transport/src/rosbag2_transport/bag_rewrite.cpp
next_messages.resize(input_bags.size(), nullptr);

std::shared_ptr<rosbag2_storage::SerializedBagMessage> next_msg;
while (next_msg = get_next(input_bags, next_messages)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This introduced a warning message into the buildfarm. See nightly_osx_debug#2246.

It's probably unintended usage, would you take a look? @emersonknapp

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is intended usage - as noted above I wasn't 100% sure that OSX would be happy. Will add the parentheses suggested by Clang ASAP.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#930

@Blast545
Copy link
Contributor

Blast545 commented Dec 3, 2021 •
edited
Loading

👨‍🌾 This test regression is probably related to this PR as well: https://ci.ros2.org/view/nightly/job/nightly_win_rel/2137/

C:\ci\ws\src\ros2\rosbag2\rosbag2_transport\test\rosbag2_transport\test_record.cpp:210
Value of: pub_manager.wait_for_matched(topic.c_str())
  Actual: false
Expected: true

@emersonknapp
Copy link
Collaborator Author

@Blast545 no, that is an existing flaky test - there is no way for this PR to have affected recording functionality

@emersonknapp emersonknapp mentioned this pull request Dec 3, 2021
Merged
@Blast545 Blast545 mentioned this pull request Dec 8, 2021
Closed
Blast545 added a commit that referenced this pull request Dec 8, 2021
@Blast545
Revert "Bag rewriter (C++) (#920)"
461be23 
This reverts commit 2abc509.

Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhdcs jhdcs jhdcs left review comments

@Blast545 Blast545 Blast545 left review comments

@lihui815 lihui815 lihui815 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.
5 participants
@emersonknapp @clalancette @Blast545 @lihui815 @jhdcs