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

Implement snapshot mechanism and corresponding ROS Service #850

Merged
merged 5 commits into from
Sep 24, 2021
Merged

Implement snapshot mechanism and corresponding ROS Service #850

merged 5 commits into from
Sep 24, 2021

Conversation

camm73
Copy link
Contributor

@camm73 camm73 commented Aug 18, 2021 •
edited by emersonknapp
Loading

Part of #663
Depends on #844

This PR implements the mechanism for taking a snapshot into the SequentialWriter, Writer, and BaseWriterInterface classes. It implements the take_snapshot function as a pure virtual function in BaseWriterInterface to ensure that all future Writers are built with snapshot mode compatibility.

Furthermore, a ROS service ~/snapshot is created within the Recorder node to provide an endpoint for triggering snapshots.

Finally, StorageOptions was updated to add a snapshot_mode field which is required by SequentialWriter to determine when the circular snapshot buffers should be created.

@camm73 camm73 requested a review from a team as a code owner August 18, 2021 18:51
@camm73 camm73 requested review from mjeronimo and MichaelOrlov and removed request for a team August 18, 2021 18:51
@camm73 camm73 marked this pull request as draft August 18, 2021 18:56
@camm73 camm73 mentioned this pull request Aug 18, 2021
Merged
@camm73 camm73 marked this pull request as ready for review August 24, 2021 19:04
MichaelOrlov
MichaelOrlov requested changes Aug 29, 2021
View reviewed changes
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

@camm73 Thank you for you contribution.
I assume that circular message cache and inner circular buffer will be implemented and reviewed in #844.
I didn't review them in the scope of this PR.
While entire implementation looks good for me, I would suggest a few changes in code.

rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp Outdated Show resolved Hide resolved
rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/recorder.cpp Outdated Show resolved Hide resolved
rosbag2_cpp/test/rosbag2_cpp/test_sequential_writer.cpp Outdated Show resolved Hide resolved
rosbag2_cpp/test/rosbag2_cpp/test_sequential_writer.cpp Outdated Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_record_services.cpp Outdated Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_record_services.cpp Outdated Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_record_services.cpp Outdated Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_record_services.cpp Outdated Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_record_services.cpp Outdated Show resolved Hide resolved
emersonknapp
emersonknapp requested changes Aug 31, 2021
View reviewed changes
Copy link
Collaborator

@emersonknapp emersonknapp 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 structural comments - mostly looks good

rosbag2_cpp/include/rosbag2_cpp/writers/sequential_writer.hpp Outdated Show resolved Hide resolved
rosbag2_cpp/include/rosbag2_cpp/writer_interfaces/base_writer_interface.hpp Outdated Show resolved Hide resolved
rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp Outdated Show resolved Hide resolved
rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp Outdated Show resolved Hide resolved
rosbag2_interfaces/srv/Snapshot.srv Show resolved Hide resolved
rosbag2_storage/include/rosbag2_storage/storage_options.hpp Show resolved Hide resolved
@camm73 camm73 force-pushed the cameron/snapshot-service branch 4 times, most recently from e25c03a to 0309cb3 Compare September 8, 2021 19:25
emersonknapp
emersonknapp reviewed Sep 13, 2021
View reviewed changes
Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

Adding some comments. I think I might be taking over this PR so they may be simply "notes to self"

rosbag2_cpp/include/rosbag2_cpp/writers/sequential_writer.hpp Outdated Show resolved Hide resolved
rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp Outdated Show resolved Hide resolved
rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp Outdated Show resolved Hide resolved
rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp Outdated Show resolved Hide resolved
Cameron Miller and others added 2 commits September 13, 2021 15:01
@emersonknapp
Add snapshot service to recorder node
3ca8350 
Signed-off-by: Cameron Miller <cammlle@amazon.com>
@emersonknapp
Address a couple nitpicks
55beff6 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Rewrite make_callback as a private method, using bind instead of lamb…
bd852fd 
…da, to reuse in take_snapshot

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Copy link
Collaborator

