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

[close #433] fix calling getStoreById without backoffer #434

Merged

Conversation

marsishandsome
Copy link
Collaborator

@marsishandsome marsishandsome commented Dec 24, 2021

close #433

Timeout in seekLeaderStore is not limited.

We should use backoffer for all the gRPC calls.

@marsishandsome
Copy link
Collaborator Author

/run-all-tests

Copy link
Collaborator

@birdstorm birdstorm left a comment

Choose a reason for hiding this comment

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

LGTM

@marsishandsome marsishandsome force-pushed the feature/fix-seekLeaderStore-backoff-master branch from 67e5ff1 to b757b2b Compare December 24, 2021 03:10
@marsishandsome
Copy link
Collaborator Author

/run-all-tests

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

please add some tests.

src/main/java/org/tikv/common/TiSession.java Outdated Show resolved Hide resolved
src/main/java/org/tikv/common/TiSession.java Outdated Show resolved Hide resolved
src/main/java/org/tikv/common/TiSession.java Outdated Show resolved Hide resolved
@@ -185,6 +184,7 @@ public boolean canRetryAfterSleep(BackOffFunction.BackOffFuncType funcType, long
}
}

SlowLogSpan slowLogSpan = getSlowLog().start("backoff " + funcType.name());
Copy link
Member

Choose a reason for hiding this comment

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

why move it here? should BACKOFF_DURATION also be moved?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there is a return in line 184, which will cause "end":"N/A",.
it's hard to understand it.

Copy link
Member

Choose a reason for hiding this comment

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

got. then why not move BACKOFF_DURATION to here as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good idea

Copy link
Member

Choose a reason for hiding this comment

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

Moving these lines to here might cause we can't see the backoff in slow log.

Copy link
Member

Choose a reason for hiding this comment

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

We may end this span while returning though we will see this backoff end shortly but this span is still there.

Signed-off-by: marsishandsome <marsishandsome@gmail.com>
@marsishandsome marsishandsome force-pushed the feature/fix-seekLeaderStore-backoff-master branch from b757b2b to ab8cf80 Compare December 24, 2021 05:36
@marsishandsome
Copy link
Collaborator Author

/run-all-tests

@marsishandsome marsishandsome force-pushed the feature/fix-seekLeaderStore-backoff-master branch from ab8cf80 to 5afd1b9 Compare December 24, 2021 05:38
Signed-off-by: marsishandsome <marsishandsome@gmail.com>
@marsishandsome marsishandsome force-pushed the feature/fix-seekLeaderStore-backoff-master branch from 5afd1b9 to 1933ca1 Compare December 24, 2021 05:39
@marsishandsome
Copy link
Collaborator Author

/run-all-tests

Signed-off-by: marsishandsome <marsishandsome@gmail.com>
@marsishandsome
Copy link
Collaborator Author

/run-all-tests

1 similar comment
@marsishandsome
Copy link
Collaborator Author

/run-all-tests

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason merged commit 86606df into tikv:master Dec 24, 2021
ti-srebot pushed a commit to ti-srebot/client-java that referenced this pull request Dec 24, 2021
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Collaborator

cherry pick to release-3.1 in PR #440

@iosmanthus
Copy link
Member

Rest LGTM

marsishandsome added a commit to ti-srebot/client-java that referenced this pull request Dec 24, 2021
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
Signed-off-by: marsishandsome <marsishandsome@gmail.com>
marsishandsome added a commit that referenced this pull request Dec 24, 2021
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
Signed-off-by: marsishandsome <marsishandsome@gmail.com>

Co-authored-by: Liangliang Gu <marsishandsome@gmail.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.

Timeout in seekLeaderStore is not limited
5 participants