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

[humble] Make snapshot writing into a new file each time it is triggered (backport #1842) #1850

Merged
merged 2 commits into from
Nov 23, 2024

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Nov 1, 2024

This PR will make snapshot writing into a new file each time it is triggered.

Note. Snapshot now becomes a blocking call and mutually exclusive with the writer::write(message) method to avoid race conditions, i.e., blocking the same writer_mutex_.
The set_data_ready() method from #1839 is not needed since "snapshot" has now become a blocking call; therefore, there are no race conditions, and we will not dump data from the cyclic buffer twice because data_ready will be settled false after the first dump with message_cache_->notify_data_ready();.

void CircularMessageCache::swap_buffers()
{
std::lock_guard<std::mutex> producer_lock(producer_buffer_mutex_);
// Swap buffers only if data is ready. Data not ready when we are calling flushing on exit and
// we should not dump buffer on exit if snapshot has not been triggered.
if (data_ready_) {
std::lock_guard<std::mutex> consumer_lock(consumer_buffer_mutex_);
consumer_buffer_->clear();
std::swap(producer_buffer_, consumer_buffer_);
data_ready_ = false;
}
}

However, we still need to call split_bagfile_local(true) to trigger callbacks and open a new storage file.

This PR could be backported. There are no API/ABI breaking changes in it.


This is an automatic backport of pull request #1842 done by Mergify.

* Make snapshot writing into a new file each time when it is triggered

- Note. Snapshot now became a blocking call and mutually exclusive with
writer::write(message) method to avoid race conditions.
i.e. blocking the same writer_mutex_

Signed-off-by: Michael Orlov <[email protected]>

* Add unit test to make sure that snapshot writing in the new file

Co-authored-by: Clemens Mühlbacher <[email protected]>
Signed-off-by: Michael Orlov <[email protected]>

* Add support for snapshot with file compression

Signed-off-by: Michael Orlov <[email protected]>

* Rename newly added tests to avoid misunderstanding

Signed-off-by: Michael Orlov <[email protected]>

* Address review comments in tests

Signed-off-by: Michael Orlov <[email protected]>

* Change order of includes in the test_sequential_compression_writer.cpp

Signed-off-by: Michael Orlov <[email protected]>

* Update metadata_.message_count unconditionally in write_messages(..)

Signed-off-by: Michael Orlov <[email protected]>

---------

Signed-off-by: Michael Orlov <[email protected]>
Co-authored-by: Clemens Mühlbacher <[email protected]>
(cherry picked from commit 3f2281f)
@mergify mergify bot requested a review from a team as a code owner November 1, 2024 23:33
@mergify mergify bot requested review from emersonknapp and jhdcs and removed request for a team November 1, 2024 23:33
@MichaelOrlov MichaelOrlov changed the title Make snapshot writing into a new file each time it is triggered (backport #1842) [humble] Make snapshot writing into a new file each time it is triggered (backport #1842) Nov 1, 2024
@MichaelOrlov MichaelOrlov requested review from MichaelOrlov and removed request for emersonknapp and jhdcs November 1, 2024 23:35
- Humble doesn't have `recv_timestamp` and `send_timestamp`. Replaced
them with ordinary `time_stamp`.
- Also reworked tests to accommodate to the Humble code base.

Signed-off-by: Michael Orlov <[email protected]>
@MichaelOrlov MichaelOrlov force-pushed the mergify/bp/humble/pr-1842 branch from b292af5 to 61e0f4f Compare November 22, 2024 21:13
@MichaelOrlov
Copy link
Contributor

@ros-pull-request-builder retest this please

@MichaelOrlov
Copy link
Contributor

Pulls: #1850
Gist: https://gist.githubusercontent.com/MichaelOrlov/8bea03590868215ded1b65a2170eb710/raw/7e08c6e461b3d8bb94661e871473870570a1248d/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_compression rosbag2_cpp rosbag2_tests
TEST args: --packages-above rosbag2_compression rosbag2_cpp rosbag2_tests
ROS Distro: humble
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14858

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Contributor

@MichaelOrlov did you take #1860? @cmuehlbacher is working on to fix the humble patch...

@MichaelOrlov
Copy link
Contributor

@MichaelOrlov did you take #1860? @cmuehlbacher is working on to fix the humble patch...

Actually not. I was off for a while and missed #1860. However, I made the similar fixes even likely more ;)

@MichaelOrlov
Copy link
Contributor

MichaelOrlov commented Nov 23, 2024

@fujitatomoya Since my version includes all changes from #1860 (I double-checked) and plus extra checks for metadata and CI ran green among the RHEL, which complaints about a warning no CONNEXTDDS_DIR nor NDDSHOME specified, which is, I believe, unrelated to the changes in this backporting PR and more like a CI build infrastructure issue, I am going to merge this PR to the Humble.

Copy link
Contributor

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

@MichaelOrlov MichaelOrlov merged commit 0f6673e into humble Nov 23, 2024
14 checks passed
@MichaelOrlov MichaelOrlov deleted the mergify/bp/humble/pr-1842 branch November 23, 2024 19: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