Add SplitBagfile recording service. #1115
Conversation
rshanor
requested review from
gbiggs and
hidmic
and removed request for
a team
|
@MichaelOrlov is this what you had in mind? |
MichaelOrlov
changed the title
|
@ros-pull-request-builder retest this please |
There was a problem hiding this comment.
@rshanor Thanks for implementation. Yes it looks very close to my proposal among a few nitpicks see them in inline comments.
We also need to add coverage in unit tests for newly exposed API.
To add coverage on SequentialWriter level perhaps we can reuse existing tests just need to alter them a bit to call split_bagfiles explicitly.
Will need to replace ManualSplitWriter with rosbag2_cpp::writers::SequentialWriter here
And call
writer.split_bagfile() hereBTW we don't need ManualSplitWriter any more please delete it.
As regards to the integration tests for checking service call on transport layer there are will be a bit more work.
Will need to add new test in https://github.com/ros2/rosbag2/blob/rolling/rosbag2_transport/test/rosbag2_transport/test_record_services.cpp similar way as trigger_snapshot test implemented.
rosbag2/rosbag2_transport/test/rosbag2_transport/test_record_services.cpp
Lines 140 to 154 in c92010b
May need to parametrize storage_options_.snapshot_mode for new unit test.
Also please correct DCO signature. Our checker giving following error:
Commit sha: 5e5679e, Author: Rick Shanor, Committer: Rick Shanor; Expected "Rick Shanor rick@hadrian.co", but got "Rick Shanor rickshanor@gmail.com".
There was a problem hiding this comment.
@MichaelOrlov I think I addressed all your PR comments and I added some test coverage. Thank you for the feedback.
The RecordFixture.record_end_to_end_test_with_zstd_file_compression tests were failing locally for me, but I think that is unrelated to my change.
rshanor
requested review from
MichaelOrlov
and removed request for
gbiggs and
hidmic
|
Just as a note here (which is not to block the review at all), the services tests are unfortunately very flaky and therefore not relied on in the main CI. If you happen to have any ideas how to make them deterministic, which they currently are not, then that improvement would be greatly appreciated. However, we shouldn't block an improvement on preexisting bad infrastructure. |
|
@emersonknapp interesting. Are all the service tests flaky or just some of them? The test I added should be totally deterministic I think (just call a service once and make sure it returns). |
|
For context #862 The problem isn't in your specific test, but in the "successful service call" portion. I doubt you will want to dig into that whole thing right now. Just a note while we're discussing it - One part to pay attention to is the "sleep" in your test with some arbitrary duration. That should make you suspicious and suggest that the test can't be deterministic really, because timing can be different on different computers. (you may note the git blame and find my name on other such sleeps, like the preexisting service tests). Probably in general it is more reliable to find a way to wait on a specific condition, with a timeout. Though when those tests are truly broken, they take such a long time so say so! Whereas when they do it's generally very quick. Any tests involving the ros2 transport layer in action are susceptible to these problems, and I think all of them have been flaky because of it at one point or another. They have occasionally revealed issues with the dds implementations, but ideally we'd catch that much closer to the source, and not be using rosbag2 as an integration test for that portion of the stack! I don't have any solid ideas how to cut transport out and still regression-test that the transport-integration parts here work though. Ok verbose aside over - again, not blocking the review on it. |
|
Thanks for the link, but ya I dont think I have time to dig into that at the moment. I am new to ROS2 and DDS but have been able to write some pretty complicated (5+ node pick and place simulation) integration tests using the launch_testing package to test service interfaces without much flakiness. ACK you comment on sleeps. Its not strictly necessary for these tests. I mostly just copy-pasted from the above node to let the recorder do something. All my test checks is that SplitBagfile is called and that some bool in the recorder is set appropriately. |
There was a problem hiding this comment.
This LGTM
One non-blocking thing I would appreciate is removing ManualSplitSequentialWriter from https://github.com/ros2/rosbag2/blob/rolling/rosbag2_cpp/test/rosbag2_cpp/fake_data.hpp#L23 now that the method is truly public.
Removed! |
|
@MichaelOrlov PTAL when you get a chance. Thanks! Side question, how frequently are changes pushed to ROS Index? How long will I need to build from source to access these changes in humble? |
|
Well - this code is going into the head Rolling branch - it won't go to Humble at all unless we take additional steps. One caveat before talking process: We try to maintain API/ABI stability within a released ROS distribution. This PR only adds new APIs, which is fine for API stability, it's non-breaking and would warrant a minor version bump. I do worry though that based on the nature of the change it may not be ABI stable, that's very hard to guarantee whenever changing any class signatures. Part of me wants to avoid breaking that, however That said, to get this into Humble, once it is merged, we need to:
|
|
@rshanor I am sorry for the delay I didn't have time to reiterate on code review. From quick view on the test. I agree it's going to be flaky. Usually if test rely on some unconditional sleep inside, it is 99.99% probability that it will be flaky on CI. service request delivery may be asynchronous and
I might will have some better ideas when I will look on it more closely. BTW other unit tests with services mostly flaky in rosbag2 player and when the same service requests sending repeatedly in short time. Also those issue reproduces on all 3 DDS vendors and unlikely to be related to the DDS implementation. |
There was a problem hiding this comment.
@rshanor Thanks for your contribution.
Even though the tests not fully deterministic, I am inclined to move forward and merge them as is. I will create follow up PR with record_services tests redesign to make them more deterministic and running in shorter time.
Please remove sleep in split test.
Also please rebase your branch on top of latest rolling branch. We have braking changes in another packages which causing failure in tests.
Those failures most probably unrelated to your changes but we need to be assure.
rosbag2_transport/test/rosbag2_transport/test_record_services.cpp
Outdated
Show resolved
Hide resolved
We'd really prefer if we don't have known flaky tests. They make it difficult for us to run CI on every single package, since we have to keep some kind of list of "known flaky" tests, and then ignore those. It means that other regressions fall through the cracks, since people see a yellow CI, think "this is probably known flaky", and then merge. It also means that external contributors get confused by yellow CI tests. If you want to move forward with this regardless, I'll suggest at least marking the known flaky tests as xfail for now. That way they won't be run at all, and won't affect all upstream and downstream packages. |
|
@clalancette We don't know yet if it's going to be flay or not on CI. It's just tend to be flaky. But it didn't fail yet. @rshanor Could you please add |
Fixes ros2#1087 Tested from the command line and verified that below command closed one log file and opened another. ros2 service call /rosbag2_recorder/split_bagfile rosbag2_interfaces/srv/SplitBagfile Signed-off-by: Rick Shanor <rickshanor@gmail.com>
Also address PR comments from @MichaelOrlov and deal with rebase merge conflicts. Signed-off-by: Rick Shanor <rickshanor@gmail.com>
After making split_bagfile public, this class was no longer necessary. Signed-off-by: Rick Shanor <rickshanor@gmail.com>
Signed-off-by: Rick Shanor <rickshanor@gmail.com>
Done |
Done |
|
Gist: https://gist.githubusercontent.com/MichaelOrlov/0f53de688de23ccfcd61b54f67af1b7e/raw/35a8cd055cb943c4d6a1e6107b731767677094f7/ros2.repos
|
|
@clalancette I need your attention to test failures in I can't figure out what is wrong with Correction need to read |
There is a bug in recent flake8-deprecated that is causing this. See ros2/ci#682, which still needs to have CI run on it and merged. |
|
@clalancette Thanks for clarification. |
|
Trying to re-run CI
|
|
@MichaelOrlov thank you for relaunching. |
|
@Mergifyio backport humble |
* feat(rosbag2_cpp): Add SplitBagfile recording service. Fixes #1087 Tested from the command line and verified that below command closed one log file and opened another. ros2 service call /rosbag2_recorder/split_bagfile rosbag2_interfaces/srv/SplitBagfile Signed-off-by: Rick Shanor <rickshanor@gmail.com> * feat(rosbag2_cpp): Add unit tests for SplitBagfile feature. Also address PR comments from @MichaelOrlov and deal with rebase merge conflicts. Signed-off-by: Rick Shanor <rickshanor@gmail.com> * fix(rosbag2_cpp): Remove unnecessary ManualSplitSequentialWriter. After making split_bagfile public, this class was no longer necessary. Signed-off-by: Rick Shanor <rickshanor@gmail.com> * ci(rosbag2_transport): Mark test_record_services as xfail. Signed-off-by: Rick Shanor <rickshanor@gmail.com> Signed-off-by: Rick Shanor <rickshanor@gmail.com> (cherry picked from commit e6f7fd7) # Conflicts: # rosbag2_cpp/src/rosbag2_cpp/writer.cpp # rosbag2_cpp/test/rosbag2_cpp/fake_data.cpp # rosbag2_cpp/test/rosbag2_cpp/fake_data.hpp # rosbag2_interfaces/CMakeLists.txt # rosbag2_transport/CMakeLists.txt
✅ Backports have been created
|
* feat(rosbag2_cpp): Add SplitBagfile recording service. Fixes #1087 Tested from the command line and verified that below command closed one log file and opened another. ros2 service call /rosbag2_recorder/split_bagfile rosbag2_interfaces/srv/SplitBagfile Signed-off-by: Rick Shanor rshanor <rickshanor@gmail.com> * feat(rosbag2_cpp): Add unit tests for SplitBagfile feature. Also address PR comments from @MichaelOrlov and deal with rebase merge conflicts. Signed-off-by: Rick Shanor rshanor <rickshanor@gmail.com> * fix(rosbag2_cpp): Remove unnecessary ManualSplitSequentialWriter. After making split_bagfile public, this class was no longer necessary. Signed-off-by: Rick Shanor rshanor <rickshanor@gmail.com> * ci(rosbag2_transport): Mark test_record_services as xfail. Signed-off-by: Rick Shanor rshanor <rickshanor@gmail.com> Signed-off-by: Rick Shanor rshanor <rickshanor@gmail.com> (cherry picked from commit e6f7fd7) # Conflicts: # rosbag2_cpp/src/rosbag2_cpp/writer.cpp # rosbag2_cpp/test/rosbag2_cpp/fake_data.cpp # rosbag2_cpp/test/rosbag2_cpp/fake_data.hpp # rosbag2_interfaces/CMakeLists.txt # rosbag2_transport/CMakeLists.txt
* feat(rosbag2_cpp): Add SplitBagfile recording service. Fixes #1087 Tested from the command line and verified that below command closed one log file and opened another. ros2 service call /rosbag2_recorder/split_bagfile rosbag2_interfaces/srv/SplitBagfile Signed-off-by: Rick Shanor <rickshanor@gmail.com> * feat(rosbag2_cpp): Add unit tests for SplitBagfile feature. Also address PR comments from @MichaelOrlov and deal with rebase merge conflicts. Signed-off-by: Rick Shanor <rickshanor@gmail.com> * fix(rosbag2_cpp): Remove unnecessary ManualSplitSequentialWriter. After making split_bagfile public, this class was no longer necessary. Signed-off-by: Rick Shanor <rickshanor@gmail.com> * ci(rosbag2_transport): Mark test_record_services as xfail. Signed-off-by: Rick Shanor <rickshanor@gmail.com> Signed-off-by: Rick Shanor <rickshanor@gmail.com> (cherry picked from commit e6f7fd7) # Conflicts: # rosbag2_cpp/src/rosbag2_cpp/writer.cpp # rosbag2_cpp/test/rosbag2_cpp/fake_data.cpp # rosbag2_cpp/test/rosbag2_cpp/fake_data.hpp # rosbag2_interfaces/CMakeLists.txt # rosbag2_transport/CMakeLists.txt
|
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-2022-10-13/28213/1 |
* feat(rosbag2_cpp): Add SplitBagfile recording service. Fixes ros2#1087 Tested from the command line and verified that below command closed one log file and opened another. ros2 service call /rosbag2_recorder/split_bagfile rosbag2_interfaces/srv/SplitBagfile Signed-off-by: Rick Shanor <rickshanor@gmail.com> * feat(rosbag2_cpp): Add unit tests for SplitBagfile feature. Also address PR comments from @MichaelOrlov and deal with rebase merge conflicts. Signed-off-by: Rick Shanor <rickshanor@gmail.com> * fix(rosbag2_cpp): Remove unnecessary ManualSplitSequentialWriter. After making split_bagfile public, this class was no longer necessary. Signed-off-by: Rick Shanor <rickshanor@gmail.com> * ci(rosbag2_transport): Mark test_record_services as xfail. Signed-off-by: Rick Shanor <rickshanor@gmail.com> Signed-off-by: Rick Shanor <rickshanor@gmail.com>
Fixes #1087
Tested from the command line and verified that below command closed one log file and opened another
ros2 service call /rosbag2_recorder/split_bagfile
rosbag2_interfaces/srv/SplitBagfile
Signed-off-by: Rick Shanor rickshanor@gmail.com