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

[improvement] consider move out management/namespace/local_kv_store.rs #1855

Closed
BohuTANG opened this issue Sep 16, 2021 · 12 comments
Closed
Labels
C-improvement Category: improvement

Comments

@BohuTANG
Copy link
Member

BohuTANG commented Sep 16, 2021

Summary

For now, local_kv_store is used by namespace and in management/namespace, user mod also need it:

impl<T> UserMgr<T>
where T: KVApi
{
    #[allow(dead_code)]
    pub fn new(kv_api: T) -> Self {
        UserMgr { kv_api }
    }
}
  1. Consider move it out from the namespace directory to store-api/src/impls/local_kv_store.rs? cc @drmingdrmer
  2. Refactor the utils.rs to:
    • Add async fn auth_user(&mut self, password: impl AsRef<[u8]>) -> bool to UserMgrApi trait (@BohuTANG )
    • Remove the NewUser (@BohuTANG )
    • Remove the utils.rs
@BohuTANG BohuTANG added the C-improvement Category: improvement label Sep 16, 2021
@BohuTANG BohuTANG changed the title [improvement] consider move management/namespace/local_kv_store.rs to management common [improvement] consider move out management/namespace/local_kv_store.rs Sep 16, 2021
@BohuTANG
Copy link
Member Author

BohuTANG commented Sep 16, 2021

For the user create interface:

impl<T> UserMgr<T>
where T: KVApi
{
    #[allow(dead_code)]
    pub fn new(kv_api: T) -> Self {
        UserMgr { kv_api }
    }
}

From the query layer, we don't know how to choose to use the local_kv_store or other KVApi who implements it, have seen that Winter use the local_kv_store in his patch directly:
https://github.com/datafuselabs/databend/pull/1731/files#diff-2362cb37f0b00a931dd7516059d7d7d64389e896ab2639c642414411b1a4f711R50

    async fn standalone_without_metastore(cfg: &Config) -> Result<ClusterDiscoveryRef> {
        let tenant = &cfg.query.tenant;
        let namespace = &cfg.query.namespace;
        let lift_time = Duration::from_secs(60);
        let local_store = LocalKVStore::new_temp().await?;
        let namespace_manager = NamespaceMgr::new(local_store, tenant, namespace, lift_time)?;

There is a bit confusion with this api exposed directly, we would like a wrapper like this(or Others, get one KVApi implement by the config):

// URI is one of the:
// "local://" -- this will use local_kv_store, mainly for local testing
// "xx://" -- this will use others kv store implement, used in production
let kv = KVApi::new(URI); 
let user_mgr = UserMgr::new(kv);
.... 

@drmingdrmer @dantengsky Would love to hear your thoughts :)

@dantengsky
Copy link
Member

dantengsky commented Sep 16, 2021

hi, for UserMgr that uses StoreClient as backend, here is a sample code snippet:

    let conf = Config::default();
    let remote_factory = RemoteFactory::new(&conf);
    let store_client_provider = remote_factory.store_client_provider();
    async {
        let client = store_client_provider.try_get_client().await?;
        let user_mgr = UserMgr::new(client);
        Ok::<(), common_exception::ErrorCode>(())
    };

@dantengsky
Copy link
Member

// "xx://" -- this will use others kv store implement, used in production
let kv = KVApi::new(URI);
let user_mgr = UserMgr::new(kv);

Since kv store channel needs authentication information( token or tls configs), it may not be convenient to embed those in a URI.

@dantengsky
Copy link
Member

will it be more friendly to use if we change

pub struct UserMgr<KV> {
    kv_api: KV,
}

to

pub struct UserMgr {
    kv_api: Arc<dyn KVApi>,
}

?

@drmingdrmer
Copy link
Member

For now, local_kv_store is used by namespace and in management/namespace, user mod also need it:

  1. Consider move it out from the namespace directory to store-api/src/impls/local_kv_store.rs? cc @drmingdrmer

It smells well.
When I added local_kv_store.rs I am not very sure where it should be. It looks like a test supporting component.
The new place for it looks intuitive and obvious.

There is a bit confusion with this api exposed directly, we would like a wrapper like this(or Others, get one KVApi implement by the config):

// URI is one of the:
// "local://" -- this will use local_kv_store, mainly for local testing
// "xx://" -- this will use others kv store implement, used in production
let kv = KVApi::new(URI); 
let user_mgr = UserMgr::new(kv);
.... 

I agree with @dantengsky .
For production use, a URI may not be an appropriate form to define a complex endpoint with auth, timeout etc.

