Skip to content

refactor(ros-z): redesign endpoint builders#2639

Merged
schmidma merged 1 commit into
HULKs:mainfrom
schmidma:cache-builder-upgrade
Jun 27, 2026
Merged

refactor(ros-z): redesign endpoint builders#2639
schmidma merged 1 commit into
HULKs:mainfrom
schmidma:cache-builder-upgrade

Conversation

@schmidma

@schmidma schmidma commented Jun 17, 2026

Copy link
Copy Markdown
Member

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:

  • moves cache creation onto the subscriber builder via .cache(capacity)
  • renames service factories to service_server and service_client
  • keeps dynamic known-schema and auto-discovered subscriber APIs aligned, including raw sample delivery
  • migrates in-repository call sites, docs, examples, and tests to the new API

Motivation

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 --check
  • cargo check -p ros-z
  • cargo test -p ros-z --test endpoint_builder_api -- endpoint_builders_expose_single_question_mark_api --nocapture
  • cargo test -p ros-z --test pubsub dynamic_ -- --nocapture
  • cargo test -p ros-z --doc

@schmidma schmidma self-assigned this Jun 17, 2026
@github-project-automation github-project-automation Bot moved this to In Progress in Development Jun 17, 2026
@schmidma schmidma force-pushed the cache-builder-upgrade branch 4 times, most recently from bc680d3 to 1199327 Compare June 23, 2026 21:45
@schmidma schmidma removed their assignment Jun 23, 2026
@schmidma schmidma moved this from In Progress to Review in Development Jun 23, 2026
@schmidma schmidma marked this pull request as ready for review June 23, 2026 21:56
@schmidma schmidma enabled auto-merge June 23, 2026 21:56
@rmburg rmburg self-assigned this Jun 24, 2026
@rmburg

rmburg commented Jun 25, 2026

Copy link
Copy Markdown
Member

There are conflicts, likely caused by #2570. Please resolve

@schmidma schmidma force-pushed the cache-builder-upgrade branch from 1199327 to e337349 Compare June 25, 2026 15:54

@rmburg rmburg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@schmidma schmidma added this pull request to the merge queue Jun 27, 2026
Merged via the queue into HULKs:main with commit d66543e Jun 27, 2026
7 checks passed
@schmidma schmidma deleted the cache-builder-upgrade branch June 27, 2026 10:39
@github-project-automation github-project-automation Bot moved this from Review to Done in Development Jun 27, 2026
@schmidma

Copy link
Copy Markdown
Member Author

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)

@rmburg

rmburg commented Jun 27, 2026

Copy link
Copy Markdown
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants