server/lock_manager: Support replacing lock by key to reduce RPC overhead of update_wait_for when the waiting queue is very long#17451
Closed
MyonKeminta wants to merge 16 commits intotikv:masterfrom
Closed
Conversation
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
…correct Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
2f277c0 to
cf8d0f5
Compare
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Contributor
Author
|
/release |
Contributor
cfzjywxk
reviewed
Sep 3, 2024
cfzjywxk
reviewed
Sep 3, 2024
| 500, | ||
| ); | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
Do we have a upgrading test case to verify the behaviours are expected when upgrading tikv-servers?
Contributor
Author
There was a problem hiding this comment.
It was now manually done.
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Contributor
Author
|
/release |
ekexium
reviewed
Sep 3, 2024
Contributor
ekexium
left a comment
There was a problem hiding this comment.
The PR itself LGTM.
Have we conducted any throughput benchmarks comparing this implementation to the original one? Additionally, micro-benchmarks could help us determine the component's maximum performance capacity.
Contributor
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
crazycs520
reviewed
Sep 13, 2024
| detect_table.clean_up(txn.into()); | ||
| None | ||
| } | ||
| (_, true) => { |
Contributor
There was a problem hiding this comment.
This match is a bit weird, how about:
match req.get_tp(){
DeadlockRequestType::CleanUpWaitFor => {
if req.has_replace_locks_by_keys() {
// replace_locks_by_keys logic
}else{
// old logic
}
}
}
Contributor
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
overvenus
reviewed
Sep 27, 2024
Comment on lines
585
to
+586
| self.wait_for_map.clear(); | ||
| self.key_reverse_map.clear(); |
Member
There was a problem hiding this comment.
Forget to shrink? An empty hashmap may still occupy memory.
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Contributor
Author
|
Benchmarks cargo bench -p tests --bench deadlock_detectorThere will be obvious regression in the deadlock detector. |
9 tasks
9 tasks
MyonKeminta
added a commit
to MyonKeminta/tikv
that referenced
this pull request
Nov 20, 2024
…sts (tikv#17500) close tikv#17394 lock_manager: Skip updating lock wait info for non-fair-locking requests This is a simpler and lower-risky fix of the OOM issue tikv#17394 for released branches, as an alternative solution to tikv#17451 . In this way, for acquire_pessimistic_lock requests without enabling fair locking, the behavior of update_wait_for will be a noop. So that if fair locking is globally disabled, the behavior will be equivalent to versions before 7.0. Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
9 tasks
ti-chi-bot bot
pushed a commit
that referenced
this pull request
Nov 20, 2024
…sts (#17500) (#17870) close #17394 lock_manager: Skip updating lock wait info for non-fair-locking requests This is a simpler and lower-risky fix of the OOM issue #17394 for released branches, as an alternative solution to #17451 . In this way, for acquire_pessimistic_lock requests without enabling fair locking, the behavior of update_wait_for will be a noop. So that if fair locking is globally disabled, the behavior will be equivalent to versions before 7.0. Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
ti-chi-bot bot
pushed a commit
that referenced
this pull request
Nov 20, 2024
…sts (#17500) (#17869) close #17394 lock_manager: Skip updating lock wait info for non-fair-locking requests This is a simpler and lower-risky fix of the OOM issue #17394 for released branches, as an alternative solution to #17451 . In this way, for acquire_pessimistic_lock requests without enabling fair locking, the behavior of update_wait_for will be a noop. So that if fair locking is globally disabled, the behavior will be equivalent to versions before 7.0. Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> Co-authored-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Contributor
|
Is this abandoned? |
Contributor
Author
Currently no plan to continue this as its overhead is large and we've find it hard to determine if it's acceptable. |
Contributor
|
@MyonKeminta: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is changed and how it works?
Issue Number: Close #17394
What's Changed:
Related changes
Check List
Tests
Side effects
Release note