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.

Sign up for GitHub

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

Jump to bottom

Add sync API for raw client #301

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add sync API for raw client #301

wants to merge 6 commits into from
+297 −2

Conversation

xuhui-lu
Copy link

@xuhui-lu xuhui-lu commented Jun 13, 2021 •
edited by ekexium
Loading

The raw part work of #289

@xuhui-lu xuhui-lu force-pushed the xlu/sync-api branch 2 times, most recently from dfe2d54 to d9bfd5c Compare June 13, 2021 18:28
@ekexium
Copy link
Collaborator

ekexium commented Jun 15, 2021

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.

@xuhui-lu
Copy link
Author

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
ekexium reviewed Jun 29, 2021
View reviewed changes
Copy link
Collaborator

@ekexium ekexium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments should be moved to the definition of the structure.

Suggested change
/// The Sync version of Client
/// The synchronous version of RawClient

src/raw/client.rs Outdated
}


fn assert_non_atomic(&self) -> Result<()> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the sync client doesn't have to implement these assert_ methods.

src/lib.rs Outdated
@@ -122,7 +122,7 @@ pub use crate::backoff::Backoff;
#[doc(inline)]
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};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
@@ -563,3 +628,117 @@ impl Client {
self.atomic.then(|| ()).ok_or(Error::UnsupportedMode)
}
}

#[derive(Clone)]
pub struct SyncClient {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we put the sync client in a separate file? It would look cleaner.

@xuhui-lu xuhui-lu force-pushed the xlu/sync-api branch 3 times, most recently from ffdfb74 to c056a75 Compare July 3, 2021 08:27
@xuhui-lu xuhui-lu requested a review from ekexium July 3, 2021 08:30
ekexium
ekexium reviewed Jul 5, 2021
View reviewed changes
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> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>>(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here

src/raw/sync_client.rs Outdated
/// ```rust,no_run
/// # use tikv_client::SyncRawClient;
/// # use futures::prelude::*;
/// # futures::executor::block_on(async {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There shouldn't be async stuff in the docs and examples.

@xuhui-lu xuhui-lu requested a review from ekexium July 5, 2021 09:46
@ekexium
Copy link
Collaborator

ekexium commented Jul 5, 2021

LGTM. Fix the fmt check please.

@ekexium ekexium mentioned this pull request Jul 5, 2021
Open
3 tasks
@xuhui-lu
Copy link
Author

hey @ekexium , could you please help take a look at it? THX!

@ekexium
Copy link
Collaborator

ekexium commented Jul 14, 2021

@xuhui-lu Could you take a look at these checks and make sure they pass?
In your local environment, you could use make check and make unit-test to run these checks.
image

@xuhui-lu
Copy link
Author

@xuhui-lu Could you take a look at these checks and make sure they pass?
In your local environment, you could use make check and make unit-test to run these checks.
image

it is weird, it all passed before I merge master into it. Let me take a look.

@ekexium
Copy link
Collaborator

ekexium commented Jul 14, 2021

@xuhui-lu Oh right. A recent PR just changed the API

@xuhui-lu
Copy link
Author

@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.

@ekexium
Copy link
Collaborator

ekexium commented Jul 14, 2021

The master branch is fine. The failed action is going to be removed. #309

@xuhui-lu
Copy link
Author

#309

hey @ekexium, when will the fix patch be merged? And then I could solve these test failures.

@ekexium
Copy link
Collaborator

ekexium commented Jul 21, 2021

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.

@xuhui-lu
Copy link
Author

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.

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 9, 2021
@ekexium
Copy link
Collaborator

ekexium commented Aug 9, 2021

LGTM. Could you sign off your last commit please?

Fix cargo clippy error
bd40a0b 
Signed-off-by: xuhui-lu <[email protected]>
@Smityz
Copy link
Contributor

Smityz commented Jun 14, 2023

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekexium ekexium ekexium approved these changes

Assignees
No one assigned
Labels
status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xuhui-lu @ekexium @Smityz @ti-srebot