refactor(ros-z): redesign endpoint builders#2639
Conversation
bc680d3 to
1199327
Compare
|
There are conflicts, likely caused by #2570. Please resolve |
1199327 to
e337349
Compare
rmburg
left a comment
There was a problem hiding this comment.
LGTM. Please split up PRs of this size in the future if possible, large diffs really slow down the review process and make it harder to review thoroughly.
|
I really thought hard on this one... I know this was a huge refactor. But I did not see any clear split lines to chop this PR into smaller pieces. The whole idea was to re-iterate and align all the builders. In that consequence one of my options was to have multiple iterations of PRs to the final goal. But this sounded like even more work, because a potential reviewer would have had to review intermediate state which is not supposed to be the final state... So, I'm sorry for that, and thank you for fighting through this. If you have any ideas or suggestions on how this would have been a better split, let me know. Maybe I can learn something from that for future PRs. (still not sure yet where the golden line is for that in our team) |
|
absolutely! splitting large PRs is often challenging (in particular those containing large architectural changes!), I struggle with it myself and would also like to find guidelines on how to do it consistently. One idea I had for this PR in particular was to do subscribers and publishers separately, but I don't know if this would have actually worked or if there are some inter-dependencies that would make this approach messy. |
Summary
Redesign ros-z endpoint factories around infallible builders. Callers now configure publishers, subscribers, services, dynamic endpoints, and caches first, then handle fallible runtime work at
.build().await?.This PR also:
.cache(capacity)service_serverandservice_clientMotivation
The old endpoint factories mixed cheap configuration with runtime work. Callers had to handle errors both when creating endpoint factories and when building runtime resources, which made endpoint setup inconsistent across publishers, subscribers, services, caches, and dynamic endpoints.
The new API uses one builder model: configure first, then build. Topic qualification, schema validation, type/hash checks, schema-service registration, Zenoh declarations, and transient-local setup now happen at build time. That gives callers one place to handle endpoint creation failures and makes cache construction fit the subscriber flow it depends on.
The repo-wide migration is included because the API change and call-site update need to land together to keep the workspace coherent.
Test Plan
cargo fmt --all --checkcargo check -p ros-zcargo test -p ros-z --test endpoint_builder_api -- endpoint_builders_expose_single_question_mark_api --nocapturecargo test -p ros-z --test pubsub dynamic_ -- --nocapturecargo test -p ros-z --doc