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

[to #656] Fix scan with lock #670

Merged
merged 12 commits into from
Dec 29, 2022
Merged

Conversation

pingyu
Copy link
Contributor

@pingyu pingyu commented Nov 20, 2022

What problem does this PR solve?

Issue Number: close #656

Problem Description: Catalog get table API return null sometimes

What is changed and how does it work?

Add resolve locks to scan implementation.

See the issue analysis here.

Code changes

Check List for Tests

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

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
while (true) {
    TiConfiguration conf = TiConfiguration.createDefault("172.16.5.32:44379");
    try (TiSession session = TiSession.create(conf)) {
      Catalog catalog = session.getCatalog();
      TiDBInfo db = catalog.getDatabase("sbtest");
      Objects.requireNonNull(db, "db is null");
    }
}

Side effects

  • Increased code complexity, WHY: Add resolve locks for scan

Related changes

  • Need to cherry-pick to the release branch

Signed-off-by: Ping Yu <[email protected]>
Signed-off-by: Ping Yu <[email protected]>
@codecov
Copy link

codecov bot commented Nov 20, 2022

Codecov Report

Base: 36.81% // Head: 37.32% // Increases project coverage by +0.51% 🎉

Coverage data is based on head (31300af) compared to base (1f096b5).
Patch coverage: 94.11% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #670      +/-   ##
============================================
+ Coverage     36.81%   37.32%   +0.51%     
- Complexity     1536     1572      +36     
============================================
  Files           278      278              
  Lines         17386    17386              
  Branches       1976     1978       +2     
============================================
+ Hits           6400     6489      +89     
+ Misses        10335    10241      -94     
- Partials        651      656       +5     
Impacted Files Coverage Δ
...java/org/tikv/common/region/RegionStoreClient.java 57.03% <94.11%> (+2.05%) ⬆️
.../org/tikv/common/apiversion/RequestKeyV1Codec.java 91.66% <0.00%> (-8.34%) ⬇️
src/main/java/io/grpc/netty/WriteQueue.java 74.43% <0.00%> (-2.26%) ⬇️
...rc/main/java/io/grpc/netty/NettyClientHandler.java 57.11% <0.00%> (+0.21%) ⬆️
src/main/java/io/grpc/internal/ClientCallImpl.java 57.07% <0.00%> (+0.24%) ⬆️
.../codec/http2/DefaultHttp2RemoteFlowController.java 59.69% <0.00%> (+0.38%) ⬆️
...n/java/org/tikv/common/util/ConcreteBackOffer.java 85.58% <0.00%> (+1.80%) ⬆️
src/main/java/org/tikv/txn/TxnStatus.java 26.66% <0.00%> (+3.33%) ⬆️
src/main/java/org/tikv/common/region/TiRegion.java 76.92% <0.00%> (+3.41%) ⬆️
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@shiyuhang0
Copy link
Collaborator

shiyuhang0 commented Dec 20, 2022

pingcap/tispark#2089
pingcap/tispark#2106
this two pr seems to related to this issue

Signed-off-by: Ping Yu <[email protected]>
@pingyu pingyu changed the title [to #656] Draft: Fix scan with lock [to #656] Fix scan with lock Dec 25, 2022
Signed-off-by: Ping Yu <[email protected]>
}
if (!locks.isEmpty()) {
ResolveLockResult resolveLockResult =
lockResolverClient.resolveLocks(backOffer, version, locks, forWrite);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why ?

It seems that resolve multiple locks at a time should be more efficient. For example, we get the maximum TTL here and just wait for the longest one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok. LGTM

@ti-srebot
Copy link
Collaborator

@shiyuhang0, Thanks for your review. The bot only counts LGTMs from Reviewers and higher roles, but you're still welcome to leave your comments. You are not a reviewer or committer or co-leader or leader.

@zhangyangyu zhangyangyu merged commit a459a6e into tikv:master Dec 29, 2022
ti-srebot pushed a commit to ti-srebot/client-java that referenced this pull request Dec 29, 2022
@ti-srebot
Copy link
Collaborator

cherry pick to release-3.3 in PR #697

zhangyangyu pushed a commit that referenced this pull request Dec 29, 2022
Signed-off-by: ti-srebot <[email protected]>

Signed-off-by: ti-srebot <[email protected]>
Co-authored-by: Ping Yu <[email protected]>
shiyuhang0 pushed a commit to shiyuhang0/client-java that referenced this pull request Dec 30, 2022
iosmanthus pushed a commit to iosmanthus/client-java that referenced this pull request Dec 30, 2022
shiyuhang0 pushed a commit to shiyuhang0/client-java that referenced this pull request Dec 30, 2022
Signed-off-by: ti-srebot <[email protected]>

Signed-off-by: ti-srebot <[email protected]>
Co-authored-by: Ping Yu <[email protected]>
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.

Catalog get table API return null sometimes
4 participants