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 new non periodic last sample miss detection mechanism #1861

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

OlivierHecart
Copy link
Contributor

Introduces a new mechanism to allow last sample miss detection.
The mechanism reuses AdvPubSub heartbeat message but sent with CongestionControl::Block instead of periodically.

@OlivierHecart OlivierHecart added the new feature Something new is needed label Mar 27, 2025
Copy link

codecov bot commented Mar 27, 2025

Codecov Report

Attention: Patch coverage is 44.44444% with 20 lines in your changes missing coverage. Please review.

Project coverage is 71.72%. Comparing base (2e4f3ed) to head (1fcb45c).
Report is 6 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
zenoh-ext/src/advanced_publisher.rs 44.44% 20 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1861      +/-   ##
==========================================
+ Coverage   70.96%   71.72%   +0.75%     
==========================================
  Files         360      363       +3     
  Lines       65228    67192    +1964     
==========================================
+ Hits        46291    48191    +1900     
- Misses      18937    19001      +64     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@fuzzypixelz fuzzypixelz left a comment

Choose a reason for hiding this comment

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

It's not immediately clear if heartbeat and heartbeat_change are mutually exclusive.

@OlivierHecart
Copy link
Contributor Author

It's not immediately clear if heartbeat and heartbeat_change are mutually exclusive.

Clarified.

@milyin
Copy link
Contributor

milyin commented Mar 28, 2025

As far as I see there is no way to user to read the heartbeat mode and hearbeat timeout from the MissDetectionConfig. This makes user unaware which settings are selected when MissDetectionConfig is created with default()
This also disallows zenoh-c API to create ze_advanced_publisher_sample_miss_detection_options_t with default values from the MissDetectionConfig::default()
I propose to:

  • add enum e.g. HeartbeatMode with values Sporadic(Duration) and Periodic(Duration)
  • rename heartbeat to periodic_heartbeat
  • make method heartbeat_mode (or just hearbeat) which return Option<HeartbeatMode>

This will also allow to more directly reflect the API in zenoh-c like this:

#[repr(C)]
pub enum ze_advanced_publisher_heartbeat_mode_t {
    PERIODIC,
    SPORADIC,
}
#[repr(C)]
pub struct ze_advanced_publisher_sample_miss_detection_options_t {
    pub is_enabled: bool,
    pub heartbeat_mode: ze_advanced_publisher_heartbeat_mode_t,
    pub heartbeat_period_ms: u64,
}

@milyin
Copy link
Contributor

milyin commented Mar 28, 2025

My primary concern is that on Rust side it's possible to add more variants of heartbeat: the bool is hidden in implementation and for user only methods heartbeat and sporadic_heartbeat are exposed. This doesn't exclude the possibility to add more methods like e.g. magic_heartbeat. So on the zenoh-c side it would be wrong to represent this heartbeat mode as bool, I have to add enum anyway. So it would be better to have corresponding enum on Rust side too.

@milyin
Copy link
Contributor

milyin commented Mar 28, 2025

Discussed with @OlivierHecart : as the default is None - no heartbeat, the read access is not necessary. So it's ok to leave Rust API as is for now. Though C API still needs the enum which don't have the Rust equivalent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature Something new is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants