refactor: rm async_trait and add trait_variant#186
Conversation
| pub trait Catalog: std::fmt::Debug { | ||
| /// see https://blog.rust-lang.org/2023/12/21/async-fn-rpit-in-traits.html | ||
| #[trait_variant::make(Catalog: Send)] | ||
| pub trait LocalCatalog: std::fmt::Debug { |
There was a problem hiding this comment.
Why we need this LocalCatalog variant? I think it would be enough to have a thread safe one.
There was a problem hiding this comment.
I think the better approach would be like FileWriter trait, which adds Send for each return type:
pub trait Catalog: Debug + Send {
fn list_namespace(&self, parent: Option<&NamespeceIdent>) -> impl Future<Output =
Result<Vec<NamespaceIdent>>> + Send;
}There was a problem hiding this comment.
My goal was to minimize code changes as much as possible. There are 13 methods and 2 implementations of Catalog, while trait_variant is a one-line change. I personally prefer async fn over impl Future since it looks neater. But it is true that LocalCatalog is never used.
There was a problem hiding this comment.
I'm hesitating adding this LocalCatalog trait. Also async fn seems much more concise to me. Maybe we should close this pr before finding a better approach?
There was a problem hiding this comment.
They are adding a rewrite feature to rewrite a trait instead of creating 2 variants. We can wait till it's done.
There was a problem hiding this comment.
Yeah, that's great! Let's wait for this.
|
cc @odysa Should we close this? |
|
@liurenjie1024 Yes, we can close this and reopen it after they release the rewrite feature. |
2 participants
Close #139
Use
trait_variant::maketo support async fn in pub traits. It creates 2 traits ----LocalCatalogfor single thread andCatalogwithSendfor multithreaded runtime.