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

Bugfix for broken bag split when using cache #936

Merged
merged 3 commits into from
Dec 16, 2021

Conversation

MichaelOrlov
Copy link
Contributor

@MichaelOrlov MichaelOrlov commented Dec 12, 2021

Add start() method to the CacheConsumer class and call it from
sequential_writer after split. Also added unit test to cover cases when bag splitting with enabled cache.

Add `start()` method to the `CacheConsumer` class and call it from
`sequential_writer` after split.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov MichaelOrlov marked this pull request as ready for review December 14, 2021 08:15
@MichaelOrlov MichaelOrlov requested a review from a team as a code owner December 14, 2021 08:15
@MichaelOrlov MichaelOrlov requested review from hidmic and removed request for a team December 14, 2021 08:15
@adamdbrw
Copy link
Collaborator

adamdbrw commented Dec 14, 2021

This looks like a regression to me, likely introduced with the snapshot feature. Cache consumer worked with bag splitting without the need to explicitly start it previously. This was guaranteed by change_consume_callback() which does restart the thread.

Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

Overall LGTM pending green CI

…rTest

writer_creates_correct_metadata_relative_filepaths since logic for split
has been changed.

Now it will be fair. If specified max_baf_file_size = 1 it will be
reasonable to expect only one message per file.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov
Copy link
Contributor Author

Running CI
CI_BRANCH_TO_TEST: morlov/bugfix_for_broken_split
BUILD args: --packages-up-to rosbag2_cpp rosbag2_transport rosbag2_tests
TEST args: --packages-select rosbag2_cpp rosbag2_transport rosbag2_tests
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/9513

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

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov
Copy link
Contributor Author

Rerun CI for Windows after trying to fix typecast warning in test.

  • Windows Build Status

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.

Bag split doesn't work during recording when cache enabled
3 participants