-
Notifications
You must be signed in to change notification settings - Fork 248
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
Notification of significant events during bag recording and playback #908
Conversation
I could do with some suggestions on how best to test this functionality. |
At the moment, it's looking like |
Not yet. This is still a work in progress. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gbiggs Thank you for implementing this feature.
I like idea with EventCallbackManager
.
However it seems you forgot to add call for execute_callbacks
to the SequentialReader::load_next_file()
IMO. It could be implemented as:
void SequentialReader::load_next_file()
{
assert(current_file_iterator_ != file_paths_.end());
auto info = std::make_shared<bag_events::BagSplitInfo>();
info->closed_file = get_current_file();
current_file_iterator_++;
load_current_file();
info->opened_file = get_current_file();
callback_manager_.execute_callbacks(bag_events::BagEvent::READ_SPLIT, info);
}
I also have concern about potential performance degradation during bag split on recording. See my comments in code.
PS: Looking forward to see implementation for relevant unit tests.
rosbag2_transport/test/rosbag2_transport/mock_sequential_reader.hpp
Outdated
Show resolved
Hide resolved
rosbag2_cpp/include/rosbag2_cpp/reader_interfaces/base_reader_interface.hpp
Outdated
Show resolved
Hide resolved
8575fbd
to
907ee38
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick comment. I'm also curious about a solution to the performance issue that @MichaelOrlov raised.
@@ -56,6 +56,13 @@ class MockSequentialReader : public rosbag2_cpp::reader_interfaces::BaseReaderIn | |||
std::shared_ptr<rosbag2_storage::SerializedBagMessage> read_next() override | |||
{ | |||
// filter_ was considered when incrementing num_read_ in has_next() | |||
if (num_read_ % 5 == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm drawing a bit of a blank at the moment, but what is the "5" here representing? Should this be a constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It means the bag is "split" every 5 messages. I'll add a comment.
I've added integration tests for the |
I've pushed unit tests for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks for the revisions
Gist: https://gist.githubusercontent.com/emersonknapp/f8266fb628fa25dd88199616b529ea12/raw/20366e0739e0daff5b03c0d68c9a387a3566740b/ros2.repos |
|
||
namespace rosbag2_cpp | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would some comments in this file explaining what the events are and how to implement them be useful? I'm just thinking of future contributors to rosbag2
that may not have been privy to the discussions on this. Comments may help to reduce onboarding time and lessen potential confusion. Maybe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! That file was completely devoid of comments. I've documented the entities in it in ad46aaa. Please take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation is very thorough! Fantastic! Love it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gbiggs Thank you for addressing my comments and proposals from previous round of review and new revisions.
Here is my second round of review.
I have proposal how to make RosBag2PlayTestFixture::read_split_callback_is_called
test non-flaky. See my proposals in code comments.
rosbag2_transport/test/rosbag2_transport/mock_sequential_writer.hpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me! Especially love the added documentation. I only had one area where I wasn't clear where something was being said, but I'm not confident that's not just me missing something.
|
||
namespace rosbag2_cpp | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation is very thorough! Fantastic! Love it!
split_event_pub_->publish(message); | ||
} | ||
|
||
should_exit = event_publisher_thread_should_exit_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I'm missing something, but where is event_publisher_thread_should_exit_
being set/modified after line 112? Is this going to be always true
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The thread exists from the start of recording for the remaining lifetime of the Recorder
and only exits when the recorder destructs. This is consistent with the way the Recorder
class, once it starts recording, only stops recording when it destructs.
I think we're good to move forward - have you addressed the yellow issues on OSX and Windows? Are you able to re-run CI yourself or would you like me to run it again? |
I haven't been able to address those issues. However a lot has changed since that CI run, so if you could run CI again that would be great. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion for replacing magic number size_t num_msgs_in_bag = 6;
with getter max_messages_per_file()
from MockSequentialReader
and MockSequentialWriter
Gist: https://gist.githubusercontent.com/emersonknapp/ea90cff73b8a33a9cf4479ef2810e008/raw/20366e0739e0daff5b03c0d68c9a387a3566740b/ros2.repos |
I've addressed the warning from OSX. The Windows test failure ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me with green CI.
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Co-authored-by: Emerson Knapp <537409+emersonknapp@users.noreply.github.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
f7e5fa5
to
b8bc753
Compare
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
@gbiggs Some tests from play_for failed on CI Actions job https://build.ros2.org/job/Rpr__rosbag2__ubuntu_jammy_amd64/126/ |
@gbiggs I was not able to reproduce those failures in Merging this request then. |
Thanks for trying to reproduce them, @MichaelOrlov . I similarly was not able to reproduce the errors locally, and the manual CI also did not turn them up. Now that this PR is merged, can we please get a release of rosbag2 to |
@gbiggs I want to defer release to the rolling until we will get merged at least #1032 and perhaps #1007 As regards to the possibility of backporting this PR to the |
…#908) * Simple hacky prototype Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Improved prototype with more flexibility and event information Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Use the full relative file path for event info Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Remove no-longer required dependency Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Remove left-over merge Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Change event names Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Add overrides to mocks Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Add missing include Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Remove empty file Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Remove unnecessary virtual Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Correct formatting Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Add missing doc Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Reformat documentation Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Change read event topic name Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Change read event topic name Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Simplify for loop Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Remove unnecessary blank line Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Change argument type to const Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Add events handling to mocks Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Add comment explaining 5-message split Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Add integration tests for player and recorder Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Add ability to check if an event has callbacks Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Shift event topic publishing to a separate thread Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Fix formatting Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Test SequentialReader event callbacks Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Test SequentialWriter event callbacks Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Document bag events Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Check if joinable before joining Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Correct spelling mistake Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Correct spelling mistake Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Reduce time wasted waiting for test to reach intended behaviour Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Address test flakiness Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Improve size check Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Use constant for split size in tests Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Add missing header Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Allocate thread on the stack Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Add getters for number of messages per 'file' Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Replace magic number Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Address unstable OSX build Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Make path in test cross-platform Co-authored-by: Emerson Knapp <537409+emersonknapp@users.noreply.github.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Add missing virtual destructor Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Follow uncrustify suggestion Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Fix rebase errors Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Remove double include Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> Co-authored-by: Michael Orlov <morlovmr@gmail.com> Co-authored-by: Emerson Knapp <537409+emersonknapp@users.noreply.github.com>
…#908) (#1037) * Simple hacky prototype Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Improved prototype with more flexibility and event information Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Use the full relative file path for event info Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Remove no-longer required dependency Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Remove left-over merge Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Change event names Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Add overrides to mocks Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Add missing include Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Remove empty file Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Remove unnecessary virtual Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Correct formatting Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Add missing doc Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Reformat documentation Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Change read event topic name Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Change read event topic name Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Simplify for loop Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Remove unnecessary blank line Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Change argument type to const Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Add events handling to mocks Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Add comment explaining 5-message split Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Add integration tests for player and recorder Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Add ability to check if an event has callbacks Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Shift event topic publishing to a separate thread Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Fix formatting Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Test SequentialReader event callbacks Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Test SequentialWriter event callbacks Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Document bag events Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Check if joinable before joining Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Correct spelling mistake Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Correct spelling mistake Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Reduce time wasted waiting for test to reach intended behaviour Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Address test flakiness Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Improve size check Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Use constant for split size in tests Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Add missing header Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Allocate thread on the stack Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Add getters for number of messages per 'file' Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Replace magic number Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Address unstable OSX build Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Make path in test cross-platform Co-authored-by: Emerson Knapp <537409+emersonknapp@users.noreply.github.com> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Add missing virtual destructor Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Follow uncrustify suggestion Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Fix rebase errors Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Remove double include Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> Co-authored-by: Michael Orlov <morlovmr@gmail.com> Co-authored-by: Emerson Knapp <537409+emersonknapp@users.noreply.github.com> Co-authored-by: Michael Orlov <morlovmr@gmail.com> Co-authored-by: Emerson Knapp <537409+emersonknapp@users.noreply.github.com>
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-8-18-2022/27050/1 |
Closes #884