Skip to content
GitHub no longer supports this web browser. Learn more about the browsers we support.
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

cop: change handle_request interface to async #6250

Merged
merged 17 commits into from Dec 20, 2019

Conversation

@sticnarf
Copy link
Contributor

sticnarf commented Dec 16, 2019

What have you changed?

This PR simplifies handle_unary_request_impl using async/await. It also changes handle_request to an async function so that we may interrupt it in the future.

Due to limitations of async/await and future compat layer, this PR introduces two more allocations. Benchmark shows there's no significant influence.

sysbench 32 tables x 1M rows
workload: read_only without point get (all requests are small coprocessor requests)

Master:

threads qps p95(ms)
32 23117.94 11.65
128 27036.21 45.79

This PR:

threads qps p95(ms)
32 23181.34 11.65
128 26904.93 45.79

What is the type of the changes?

  • Engineering (engineering change which doesn't change any feature or fix any issue)

How is the PR tested?

  • Unit test
  • Integration test

Does this PR affect documentation (docs) or should it be mentioned in the release notes?

No

Does this PR affect tidb-ansible?

No

sticnarf added 3 commits Dec 10, 2019
wip
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf

This comment has been minimized.

Copy link
Contributor Author

sticnarf commented Dec 16, 2019

/release

@sticnarf sticnarf removed the S: DNM label Dec 17, 2019
@sticnarf sticnarf changed the title [DNM] cop: change handle_request interface to async cop: change handle_request interface to async Dec 17, 2019
@sticnarf sticnarf requested a review from breeswish Dec 17, 2019
Copy link
Member

breeswish left a comment

Nice clean up! Could you also change the streaming version to use async await as well? The rest LGTM.

src/coprocessor/endpoint.rs Outdated Show resolved Hide resolved
@sticnarf

This comment has been minimized.

Copy link
Contributor Author

sticnarf commented Dec 17, 2019

@breeswish Ok. I'll also do it for the streaming part.

sticnarf added 7 commits Dec 18, 2019
wip
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
…to cop-async-await
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf

This comment has been minimized.

Copy link
Contributor Author

sticnarf commented Dec 18, 2019

@breeswish I've changed the streaming part to async/await too. PTAL.

sticnarf added 2 commits Dec 18, 2019
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf sticnarf requested review from breeswish and AndreMouche Dec 18, 2019
Copy link
Member

AndreMouche left a comment

Rest LGTM

@@ -20,6 +20,23 @@ where
(callback, future)
}

pub fn paired_future_callback_new<T>() -> (

This comment has been minimized.

Copy link
@AndreMouche

AndreMouche Dec 18, 2019

Member

I think it is not a good name..

This comment has been minimized.

Copy link
@sticnarf

sticnarf Dec 18, 2019

Author Contributor

I change it to paired_std_future_callback

Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf sticnarf force-pushed the sticnarf:cop-async-await branch from 87ddbe2 to 547a0f9 Dec 18, 2019
@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented Dec 20, 2019

/bench +sysbench +tpch

Copy link
Member

breeswish left a comment

Cool

src/coprocessor/endpoint.rs Show resolved Hide resolved
Copy link
Member

AndreMouche left a comment

LGTM

@AndreMouche

This comment has been minimized.

Copy link
Member

AndreMouche commented Dec 20, 2019

/merge

@sre-bot sre-bot added the S: CanMerge label Dec 20, 2019
@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Dec 20, 2019

/run-all-tests

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Dec 20, 2019

@sticnarf merge failed.

@sticnarf

This comment has been minimized.

Copy link
Contributor Author

sticnarf commented Dec 20, 2019

/merge

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Dec 20, 2019

/run-all-tests

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented Dec 20, 2019

/run-all-tests

@breeswish breeswish merged commit a426b3b into tikv:master Dec 20, 2019
5 of 6 checks passed
5 of 6 checks passed
idc-jenkins-ci-tikv/integration-copr-test Jenkins job is running.
Details
DCO All commits are signed off!
Details
idc-jenkins-ci-tikv/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.