emersonknapp commented Sep 14, 2021 •
edited
Loading

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

@emersonknapp
Simplify and clarify double buffering patterns
cdeaf13 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Copy link
Collaborator

@MichaelOrlov I have reworked this PR a bit to make the double-buffering logic easier to follow (I think). Would you mind taking another look at this modified implementation and let me know what you think?

Rename to release_consumer_buffer
a36ffab 
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp
Copy link
Collaborator

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

@emersonknapp emersonknapp merged commit 200c4c3 into ros2:master Sep 24, 2021
@MichaelOrlov
Copy link
Contributor

@emersonknapp I've just recently got to your ping in this issue.
I am sorry I didn't catch it in time early.
I see that you made a significant overhaul in sequential_writer and message_cache classes.
Actually I didn't expect it in the scope of this PR, but nevertheless thanks for the improvements and clean up.
I will do post merge review to make sure that we didn't missed something or brake some logic.

@MichaelOrlov
Copy link
Contributor

@emersonknapp It seems I found a breakage in logic. Split doesn't work as before. i.e. after finalizing bag split, messages in next bag will not be written.
I have a quick fix in mind, however would like to spend some more time to find more optimal solution to address problem related to the high risk of dropping messages during the split.

@MichaelOrlov
Copy link
Contributor

@emersonknapp Also found out that in case of using CircularMessageCache it will be busy loop in CacheConsumer class.
The consequences will be that in Snapshot mode rosbag2 recorder will consume 100% of CPU resources on one core.
Also thinking how to proper address this issue. Don't want to do a quick patchy fix.

This was referenced Dec 14, 2021
Closed
Closed
@vachelyang vachelyang mentioned this pull request Jun 3, 2022
Closed
YXL76 pushed a commit to YXL76/rosbag2 that referenced this pull request Nov 18, 2022
@camm73 @emersonknapp
Implement snapshot mechanism and corresponding ROS Service (ros2#850)
031e824 
* Add snapshot service to recorder node
* Simplify and clarify double buffering patterns

Co-authored-by: Cameron Miller <cammlle@amazon.com>
Co-authored-by: Emerson Knapp <emerson.b.knapp@gmail.com>
YXL76 pushed a commit to YXL76/rosbag2 that referenced this pull request Nov 18, 2022
@camm73 @emersonknapp @YXL76
Implement snapshot mechanism and corresponding ROS Service (ros2#850)
3949214 
* Add snapshot service to recorder node
* Simplify and clarify double buffering patterns

Co-authored-by: Cameron Miller <cammlle@amazon.com>
Co-authored-by: Emerson Knapp <emerson.b.knapp@gmail.com>
YXL76 pushed a commit to YXL76/rosbag2 that referenced this pull request Nov 18, 2022
@camm73 @emersonknapp @YXL76
Implement snapshot mechanism and corresponding ROS Service (ros2#850)
53c8afe 
* Add snapshot service to recorder node
* Simplify and clarify double buffering patterns

Co-authored-by: Cameron Miller <cammlle@amazon.com>
Co-authored-by: Emerson Knapp <emerson.b.knapp@gmail.com>
YXL76 pushed a commit to YXL76/rosbag2 that referenced this pull request Nov 18, 2022
@camm73 @emersonknapp @YXL76
Implement snapshot mechanism and corresponding ROS Service (ros2#850)
7213702 
* Add snapshot service to recorder node
* Simplify and clarify double buffering patterns

Co-authored-by: Cameron Miller <cammlle@amazon.com>
Co-authored-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@YXL76 YXL76 mentioned this pull request Nov 18, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lihui815 lihui815 lihui815 left review comments

@emersonknapp emersonknapp emersonknapp approved these changes

@mjeronimo mjeronimo Awaiting requested review from mjeronimo

@MichaelOrlov MichaelOrlov Awaiting requested review from MichaelOrlov

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
@camm73 @emersonknapp @MichaelOrlov @lihui815