For use in unit test, a URI would be expressive and convenient.

@BohuTANG
Copy link
Member Author

BohuTANG commented Sep 17, 2021

hi, for UserMgr that uses StoreClient as backend, here is a sample code snippet:

    let conf = Config::default();
    let remote_factory = RemoteFactory::new(&conf);
    let store_client_provider = remote_factory.store_client_provider();
    async {
        let client = store_client_provider.try_get_client().await?;
        let user_mgr = UserMgr::new(client);
        Ok::<(), common_exception::ErrorCode>(())
    };

@drmingdrmer @dantengsky
Looks good.
The main requirement here is that we want to do unit tests at query layer, and if we using this at query does it work for unit-test without metasotre service? It would be great if this could work too.

@drmingdrmer
Copy link
Member

For usage in unit test, it should be something like this? 🤔 :

    let t = tempfile::tempdir().expect("create temp dir to sled db");
    init_temp_sled_db(t);

    let api = LocalKVStore::new_temp().await?;

    let mut mgr = NamespaceMgr::new(api);
    let res = mgr
        .add_node(
            tenant_id.to_string(),
            namespace_id.to_string(),
            node.clone(),
        )
        .await?;

@BohuTANG
Are you suggesting letting remote_factory build a LocalKVStore in a unit test?

@BohuTANG
Copy link
Member Author

BohuTANG commented Sep 17, 2021

For usage in unit test, it should be something like this? 🤔 :

    let t = tempfile::tempdir().expect("create temp dir to sled db");
    init_temp_sled_db(t);

    let api = LocalKVStore::new_temp().await?;

    let mut mgr = NamespaceMgr::new(api);
    let res = mgr
        .add_node(
            tenant_id.to_string(),
            namespace_id.to_string(),
            node.clone(),
        )
        .await?;

@BohuTANG
Are you suggesting letting remote_factory build a LocalKVStore in a unit test?

Yes, exactly.
In query, it must choose one KVApi implement, it can't use local store in unit test and uses Remote store for production.
We prefer this is provided from an wrapper like Remote Factory:
let KVApi = RemoteFactory::create (local/remote)

BTW The type local or remote can be decided by the query from the config, something like a URI.

@drmingdrmer
Copy link
Member

BTW The type local or remote can be decided by the query from the config, something like a URI.

I like the idea of choosing an impl by querying a config entry.

In order to implement this, shall RemoteFactory build StoreClientProvider and local_kv_provider? Or just let RemoteFactory build StoreClient and LocalKVStore directly?

In literal, it seems like that RemoteFactory and StoreClientProvider both define the abstraction to build a remote KVApi.

@BohuTANG
Copy link
Member Author

BohuTANG commented Sep 17, 2021

BTW The type local or remote can be decided by the query from the config, something like a URI.

I like the idea of choosing an impl by querying a config entry.

In order to implement this, shall RemoteFactory build StoreClientProvider and local_kv_provider?
Or just let RemoteFactory build StoreClient and LocalKVStore directly?

RemoteFactory build them may be better?

impl RemoteFactory {
    pub fn new(conf: &Config) -> Self {
      let local_mode = conf.meta_store.meta_address.is_empty();
        client = if local_mode {
                local_kv_provider...
        } else {
           remote_kv_provider...
        };
    }

Another question is that, the query config have meta_config and store_config, and the RemoteFactory is about meta or the storage? This is another area where it is easy to get confused and perhaps we should separate them.
Just found the codes here:
https://github.com/datafuselabs/databend/blob/master/query/src/catalogs/impls/catalog/metastore_catalog.rs#L65

It should be conf.meta.meta_address.is_empty() not the conf.store.store_address.is_empty(), Maybe it's time for us to change them :)
Meta Client for the MetaStore service and the Remote Storge Client for the DFS or others datasource URI(If I understand it right)? This config may need by the DAL layer.
also cc @dantengsky

In literal, it seems like that RemoteFactory and StoreClientProvider both define the abstraction to build a remote KVApi.

@BohuTANG
Copy link
Member Author

#1876 Resolved

@drmingdrmer drmingdrmer reopened this Sep 20, 2021
@BohuTANG BohuTANG added this to To do in Store Refactor Sep 23, 2021
@drmingdrmer
Copy link
Member

Is this issue all right to close yet?

Store Refactor automation moved this from To do to Done Sep 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-improvement Category: improvement
Projects
No open projects
Development

No branches or pull requests

3 participants