The minimal region cache #291

ekexium · 2021-05-19T14:13:43Z

The PR adds a minimal region cache implementation. It's far away from maturity. It's intended to discuss how the structure should look.

Related issue: #299

Main changes:

Place the region cache as a component in PdRpcClient. PdRpcClient -> region cache -> retry client (the low level pd client)
Make single-region plan a special case of multi-region plan

Credit to #116, #121 and #139. Since there have been some refactoring after the previous work, I didn't continue development based on those commits.

Naive manual benchmark

Environment: single machine, 1 PD, 3 TiKV, default scheduler + shuffle-region + shuffle-leader
Workload: 6 threads, 512 workers, 50% single-key-read, 50% single-key-write

zipfian distribution, pessimistic txns

master: TPS: 1943.9. Latency: mean: 41ms, P99: 1018ms, maximum: 1815ms
This PR: TPS 2108.5. Latency: mean: 12ms, P99: 108ms, maximum: 1061ms

uniform distribution, optimistic txns

master: TPS: 4279.4, mean: 70ms, P99: 1048ms, maximum: 2601ms
This PR: TPS: 21129.8, mean: 22ms, P99: 44ms, maximum: 286ms

Todo

Remove read_through_cache parameters, if we can always know the invalid cache entry
Treat single-region as a special case of multi-region
Test
Save unnecessary PD requests if they are querying the same region
Finer grained error handling
Naming

Signed-off-by: ekexium <ekexium@gmail.com>

… rwlocks Signed-off-by: ekexium <ekexium@gmail.com>

Signed-off-by: ekexium <ekexium@gmail.com>

tests/common/mod.rs

src/region_cache.rs

src/request/plan.rs

tikv-client-store/src/errors.rs

tests/integration_tests.rs

Signed-off-by: ekexium <ekexium@gmail.com>

src/request/plan.rs

sticnarf · 2021-06-30T09:26:27Z

src/request/plan.rs

+                        Self::handle_region_error(pd_client.clone(), e, region_store).await?;
+                    // don't sleep if we have resolved the region error
+                    if !region_error_resolved {
+                        futures_timer::Delay::new(duration).await;


Not so related to the topic of the PR. As we assume the user has tokio runtime, the tokio timer may be preferred. (And we can remove the futures_timer dependency)

There is a wish that we can remove the dependency of tokio one day...

At first I refused to introduce the runtime too. But I must admit, with a runtime, it is both easy for us to design the API and code, and for the users to use...

The client inevitably needs some background work to do. Rust does not provide an async runtime interface, and it is not so suitable to start a runtime by the client. Relying on a specific runtime looks like the only solution in the near future :(

All right. We can do it in another PR. #246

src/request/plan.rs

sticnarf · 2021-06-30T11:12:41Z

src/request/plan.rs

+                    Ok(false)
+                }
+            }
+            // errors from PD requests, backoff and retry


Emm, I don't see why there can be PD errors.

I didn't considerably think whether it is possible. Just in case :)

The function name is handle_region_error, so it is unreasonable that non-region errors occur here. Maybe the definition of HasRegionError::region_error can be changed to just return an errorpb::Error).

But it indeed reminds me that the error handling for PD errors is coarse grained. Not a big problem for now as there is typically not much trouble PD can cause.

The function name is handle_region_error, so it is unreasonable that non-region errors occur here.

Right. I just made the change.

Maybe the definition of HasRegionError::region_error can be changed to just return an errorpb::Error)

Good idea

Cargo.toml

Signed-off-by: ekexium <ekexium@gmail.com>

sticnarf

I'm fine with the current code.

By the way, the backoff design is a bit from the client go. It is simpler because it always use the same backoff type (for different errors) and does not record the backoff history. These features are nice to have, but not an urgent task.

ekexium · 2021-07-01T09:17:07Z

By the way, the backoff design is a bit from the client go. It is simpler because it always use the same backoff type (for different errors) and does not record the backoff history. These features are nice to have, but not an urgent task.

Yep, there is an issue there #310

youjiali1995

The minimal region cache LGTM.

src/request/plan.rs

Signed-off-by: ekexium <ekexium@gmail.com>

sticnarf · 2021-07-21T07:07:55Z

Please fix the clippy warnings.

sticnarf

LGTM

ekexium added 2 commits May 18, 2021 22:58

feat: add region cache

ef1ff95

Signed-off-by: ekexium <ekexium@gmail.com>

feat: add store cache

1f38e61

Signed-off-by: ekexium <ekexium@gmail.com>

ekexium requested a review from andylokandy May 19, 2021 14:13

opt: reduce lock contention by replacing the large mutex with smaller…

07f0a47

… rwlocks Signed-off-by: ekexium <ekexium@gmail.com>

ekexium changed the title ~~The minimal region cache~~ [WIP] The minimal region cache May 19, 2021

ekexium added 4 commits May 20, 2021 00:19

fix: locking correctness in add_region

2fa6569

Signed-off-by: ekexium <ekexium@gmail.com>

refactor: use ver_id as keys of region cache

4bb31e8

Signed-off-by: ekexium <ekexium@gmail.com>

feat: merge retry_region and multi_region; only retry failed shards

8afaae3

Signed-off-by: ekexium <ekexium@gmail.com>

feat: parallelize multi-region requests

3a3b7c6

Signed-off-by: ekexium <ekexium@gmail.com>

ekexium force-pushed the region-cache branch from b4be6f2 to 3a3b7c6 Compare May 26, 2021 14:29

