Conversation
xuhui-lu
force-pushed
the
xlu/sync-api
branch
2 times, most recently
from
dfe2d54 to
d9bfd5c
Compare
|
Thanks! I'm wondering if it is better to have a separate sync RawClient, instead of mixing sync and async API in the same struct. How do you like it? Also, could you please add appropriate example(s) in the README and/or under the example directory? It would best help the new users who are the expected users of sync API. |
I think it will be like a SyncRawClient which wraps the RawClient insides. And sure, I will add some examples into README doc. |
ekexium
left a comment
ekexium
left a comment
There was a problem hiding this comment.
Thanks! I have a few suggestions. And please fix the errors according to CI results.
Tips: make check and make unit-test can be helpful in your local environment.
src/raw/client.rs
Outdated
| } | ||
|
|
||
| impl SyncClient { | ||
| /// The Sync version of Client |
There was a problem hiding this comment.
The comments should be moved to the definition of the structure.
| /// The Sync version of Client | |
| /// The synchronous version of RawClient |
src/raw/client.rs
Outdated
| } | ||
|
|
||
|
|
||
| fn assert_non_atomic(&self) -> Result<()> { |
There was a problem hiding this comment.
I think the sync client doesn't have to implement these assert_ methods.
src/lib.rs
Outdated
| pub use crate::kv::{BoundRange, IntoOwnedRange, Key, KvPair, Value}; | ||
| #[doc(inline)] | ||
| pub use crate::raw::{lowering as raw_lowering, Client as RawClient, ColumnFamily}; | ||
| pub use crate::raw::{lowering as raw_lowering, Client as RawClient, SyncClient, ColumnFamily}; |
There was a problem hiding this comment.
| pub use crate::raw::{lowering as raw_lowering, Client as RawClient, SyncClient, ColumnFamily}; | |
| pub use crate::raw::{lowering as raw_lowering, Client as RawClient, SyncClient as SyncRawClient, ColumnFamily}; |
src/raw/client.rs
Outdated
| } | ||
|
|
||
| #[derive(Clone)] | ||
| pub struct SyncClient { |
There was a problem hiding this comment.
Shall we put the sync client in a separate file? It would look cleaner.
xuhui-lu
force-pushed
the
xlu/sync-api
branch
3 times, most recently
from
ffdfb74 to
c056a75
Compare
src/raw/sync_client.rs
Outdated
| /// let client = SyncRawClient::new(vec!["192.168.0.100"]).await.unwrap(); | ||
| /// # }); | ||
| /// ``` | ||
| pub async fn new<S: Into<String>>(pd_endpoints: Vec<S>) -> Result<SyncClient> { |
There was a problem hiding this comment.
It should also be synchronous.
src/raw/sync_client.rs
Outdated
| Self::new_with_config(pd_endpoints, Config::default()).await | ||
| } | ||
|
|
||
| pub async fn new_with_config<S: Into<String>>( |
src/raw/sync_client.rs
Outdated
| /// ```rust,no_run | ||
| /// # use tikv_client::SyncRawClient; | ||
| /// # use futures::prelude::*; | ||
| /// # futures::executor::block_on(async { |
There was a problem hiding this comment.
There shouldn't be async stuff in the docs and examples.
|
LGTM. Fix the fmt check please. |
|
hey @ekexium , could you please help take a look at it? THX! |
|
@xuhui-lu Could you take a look at these checks and make sure they pass? |
it is weird, it all passed before I merge master into it. Let me take a look. |
|
@xuhui-lu Oh right. A recent PR just changed the API |
cool, I have merge that change into my PR. It seems that the master branch's CI failed also. |
|
The master branch is fine. The failed action is going to be removed. #309 |
|
The failed action is about deploying the doc and is only executed on the master branch, so it shouldn't block merging this PR. You could fix the tests first. |
sure, unit test fixed. |
Signed-off-by: xuhui-lu <luxuhui12345@126.com>
|
LGTM. Could you sign off your last commit please? |
Signed-off-by: xuhui-lu <luxuhui12345@126.com>
|
I recently found that I need a synchronous interface with timeout in my development work. Will this PR continue to be developed? Can I continue this work? |
4 participants
The raw part work of #289