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

Snapshot writes to a different file every time it is triggered #1828

Conversation

cmuehlbacher
Copy link
Contributor

Addresses the issue described in #1099.

This is a simple change adding an argument on the command line to use a split function each time a snapshot is taken. It uses split bagfile function already implemented. Any improvement on this function regarding file names or returns can be simple translated to the snapshot function afterwards.

It is developed on the Humble branch as we need this feature in humble. But it should be easily ported to rolling.

There might be one concern the StorageOptions struct has a change (a member was added). This might cause issues with the ABI (depend on if this structure is part of it or not).

@cmuehlbacher cmuehlbacher requested a review from a team as a code owner October 11, 2024 10:35
@cmuehlbacher cmuehlbacher requested review from MichaelOrlov and hidmic and removed request for a team October 11, 2024 10:35
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 Thanks for the initiative, but not this way.

@cmuehlbacher
Copy link
Contributor Author

@cmuehlbacher Thanks for the initiative, but not this way.

Thanks for the quick look.

As there is no renaming involved only point number 2 is not properly considered in my change.
I will have a look if this change is possible in Humble, otherwise I will give it a shot in Rolling (with the API constraints in Humble this might be easier).

But just for my understanding about the code as we have a specific parameter setting (snapshot and cache enabled).

  • In the SequentialWriter use_cache_ is set (line 140 in sequential_writer.cpp).
  • Therefore, we use the circular message cache (line 148-149 in sequential_writer.cpp). Which is also noted within the documentation about the cache.
  • With the CircularMessageCache the function swap_buffers (line 95 in circular_message_cache.cpp) swaps only the buffers if the data are ready. And clears the flag afterwards.
  • This flag is only set if notify_data_ready is called. Which is called when performing a snapshot.
  • Therefore, data are only written once, regardless how often swap buffers is called.
    So, there should be no race condition/additional data written for this specific case. Right?

@cmuehlbacher
Copy link
Contributor Author

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

@cmuehlbacher cmuehlbacher deleted the feature/snapshot_write_different_file_each_trigger branch November 21, 2024 07:22
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