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

'complete' option in PublicationCache, refactoring #688

Merged
merged 7 commits into from
Feb 2, 2024
Merged

Conversation

milyin
Copy link
Contributor

@milyin milyin commented Jan 26, 2024

  • added 'complete' option to query of PublicationCache
  • refactored access to SessionRef object: added mthods for creating publisher, queryable, etc to it. This allowed to access the SessionRef without code duplication just to satisfy borrow checker
  • Simplified SessionExt trait

Closes 'rust' checkbox of #668

@eclipse-zenoh-bot
Copy link
Contributor

@{login} If this pull request contains a bugfix or a new feature, then please consider using Closes #ISSUE-NUMBER syntax to link it to an issue.

@milyin milyin linked an issue Jan 26, 2024 that may be closed by this pull request
3 tasks
@milyin milyin added the enhancement Existing things could work better label Jan 27, 2024
@milyin milyin self-assigned this Jan 27, 2024
@OlivierHecart
Copy link
Contributor

This broke The usage of SessionExt with Arc<Session>.

#[async_std::main]
async fn main() {
    use zenoh::prelude::r#async::*;
    use zenoh_ext::SessionExt;

    let session = zenoh::open(Config::default())
        .res()
        .await
        .unwrap()
        .into_arc();

    let publication_cache = session
        .declare_publication_cache("key/expression")
        .res()
        .await
        .unwrap();

    async_std::task::spawn(async move {
        publication_cache.key_expr();
    });
}

@milyin
Copy link
Contributor Author

milyin commented Jan 31, 2024

@OlivierHecart Fixed missing implementation of SessionExt for Arc<Session>.
Also modified SessionDeclaration trait: now this trait is the only source of declaration functions. Scheme is exaclty the same as in SessionExt
@p-avital could you please look at it too? I hope this approach to using lifetimes is ok, but it's worth to check

@milyin milyin requested a review from p-avital January 31, 2024 00:34
@@ -499,135 +562,49 @@ impl Session {
pub fn config(&self) -> &Notifier<Config> {
self.runtime.config()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually a sneaky breaking change: the SessionDeclarations trait will have to be in scope for the methods to be called on Session, whereas this is currently unnecessary.

While I agree this makes a lot of sense, we should probably have a quick internal chat about whether or not we want to make that break (we have other more localized breaking changes in the next release, so I'm open to it)

Copy link
Contributor

@p-avital p-avital left a comment

Choose a reason for hiding this comment

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

The lifetime handling looks correct to me (GATs could have been another way to reach the solution, but this pattern also allows some nice variance tricks if we ever need them).

There's a sneaky breaking change by moving some of Session's method to a trait. I think moving to the trait makes a lot of sense, but we might also want to re-add the intrinsic methods to avoid the breaking change.

This review will be updated to approved when a decision has been made regarding this breaking change.

@milyin
Copy link
Contributor Author

milyin commented Feb 1, 2024

Michael Ilyin — Today at 11:04 AM
Thank you! Speaking on breaking change - I don't mind to repeat declaration functions in impl Session. I have only one argument against it:

If even core API, like declare_publisher is implemented in a trait outside the Session, this makes SessionExt extension first class citizen. It's now not a some hack to add new methods to Session, but completely valid and supported way to extend it.

Pierre 'Fox' Avital — Today at 11:08 AM
I agree, in general, I'm always in favour of traitifying, although that does mean more effort is necessary on docs to help the user navigate the API, as trait implementations are generally less obvious than methods

@milyin
Copy link
Contributor Author

milyin commented Feb 1, 2024

Talked with @Mallets - the only concern from his point of view is navigation on documentation. Created task to not forget to take care of it #710. Ohterwise he also thinks that this update is ok. Will integrate it today

@milyin milyin merged commit e8dca1e into main Feb 2, 2024
15 checks passed
@milyin milyin deleted the pubcache_complete branch February 2, 2024 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Existing things could work better
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Expose completeness option in PublicationCach in Rust, C, C++
4 participants