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

Use async/await in the transaction APIs #91

Merged
merged 3 commits into from
Jul 31, 2019

Conversation

sticnarf
Copy link
Collaborator

After this commit, the library requires a nightly Rust compiler to build.

The required toolchain version is also updated in README.md and .travis.yml.

After this commit, the library requires a nightly Rust compiler to build.

The required toolchain version is also updated in README.md and .travis.yml.

Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Copy link
Collaborator

@nrc nrc left a comment

Choose a reason for hiding this comment

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

A couple of minor comments, otherwise lgtm

@@ -28,8 +28,6 @@ The client requires a Git dependency until we can [publish it](https://github.co

There are [examples](examples) which show how to use the client in a Rust program.

The examples and documentation use async/await syntax. This is a new feature in Rust and is currently unstable. To use async/await you'll need to add the feature flag `#![async_await]` to your crate and use a nightly compiler (see below).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you alter this comment rather than delete it please? I think it is important for users to know

pub fn current_timestamp(&self) -> Timestamp {
unimplemented!()
pub async fn current_timestamp(&self) -> Result<Timestamp> {
Arc::clone(&self.rpc).get_timestamp().await
Copy link
Collaborator

Choose a reason for hiding this comment

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

prefer self.rpc.clone()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm OK with both, but the official doc says "The Arc::clone(&from) syntax is the most idiomatic": https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#cloning-references

Copy link
Collaborator

Choose a reason for hiding this comment

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

In practice, I don't see the explicit form unless it is necessary for disambiguation (I had a quick search though the Rust repo and although it is hard to tell where .clone is used on an Arc, it does seem to be used more often than the explicit version).

Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf sticnarf requested a review from nrc July 31, 2019 08:11
Copy link
Member

@AndreMouche AndreMouche left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf
Copy link
Collaborator Author

Previously, CI failed because of a clippy bug. rust-lang/rust-clippy#4266 fixed it. Now, I have upgraded toolchain to latest.

Copy link
Collaborator

@nrc nrc left a comment

Choose a reason for hiding this comment

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

lgtm

@AndreMouche AndreMouche merged commit 91bc7f6 into tikv:master Jul 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants