executor: global kill 32bits (local connID part) #25385
Conversation
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
ti-chi-bot
added
the
size/XXL
|
/cc @SunRunAway @breeswish |
github-actions
bot
added
sig/execution
|
@pingyu I'll take a look by the end of this weekend. |
util/global_conn_id.go
Outdated
| } | ||
|
|
||
| // LocalConnIDAllocator32 is local connID allocator for 32bits global connection ID. | ||
| type LocalConnIDAllocator32 struct { |
There was a problem hiding this comment.
LocalPoolIDAllocator? (which is an ID allocator allocates ID from a local pool and returns the ID to the local pool when deallocates)
There was a problem hiding this comment.
Rename to LockFreeCircularPool. Please see this comment.
util/global_conn_id.go
Outdated
| return localConnID | ||
| } | ||
| } | ||
| panic(fmt.Sprintf("Failed to allocate 64bits local connID after retry %v times. Should never happen", LocalConnIDAllocator64RetryCount)) |
There was a problem hiding this comment.
It might not be never happen (from the perspective of the allocator itself, since a caller can set a relatively small ID range). How about throwing errors and let caller to decide what to do when allocate failed?
util/global_conn_id.go
Outdated
| ) | ||
|
|
||
| // SimpleConnIDAllocator is a simple auto-increment allocator. | ||
| type SimpleConnIDAllocator struct { |
There was a problem hiding this comment.
LocalAutoIncIDAllocator? (which is an ID allocator that simply do auto-increment to allocate ID and will never fail. Wrapping will happen. After that, the uniqueness of the ID will no longer be ensured.)
There was a problem hiding this comment.
I keep this SimpleConnIDAllocator. Please see this comment.
util/global_conn_id.go
Outdated
|
|
||
| // Init initiates LocalConnIDAllocator64 | ||
| func (a *LocalConnIDAllocator64) Init(existedChecker connectionIDExistCheckerFn) { | ||
| a.existedChecker = existedChecker |
There was a problem hiding this comment.
How about maintaining the exist checker inside the allocator, considering that it provides alloc and dealloc so that it can fully know whether ID exists?
util/global_conn_id.go
Outdated
| } | ||
|
|
||
| // LocalConnIDAllocator64 is local connID allocator for 64bits global connection ID. | ||
| type LocalConnIDAllocator64 struct { |
There was a problem hiding this comment.
LocalRandomIDAllocator? (which is an ID allocator that allocates ID using random probes in the ID range, fails after several attempts)
There was a problem hiding this comment.
Rename to AutoIncPool, as the implementation is actually auto-increment. Please see this comment.
|
Be inspired by @breeswish , I separate objects into two roles. One is
Hope the new class design would be a better encapsulation. P.S. I place a class diagram in source file: |
|
/run-check_dev_2 |
ti-chi-bot
added
the
needs-rebase
pingyu
force-pushed
the
global_kill_32_local-conn-id
branch
from
131d9aa
to
b3f930b
Compare
ti-chi-bot
bot
added
approved
needs-rebase
Signed-off-by: Ping Yu <yuping@pingcap.com>
pingyu
force-pushed
the
global_kill_32_local-conn-id
branch
from
ed57ab6
to
88d0cdb
Compare
ti-chi-bot
bot
removed
the
needs-rebase
| globalConnID := GCID{ | ||
| ServerID: serverID, | ||
| LocalConnID: (1 << LocalConnIDBits64) - 1 - reservedNo, | ||
| Is64bits: true, |
There was a problem hiding this comment.
We always use 64bits for the reserved conn ID. Will that bring any problem?
There was a problem hiding this comment.
No.
A 64 bits global connection ID with server ID less than MaxServerID32 (2^11 - 1) is still valid, and globally unique.
This scenario will also happen after upgrade from 32 bits to 64 bits when the 20 bits local ID pool has been used up, while server ID stay unchanged.
|
@xuyifangreeneyes: adding LGTM is restricted to approvers and reviewers in OWNERS files. In response to this: Instructions 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/test-infra repository. |
|
@xuyifangreeneyes: adding LGTM is restricted to approvers and reviewers in OWNERS files. In response to this: Instructions 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/test-infra repository. |
|
/lgtm |
|
@xuyifangreeneyes: adding LGTM is restricted to approvers and reviewers in OWNERS files. In response to this:
Instructions 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/test-infra repository. |
|
/approve |
|
/lgtm |
|
/approve |
|
/merge |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: breezewish, xuyifangreeneyes The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
What problem does this PR solve?
Issue Number: ref #8854
(32bits local connection IDs allocation part ONLY. Server id part will be in the next PR.)
Problem Summary:
Support CTRL-C or kill to kill a connection/query by implementing global connection IDs.
What is changed and how it works?
What's Changed:
ConnectionIDAllocator) from originalGlobalConnID, to improve codes organization.How it Works:
Related changes
Check List
Tests
Verify correctness of lock-free ring buffer by Go's race detector and long-time pressure test.
Integration test
By tests/globalkilltest
Benchmark
Side effects
Release note