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.

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

Implement snapshot mechanism and corresponding ROS Service #850

Merged
merged 5 commits into from
Sep 24, 2021

Conversation

camm73
Copy link
Contributor

@camm73 camm73 commented Aug 18, 2021

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 marked this pull request as ready for review August 24, 2021 19:04
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
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

@camm73 camm73 force-pushed the cameron/snapshot-service branch 4 times, most recently from e25c03a to 0309cb3 Compare September 8, 2021 19:25
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"

Cameron Miller and others added 2 commits September 13, 2021 15:01
Signed-off-by: Cameron Miller <cammlle@amazon.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
…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

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

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?

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.

YXL76 pushed a commit to YXL76/rosbag2 that referenced this pull request Nov 18, 2022
* 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
* 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
* 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
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants