bugfix: Global MetaGrpcClient cause dispatch drop error

[ariesdevil]

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

Bugfix: Global MetaGrpcClient cause dispatch drop error.

• move MetaGrpcClient from SessionManager to Session to avoid cross-runtime client clone.

NOTE
The current implementation leaves two issues

1. the Drop trait that impl by SessionRef using block_on to call async fn.
2. As of current existing query processing logic of HTTP handler is different from others(MySQL, ClickHouse), so the SessionManager still has a Client that provides HTTP handler to use.

Changelog

• Bug Fix

Related Issues

Fixes #4347

Test Plan

Unit Tests

Stateless Tests

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/databend/databend/7c3JkV9riAtfm8dyWkEgJjSYZiWq
✅ Preview: Canceled

[Deployment for 157ff97 canceled]

[mergify]

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

[youngsofun]

GLOBAL_META_EMBEDDED not inited
MetaEmbedded::new_temp() called mutli times.

https://github.com/youngsofun/databend/commits/zyj_dev

test passed after it is inited

[youngsofun]

@ariesdevil

block_on inside async block may lead to dead lock, is this the case here？

seanmonstar/reqwest#1215 (comment)

cc @zhang2014 is this the reason for https://github.com/datafuselabs/databend/pull/4317/files? authenticate() is called in an async fn init()

[ariesdevil]

GLOBAL_META_EMBEDDED not inited MetaEmbedded::new_temp() called mutli times.

youngsofun/databend@zyj_dev (commits)

test passed after it is inited

Previously We using session_mgr to create KVApi that only one tmp meta_store will created.

Now we change to create KVApi by session, so each session will create it's own tmp meta_store
and this will cause first query insert first tmp meta_store and second query read it's own store
then cannot fetch the previous result.

When using global init here it can be passed if we just run this test only, but if we run with make test, it will failed with following error msg:

thread 'servers::http::http_query_handlers::test_auth_basic' panicked at 'sled db is already initialized with temp dir: /tmp/.tmpOyUpf9, can not re-init with path __global_meta',

[youngsofun]

try to run globalinit only once in tests？

including

1.utils like once staic
1.check and call in sessionmgr builder for all tests
1.hack global init itself，afterall embed is mainly for test？

[ariesdevil]

try to run globalinit only once in tests？

including

1.utils like once staic 1.check and call in sessionmgr builder for all tests 1.hack global init itself，afterall embed is mainly for test？

We should avoid using global init in unit tests, global init is just used when query node init and no meta node is configured in the config file. Let's find a way solve it.

[BohuTANG]

await?

[ariesdevil]

This function is eventually called by drop function that impl Drop trait, async drop still in rust roadmap...

[drmingdrmer]

Is it possible to make active_sessions an std::sync::Mutex?
Then the await can be get rid of.

But using a sync-mode mutex introduces some retry-loops when inserting items into it.
Because it can not be held across an await.

[flaneur2020]

i guess std::sync::Mutex is ok in this case, but we should be careful NOT to introduce any async/await point inside the mutex guard.

1. sync mutex lock
2. await yield out to another coroutine
3. another coroutine tries to acquire the lock, and dead lock

however, it's a good practise to not do any blocking operations (includes logging) inside a mutex guard

[youngsofun]

@ariesdevil

We should avoid using global init in unit tests,

got it.

so we need:

1. diff test case use diff MetaEmbedded.
2. multi session in a test case should use the same one. todo.

[youngsofun]

@ariesdevil wrt http handler， I prefer to retain global auth_mgr/user_mgr. when not related to diff runtimes, it is safe and efficient to have a global conn pool, we can add some comment avoid miss using it.

---

multi session in a test case should use the same MetaEmbedded

I think of 2 ways

1. keep an the instance in session_manager, need refactor
2. a global dict to hold MetaEmbedded instance (bad), assign a uniq id to some field of config when build session_manager, get_or_create(uniq id) instead of new_temp(),

[drmingdrmer]

It looks good to me.
But I'm not familiar with the underlying logic around session.
So let experts approve.