Skip to content

Conversation

@iosmanthus
Copy link
Member

@iosmanthus iosmanthus commented Jul 28, 2022

Signed-off-by: iosmanthus myosmanthustree@gmail.com

What problem does this PR solve?

Issue Number: close #639

Problem Description:

This pull request tries to reduce the lock granularity of ReionStoreClient and PDClient, we might block the requests while under a heavy workload.

What is changed and how does it work?

  1. Remove the synchronized keyword for RegionStoreBuilder.build, since the region cache's methods have already been synced.
  2. Remove the synchronized keyword in PDClient's update leader logic, since the leader will be eventually updated to the latest one by the update leader scheduler.

Check List for Tests

This PR has been tested by at least one of the following methods:

  • Unit test
  • Integration test

Related changes

  • Need to cherry-pick the release branch

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
@iosmanthus iosmanthus changed the title [WIP] reduce granularity 嗯 [WIP] reduce lock granularity of RegionManager and PDClient Jul 28, 2022
@codecov
Copy link

codecov bot commented Jul 28, 2022

Codecov Report

Merging #638 (54ecb43) into master (506d58f) will decrease coverage by 0.25%.
The diff coverage is 70.83%.

@@             Coverage Diff              @@
##             master     #638      +/-   ##
============================================
- Coverage     35.11%   34.86%   -0.26%     
+ Complexity     1442     1435       -7     
============================================
  Files           278      278              
  Lines         17362    17377      +15     
  Branches       1974     1974              
============================================
- Hits           6097     6058      -39     
- Misses        10651    10712      +61     
+ Partials        614      607       -7     
Impacted Files Coverage Δ
...ain/java/org/tikv/common/region/RegionManager.java 74.00% <0.00%> (+2.00%) ⬆️
...java/org/tikv/common/region/RegionStoreClient.java 52.49% <ø> (ø)
...java/org/tikv/common/operation/PDErrorHandler.java 40.00% <50.00%> (+1.29%) ⬆️
src/main/java/org/tikv/common/PDClient.java 58.73% <80.00%> (-6.02%) ⬇️
.../main/java/org/tikv/common/AbstractGRPCClient.java 43.42% <100.00%> (-5.93%) ⬇️
...rc/main/java/io/grpc/netty/NettyClientHandler.java 56.03% <0.00%> (-7.33%) ⬇️
...va/org/tikv/common/region/StoreHealthyChecker.java 72.15% <0.00%> (-1.27%) ⬇️
...n/java/org/tikv/common/util/ConcreteBackOffer.java 83.78% <0.00%> (-0.91%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 506d58f...54ecb43. Read the comment docs.

@iosmanthus iosmanthus changed the title [WIP] reduce lock granularity of RegionManager and PDClient [close #639] reduce lock granularity of RegionManager and PDClient Jul 29, 2022
@iosmanthus iosmanthus changed the title [close #639] reduce lock granularity of RegionManager and PDClient [close #639] reduce lock granularity of RegionStoreClient and PDClient Jul 29, 2022
@iosmanthus iosmanthus marked this pull request as ready for review July 29, 2022 03:30
if (pdClientWrapper != null) {
if (leaderUrlStr.equals(pdClientWrapper.getLeaderInfo())) {
@VisibleForTesting
public boolean trySwitchLeader(String leaderUrlStr) {

Choose a reason for hiding this comment

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

This method is not Thread-safe, since the value of pdClientWrapper can be changed during statements

// If we have not used follower forward, try the first follower.
boolean hasReachNextMember =
pdClientWrapper != null && pdClientWrapper.getStoreAddress().equals(leaderUrlStr);
pdClientWrapper.get() != null

Choose a reason for hiding this comment

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

The same as above, loss of atomicity

String followerUrlStr = member.getClientUrlsList().get(0);
followerUrlStr = uriToAddr(addrToUri(followerUrlStr));
if (pdClientWrapper != null && pdClientWrapper.getStoreAddress().equals(followerUrlStr)) {
if (pdClientWrapper.get() != null

Choose a reason for hiding this comment

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

The same as above, loss of atomicity

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
@iosmanthus iosmanthus force-pushed the fix-pd-requests-unlimit-timeout branch from 88be84c to 743415b Compare July 29, 2022 16:57
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
@iosmanthus iosmanthus force-pushed the fix-pd-requests-unlimit-timeout branch from 08de89c to 5c86cbd Compare July 29, 2022 19:26
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
@iosmanthus iosmanthus force-pushed the fix-pd-requests-unlimit-timeout branch from 5c86cbd to d2bf550 Compare July 29, 2022 19:55
…ests-unlimit-timeout

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
@iosmanthus iosmanthus merged commit 1b5edcd into tikv:master Jul 31, 2022
ti-srebot pushed a commit to ti-srebot/client-java that referenced this pull request Jul 31, 2022
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Collaborator

cherry pick to release-3.3 in PR #642

iosmanthus added a commit that referenced this pull request Jul 31, 2022
#638) (#642)

Co-authored-by: iosmanthus <dengliming@pingcap.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PDClient and RegionStoreClient are slow down massively in heavy workload.

4 participants