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] store client factory #1869 #1876
Conversation
Thanks for the contribution! Please review the labels and make any necessary changes. |
3cbd743
to
1435826
Compare
I see the unit test failure. It is caused by RPC timeout. Running the test We need to find a way to deal with these time sensitive tests. 🤔 https://github.com/datafuselabs/databend/pull/1876/checks?check_run_id=3638769792 |
Codecov Report
@@ Coverage Diff @@
## master #1876 +/- ##
=======================================
Coverage 70% 70%
=======================================
Files 639 631 -8
Lines 38280 37814 -466
=======================================
- Hits 27088 26805 -283
+ Misses 11192 11009 -183
Continue to review full report at Codecov.
|
44606e1
to
78e5f48
Compare
/lgtm 👍 |
Approved! Thank you for the PR @dantengsky |
CI Passed |
5 similar comments
CI Passed |
CI Passed |
CI Passed |
CI Passed |
CI Passed |
impl KVApi for LocalKVStore { | ||
async fn upsert_kv( | ||
&self, | ||
key: &str, | ||
matcher: MatchSeq, | ||
value: Option<Vec<u8>>, | ||
value_meta: Option<KVMeta>, | ||
) -> Result<UpsertKVActionResult> { | ||
let (prev, result) = { | ||
let mut inner = self.inner.write(); | ||
|
||
if let Some((seq, v)) = inner.kv.get(key) { | ||
let r = matcher.match_seq(*seq); | ||
let prev = (*seq, v.clone()); | ||
if r.is_ok() { | ||
// seq match | ||
let mut new_v = v.clone(); | ||
new_v.meta = value_meta; | ||
new_v.value = value.unwrap_or_default(); | ||
let new_seq = inner.next_seq(); | ||
let result = (new_seq, new_v); | ||
inner.kv.insert(key.to_owned(), result.clone()); | ||
(Some(prev), Some(result)) | ||
} else { | ||
// seq not match, | ||
// store state machine use a pair of identical values to indicates | ||
// that nothing changed, we simulate it. | ||
let c = prev.clone(); | ||
(Some(prev), Some(c)) | ||
} | ||
} else { | ||
// key not found, let's insert | ||
let new_v = KVValue { | ||
meta: value_meta, | ||
value: value.unwrap_or_default(), | ||
}; | ||
let new_seq = inner.next_seq(); | ||
let result = (new_seq, new_v); | ||
inner.kv.insert(key.to_owned(), result.clone()); | ||
(None, Some(result)) | ||
} | ||
}; | ||
Ok(UpsertKVActionResult { prev, result }) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A re-implementation of the meta-kv is not guaranteed to be consistent with the remote meta service.
Somebody who is not aware of there are two implementations may have changed one of them without updating the other one accordingly.
That's why a local kv should use a embedded StateMachine
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change log:
a. there are circular dependencies if we place StateMachine based LocalKVStore into crate store-api-sdk
b. also, make common-xxx depend on crate store is not a good idea IMO
c. let's rollback to the former implementations after the dependency issues resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We both know about the reasons. A better way is to solve the dependency cycle first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not know what exactly the deps issues are before working on this pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw,maybe we should also try harder to NOT leak internal concepts like StateMachine to common-xxx directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw,maybe we should also try harder to NOT leak internal concepts like StateMachine to common-xxx directly
It seems we need a wrapper api of StateMachine?
I hereby agree to the terms of the CLA available at: https://databend.rs/policies/cla/
Summary
fix:
Changelog
Summary
Crate
common-fligts
has been renamed tostore-api-sdk
since common-xxxx are not supposed to depend on crate
query::configs::Config
, we can not useConfig
directly instore-api-sdk
.A converter form
Config
toStoreClientConf
is provide in crate
query
.The mock of KVStoreApi --
LocalKVStore
is provided by cratestore-api-sdk
LocalKVStore
has been changed to a simple in-memory implementationThe idea of re-use code of store-state-machine is great, but, currently,
LocalKVStore
into cratestore-api-sdk
common-xxx
depend on cratestore
is not a good idea IMOThe mock is provided as a part of a non-default feature of crate
store-api-sdk
crates that depend on it, like
common-management
should declare an extra dev dependency in cargo.toml, like thisIt has been added to the cargo.toml of crate
common-management
management::namespace::local_kv_store_test.rs
now usingcommon_store_api_sdk::mocks::LocalKVStore
UserMgr Api changed
to runtime dispatching style:
Since, the provider method signature
fn try_get_kv_client(&self) -> Result<Arc<dyn KVApi>>
indicates that the caller site does not know the concrete type of which implementedKVApi
.Previous static dispatching style of using Store Apis can also be implemented but seems to be not user-friendly (to compose the components).
Same modification goes to
UserMgr
Store Apis slightly changed
trait
StoreApis
are removed since we are going to split theStore
services into multiple services.KV/Meta/Apis
no longer take&mut self
, since they are all wrapped inArc
while being passed around.Instead, the flight client is cloned(which is lightweight, according to this), before calling the flights API.
Related Issues
Fixes #1869
Test Plan
Unit Tests
Stateless Tests