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

Add node name to the Read(Write)SplitEvent message #1609

Merged
merged 1 commit into from Apr 16, 2024

Conversation

MichaelOrlov
Copy link
Contributor

  • Rationale:
    When running multiple instances of the rosbag2 recorder or player there are no way to know from what instance arrived notification about files split event. With node name in event message we will be able to identify instance of the recorder or player from which event arrived.

@MichaelOrlov MichaelOrlov requested a review from a team as a code owner April 14, 2024 20:14
@MichaelOrlov MichaelOrlov requested review from gbiggs, james-rms, clalancette, emersonknapp and fujitatomoya and removed request for a team April 14, 2024 20:14
@MichaelOrlov
Copy link
Contributor Author

@fujitatomoya @clalancette could you please review/approve this small PR.
I would like to land it to the Jazzy release.

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.

having fully qualified node name to the event publisher makes sense to me.

@MichaelOrlov
Copy link
Contributor Author

Gist: https://gist.githubusercontent.com/MichaelOrlov/dea13b6d6c78211829cf64ab14776a38/raw/5f18de8a350a609a04881a4b1c99815361bdb75b/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_interfaces rosbag2_transport rosbag2_tests
TEST args: --packages-above rosbag2_interfaces rosbag2_transport rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/13700

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

- Rationale: When running multiple instances of the rosbag2 recorder or
player there are no way to know from what instance arrived notification
about files split event. With node name in event message we will be
able to identify instance of the recorder or player from which event
arrived.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov MichaelOrlov force-pushed the morlov/add_node_name_to_split_event_msg branch from 536e9f1 to 5a4d923 Compare April 15, 2024 01:55
@MichaelOrlov
Copy link
Contributor Author

@clalancette @emersonknapp Could some of you please additionally formally approve this PR to be able to merge it?

@MichaelOrlov
Copy link
Contributor Author

@marcoag Could you please approve and merge this PR? We need it for Jazzy.

@MichaelOrlov
Copy link
Contributor Author

@clalancette @marcoag friendly ping here.

@clalancette
Copy link
Contributor

I have to say that this seems like an odd change. A lot of what ROS is built around is the concept of anonymous pub/sub, and this is explicitly breaking that.

Can I have more of an explanation of the reason for this change? Also, are there other ways we can accomplish the same thing?

@MichaelOrlov
Copy link
Contributor Author

MichaelOrlov commented Apr 15, 2024

@clalancette Ok here is more context:
When rosbag2 recorder recording with the option to split recorded files by size or by duration, when the split event happens we publish a message on the specific topic "events/write_split" where we specify closed and newly opened files.
We need this sort of notification for third-party nodes or scripts which will wait for such split messages and upon arrival can process freshly written rosbag2 file while the other one still writing.
One of the common use cases is uploading to the cloud freshly written files or doing custom compression or encryption.

Now imagine if we would have multiple instances of the rosbag2 recorder running in parallel. A valid case to split writing to the separate NVME drives to achieve higher throughput or not mess up high-frequency topics like IMU with the slower camera or point cloud images. The monitoring script/node which is running in a separate process needs to know what instance of the rosbag2 recorder sent a notification. It might be a different logic for processing files from different instances. One may need encryption another one may not for example.

As regards "Also, are there other ways we can accomplish the same thing?" - Hmm, I don't know on top of my head.

Honestly, I don't quite understand why the concept of the anonymous pub/sub could be relevant or somehow matter in this case.

@clalancette
Copy link
Contributor

Honestly, I don't quite understand why the concept of the anonymous pub/sub could be relevant or somehow matter in this case.

The reason I'm hesitant here is because it breaks the idea of being anonymous, which is a core design philosophy of ROS 2. That is, the node name now matters, so doing a remap during playback, for instance, won't work anymore.

I need to think about this more, and we should probably discuss it in the ROS 2 meeting tomorrow. I very well may be wrong, but I don't think we should rush this in right now.

@MichaelOrlov
Copy link
Contributor Author

Update: On today's ROS 2 sync meeting we decided to merge this PR as is for user convenience.

However, the alternative way to detect the sender node name on the receiver side could be as proposed in the ros2/geometry2#545

@MichaelOrlov
Copy link
Contributor Author

@clalancette I need your formal approval to be able to merge this PR

@MichaelOrlov MichaelOrlov merged commit e3cfc25 into rolling Apr 16, 2024
14 checks passed
@delete-merged-branch delete-merged-branch bot deleted the morlov/add_node_name_to_split_event_msg branch April 16, 2024 16:59
@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/freeze-of-ros-2-base-packages-upcoming-branch-and-tutorial-party-for-jazzy-jalisco/37191/2

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.

None yet

4 participants