ekexium added 5 commits May 27, 2021 21:32

feat: hanlde region error based on error kinds

719e6f3

Signed-off-by: ekexium <ekexium@gmail.com>

Merge remote-tracking branch 'upstream/master' into region-cache

38ced7e

Signed-off-by: ekexium <ekexium@gmail.com>

refactor: remove all read_through_cache

ca41651

Signed-off-by: ekexium <ekexium@gmail.com>

fix: treat retry_single_region as a special case of retry_multi_region

204e271

Signed-off-by: ekexium <ekexium@gmail.com>

Merge remote-tracking branch 'upstream/master' into region-cache

c9f72a8

ekexium force-pushed the region-cache branch 3 times, most recently from 9b8ed23 to c16c6dd Compare June 21, 2021 09:00

opt: don't send multiple concurrent queries of same region id

75663eb

Signed-off-by: ekexium <ekexium@gmail.com>

ekexium force-pushed the region-cache branch 2 times, most recently from d24e490 to 2668d9f Compare June 21, 2021 12:52

test: enable txn_bank_transfer

0ac5807

Signed-off-by: ekexium <ekexium@gmail.com>

ekexium force-pushed the region-cache branch 2 times, most recently from 59eb893 to dbddc98 Compare June 21, 2021 14:05

lint: check with --all-features --all-targets

0e36188

Signed-off-by: ekexium <ekexium@gmail.com>

ekexium force-pushed the region-cache branch from dbddc98 to 0e36188 Compare June 21, 2021 14:11

ekexium added 3 commits June 21, 2021 23:12

increase region error backoff attempts

61c99ae

Signed-off-by: ekexium <ekexium@gmail.com>

test: add basic unit test

aad9b89

Signed-off-by: ekexium <ekexium@gmail.com>

fix: handle EpochNotMatch

2de9959

Signed-off-by: ekexium <ekexium@gmail.com>

ekexium force-pushed the region-cache branch from ab96bbf to 2de9959 Compare June 23, 2021 09:18

andylokandy reviewed Jun 24, 2021

View reviewed changes

tests/common/mod.rs Outdated Show resolved Hide resolved

src/region_cache.rs Outdated Show resolved Hide resolved

src/request/plan.rs Outdated Show resolved Hide resolved

tikv-client-store/src/errors.rs Outdated Show resolved Hide resolved

andylokandy reviewed Jun 24, 2021

View reviewed changes

tests/integration_tests.rs Outdated Show resolved Hide resolved

address comments

7c8cc69

Signed-off-by: ekexium <ekexium@gmail.com>

ekexium force-pushed the region-cache branch from d089fe4 to 7c8cc69 Compare June 24, 2021 11:43

fix: a stale usage of RegionError

47c9c61

Signed-off-by: ekexium <ekexium@gmail.com>

ekexium requested review from sticnarf and youjiali1995 June 25, 2021 02:55

sticnarf reviewed Jun 30, 2021

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

ekexium force-pushed the region-cache branch from a7584a4 to aa02214 Compare June 30, 2021 11:38

*: address comments

04d61b4

Signed-off-by: ekexium <ekexium@gmail.com>

ekexium force-pushed the region-cache branch from aa02214 to 04d61b4 Compare June 30, 2021 11:51

refactor: region_error() returns errorpb::Error

ff07c07

Signed-off-by: ekexium <ekexium@gmail.com>

sticnarf reviewed Jul 1, 2021

View reviewed changes

youjiali1995 reviewed Jul 7, 2021

View reviewed changes

src/request/plan.rs Show resolved Hide resolved

ekexium changed the title ~~[WIP] The minimal region cache~~ The minimal region cache Jul 21, 2021

ekexium added 2 commits July 21, 2021 14:53

Merge remote-tracking branch 'upstream/master' into region-cache

adc84d5

Signed-off-by: ekexium <ekexium@gmail.com>

style: fix clippy

fa76c85

Signed-off-by: ekexium <ekexium@gmail.com>

ekexium mentioned this pull request Jul 21, 2021

Add region cache #299

Open

4 tasks

sticnarf approved these changes Jul 21, 2021

View reviewed changes

sticnarf merged commit c14f23a into tikv:master Jul 21, 2021

ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The minimal region cache #291

The minimal region cache #291

ekexium commented May 19, 2021 •

edited

Loading

sticnarf Jun 30, 2021

ekexium Jun 30, 2021

sticnarf Jun 30, 2021 •

edited

Loading

sticnarf Jun 30, 2021

ekexium Jun 30, 2021

sticnarf Jun 30, 2021

ekexium Jun 30, 2021

sticnarf Jun 30, 2021

ekexium Jun 30, 2021

ekexium Jun 30, 2021

sticnarf left a comment

ekexium commented Jul 1, 2021

youjiali1995 left a comment

sticnarf commented Jul 21, 2021

sticnarf left a comment

The minimal region cache #291

The minimal region cache #291

Conversation

ekexium commented May 19, 2021 • edited Loading

Main changes:

Naive manual benchmark

Todo

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sticnarf Jun 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sticnarf left a comment

Choose a reason for hiding this comment

ekexium commented Jul 1, 2021

youjiali1995 left a comment

Choose a reason for hiding this comment

sticnarf commented Jul 21, 2021

sticnarf left a comment

Choose a reason for hiding this comment

ekexium commented May 19, 2021 •

edited

Loading

sticnarf Jun 30, 2021 •

edited

Loading