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

RecoveryConfig in AdvancedSubscriber needs to be refined #1724

Open
YuanYuYuan opened this issue Jan 16, 2025 · 1 comment
Open

RecoveryConfig in AdvancedSubscriber needs to be refined #1724

YuanYuYuan opened this issue Jan 16, 2025 · 1 comment
Labels
api sync Synchronize API with other bindings release Part of the next release

Comments

@YuanYuYuan
Copy link
Contributor

YuanYuYuan commented Jan 16, 2025

Describe the release item

We expect the RecoveryConfig to be configured as either the heartbeat or the periodic_queries in our current design. To achieve this, we use the following magic trick and introduce some condition checks in the runtime.

#[derive(Default, Clone, Copy)]
pub struct Unconfigured;
#[derive(Default, Clone, Copy)]
pub struct Configured;

#[zenoh_macros::unstable]
pub struct RecoveryConfig<T> {
    periodic_queries: Option<Duration>,
    heartbeat: Option<()>,
    phantom: PhantomData<T>,
}
#[zenoh_macros::unstable]
impl RecoveryConfig<Unconfigured> {
    pub fn periodic_queries(self, period: Duration) -> RecoveryConfig<Configured> {
        RecoveryConfig {
            periodic_queries: Some(period),
            heartbeat: None,
            phantom: PhantomData::<Configured>,
        }
    }
    pub fn heartbeat(self) -> RecoveryConfig<Configured> {
        RecoveryConfig {
            periodic_queries: None,
            heartbeat: Some(()),
            phantom: PhantomData::<Configured>,
        }
    }
}

However, users could construct an empty config by RecoveryConfig::default surprisingly marked as Configured. For instance,

let subscriber = session
    .declare_subscriber(key_expr)
    .history(HistoryConfig::default().detect_late_publishers())
    .recovery(RecoveryConfig::default())  // <-- with two default None values inside and resolved as Configured
    .subscriber_detection()
    .await
    .unwrap();

this means recovery is used but does nothing.

Moreover, it doesn't align with the c/cpp behavior. https://github.com/eclipse-zenoh/zenoh-c/blob/cdc53527c88c5d27b35fa5d4552560a06f287e44/src/advanced_subscriber.rs#L91-L105
The default recovery option is set to heartbeat and users can't configure it explicitly since it's not exposed in c/cpp due to the generic type used in Rust.

I would suggest using the following isomorphic enum to avoid unnecessary complexity and solve the above API misalignment issue effortlessly.

enum RecoveryConfig {
   PeriodicQueries(Option<Duration>),
   #[default]
   Heartbeat
}
@YuanYuYuan YuanYuYuan added the release Part of the next release label Jan 16, 2025
@YuanYuYuan YuanYuYuan added the api sync Synchronize API with other bindings label Jan 16, 2025
@OlivierHecart
Copy link
Contributor

RecoveryConfig does not need to be configured as either the heartbeat or the periodic_queries.

Recovery by default detects misses through the sequence_numbers and so don't need heartbeat or periodic_queries. This is what you get with .recovery(RecoveryConfig::default()). This works well for periodic publications, but for sporadic publications you may miss the last sample and not be able to detect it. We provide 2 ways of detecting last sample miss: heartbeat or periodic_queries that we want to be mutually exclusive (we don't want to allow users to activate both).

So to summarize: we currently have 3 recovery modes:

  • recovery(RecoveryConfig::default()): sample miss detection based on sequence_numbers only.
  • recovery(RecoveryConfig::heartbeat()): sample miss detection based on sequence_numbers and heartbeat.
  • recovery(RecoveryConfig:: periodic_queries()): sample miss detection based on sequence_numbers and periodic queries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api sync Synchronize API with other bindings release Part of the next release
Projects
Status: No status
Development

No branches or pull requests

2 participants