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

Feature/snapshot write different file each trigger rolling #1839

Conversation

cmuehlbacher
Copy link
Contributor

@cmuehlbacher cmuehlbacher commented Oct 17, 2024

This ports the pull request #1828.
It also addresses the race condition mention in @reinzor That is one more very hacky way to implement this feature., which was also mention in #1828

Signed-off-by: Clemens Mühlbacher <[email protected]>
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.

@cmuehlbacher Thank you for your contribution.
We need to make snapshot writing to the new file by default without providing CLI options.
Allowing multiple snapshots to be written in one file was a mistaken design decision from the very beginning. It has no value. The file with multiple snapshots is useless and difficult to handle during playback or introspection.
Imagine a situation when a few snapshot recorded in one file with interval of several hours.
The playback of such file will take also a several hours with huge delays when nothing will be published. Also it would be difficult to find when one snapshot beginnig and another is started during introspection.

In your current implementation possible at least two data races. The one when updating metadata_ in switch_to_next_storage() and the second one when re-registering topics in the newly opened storage in the same function, because at the same time subscription callback can be triggered and writer::write(message) could be called.
To avoid data races we need to either add synchronization primitives and partially block writer::write(message) when we are updating metadata_ and creating and switching to a new storage or make snapshot as blocking call the same as writer::write(message).

Another point is that this PR with adding a new field to the storage_options.hpp and a new virtual function to the message_cache_interface.hpp is not backportable to other ROS 2 distros because this is an ABI breaking changes.

@cmuehlbacher
Copy link
Contributor Author

Close this pull request as this feature will be implemented through #1842.

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.

2 participants