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

refactor: Remove async_trait in Catalog trait. #139

Closed
liurenjie1024 opened this issue Dec 29, 2023 · 7 comments
Closed

refactor: Remove async_trait in Catalog trait. #139

liurenjie1024 opened this issue Dec 29, 2023 · 7 comments
Labels
enhancement New feature or request

Comments

@liurenjie1024
Copy link
Collaborator

liurenjie1024 commented Dec 29, 2023

After #140 get merged, we will be able to use async fn in trait, and async_trait macro is no longer needed.

@hiirrxnn
Copy link
Contributor

Could I work on this ? Please tell me what precisely is to be done here?

@liurenjie1024
Copy link
Collaborator Author

Could I work on this ? Please tell me what precisely is to be done here?

The release of rust 1.75 enables a fancy feature so that we no longer need to use async_trait macro, you can read this blog for reference.

You can also see the FileWriter trait in this pr as an example.

@jdockerty
Copy link
Contributor

jdockerty commented Jun 1, 2024

Is there any movement on this one? I see there was a prior PR to close, but it was closed pending upstream releases.

If there's nobody working on it anymore, I would like to take a shot at it if that's okay 👍

For mine/anyone else's reference, is there a preference to using impl Future<Output = ...> or maintaining the async fn signatures with trait_variant crate?

@liurenjie1024
Copy link
Collaborator Author

Is there any movement on this one? I see there was a prior PR to close, but it was closed pending upstream releases.

If there's nobody working on it anymore, I would like to take a shot at it if that's okay 👍

For mine/anyone else's reference, is there a preference to using impl Future<Output = ...> or maintaining the async fn signatures with trait_variant crate?

Hi, @jdockerty Thanks for you interest, I think we can go on this work after rust-lang/impl-trait-utils#27 is release on new rust release.

@Xuanwo
Copy link
Member

Xuanwo commented Jun 10, 2024

I personally do not favor adopting async in trait for the Catalog API because it compromises object safety. Users will no longer be able to use Box<dyn Catalog>. All APIs related to Catalog should include generic parameters.

@Xuanwo
Copy link
Member

Xuanwo commented Jul 5, 2024

I propose closing this issue as it's unlikely we'll remove async_trait from Catalog until easier methods for implementing object-safe trait objects are available. Additionally, catalog API calls are usually not on the critical path in most of our use cases. Adding such a maintenance burden isn't worth it.

cc @liurenjie1024 @jdockerty

@liurenjie1024
Copy link
Collaborator Author

+1, we can close this first.

@liurenjie1024 liurenjie1024 closed this as not planned Won't fix, can't repro, duplicate, stale Jul 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants