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

tikvrpc: avoid data race on XxxRequest.Context #1242

Merged
merged 7 commits into from
Mar 22, 2024
Merged

Conversation

zyguan
Copy link
Contributor

@zyguan zyguan commented Mar 20, 2024

Try to fix pingcap/tidb#51921 . To reduce the memory allocation overhead, only do COW when req.rev > 0 .

Signed-off-by: zyguan <zhongyangguan@gmail.com>
@zyguan zyguan marked this pull request as ready for review March 20, 2024 11:55
tikvrpc/tikvrpc_test.go Show resolved Hide resolved
tikvrpc/gen.sh Show resolved Hide resolved
tikvrpc/tikvrpc.go Show resolved Hide resolved
Copy link
Contributor

@cfzjywxk cfzjywxk left a comment

Choose a reason for hiding this comment

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

Rest LGTM

Signed-off-by: zyguan <zhongyangguan@gmail.com>
tikvrpc/gen.sh Outdated Show resolved Hide resolved
tikvrpc/gen.sh Show resolved Hide resolved
@@ -118,6 +118,11 @@ const (
CmdEmpty CmdType = 3072 + iota
)

// CmdType aliases.
const (
CmdGetKeyTTL = CmdRawGetKeyTTL
Copy link
Contributor

Choose a reason for hiding this comment

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

😰

} else {
cmd := *req.${cmd}()
cmd.Context = ctx
req.Req = &cmd
Copy link
Contributor

Choose a reason for hiding this comment

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

May there still be data race on the reading/writing the pointer req.Req?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I only see the data race on the value refered by the pointer, AFAIK, there is no concurrent access to the pointer field itself.

Signed-off-by: zyguan <zhongyangguan@gmail.com>
Copy link
Contributor

@MyonKeminta MyonKeminta left a comment

Choose a reason for hiding this comment

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

The chagne LGTM. Needs to figure out what's wrong in the unit test..

@MyonKeminta
Copy link
Contributor

2024-03-21T04:59:41.5223148Z --- FAIL: TestReplicaReadAccessPathByGenError (330.55s)
2024-03-21T04:59:41.5223328Z     replica_selector_test.go:2676:
2024-03-21T04:59:41.5224007Z            Error Trace:    /home/runner/work/client-go/client-go/internal/locate/replica_selector_test.go:2676
2024-03-21T04:59:41.5224917Z                                                    /home/runner/work/client-go/client-go/internal/locate/replica_selector_test.go:2621
2024-03-21T04:59:41.5225810Z                                                    /home/runner/work/client-go/client-go/internal/locate/replica_selector_test.go:2538
2024-03-21T04:59:41.5226753Z                                                    /home/runner/work/client-go/client-go/internal/locate/replica_selector_test.go:2561
2024-03-21T04:59:41.5226932Z            Error:          Not equal:
2024-03-21T04:59:41.5227152Z                            expected: true
2024-03-21T04:59:41.5227481Z                            actual  : false
2024-03-21T04:59:41.5227765Z            Test:           TestReplicaReadAccessPathByGenError
2024-03-21T04:59:41.5227982Z            Messages:       enable_forwarding: false
2024-03-21T04:59:41.5228190Z                            version: v2
2024-03-21T04:59:41.5228343Z                            {
2024-03-21T04:59:41.5228538Z                                    req: Get
2024-03-21T04:59:41.5228780Z                                    read_type: leader
2024-03-21T04:59:41.5229087Z                                    stale_read: false
2024-03-21T04:59:41.5229306Z                                    timeout: 0s
2024-03-21T04:59:41.5229568Z                                    busy_threshold_ms: 0
2024-03-21T04:59:41.5229757Z                                    label:
2024-03-21T04:59:41.5230632Z                                    access_err: ReadIndexNotReadyErr, NotLeaderWithNewLeader3Err, DeadLineExceededErr, RegionNotFoundErr
2024-03-21T04:59:41.5232655Z                                    access_path: {addr: store1, replica-read: false, stale-read: false}, {addr: store1, replica-read: false, stale-read: false}, {addr: store3, replica-read: false, stale-read: false}, {addr: store2, replica-read: false, stale-read: false}
2024-03-21T04:59:41.5232862Z                                    resp_err:
2024-03-21T04:59:41.5233246Z                                    resp_region_err: region_not_found:<>
2024-03-21T04:59:41.5233473Z                                    backoff_cnt: 2
2024-03-21T04:59:41.5233893Z                                    backoff_detail: regionScheduling+1, tikvRPC+1
2024-03-21T04:59:41.5234173Z                                    region_is_valid: false
2024-03-21T04:59:41.5234323Z                            }
2024-03-21T04:59:41.5239573Z                            sender: {rpcError:context deadline exceeded,replicaSelector: replicaSelectorV2{replicaReadType: leader, attempts: 4, cacheRegionIsValid: false, replicaStatus: [peer: 4, store: 1, isEpochStale: false, attempts: 2, replica-epoch: 63022, store-epoch: 63022, store-state: resolved, store-liveness-state: reachable peer: 5, store: 2, isEpochStale: false, attempts: 1, replica-epoch: 29596, store-epoch: 29596, store-state: resolved, store-liveness-state: reachable peer: 6, store: 3, isEpochStale: true, attempts: 1, replica-epoch: 23167, store-epoch: 23168, store-state: needCheck, store-liveness-state: unreachable]}}

Does this have anything to do with this PR?

@cfzjywxk
Copy link
Contributor

2024-03-21T04:59:41.5223148Z --- FAIL: TestReplicaReadAccessPathByGenError (330.55s)
2024-03-21T04:59:41.5223328Z     replica_selector_test.go:2676:
2024-03-21T04:59:41.5224007Z            Error Trace:    /home/runner/work/client-go/client-go/internal/locate/replica_selector_test.go:2676
2024-03-21T04:59:41.5224917Z                                                    /home/runner/work/client-go/client-go/internal/locate/replica_selector_test.go:2621
2024-03-21T04:59:41.5225810Z                                                    /home/runner/work/client-go/client-go/internal/locate/replica_selector_test.go:2538
2024-03-21T04:59:41.5226753Z                                                    /home/runner/work/client-go/client-go/internal/locate/replica_selector_test.go:2561
2024-03-21T04:59:41.5226932Z            Error:          Not equal:
2024-03-21T04:59:41.5227152Z                            expected: true
2024-03-21T04:59:41.5227481Z                            actual  : false
2024-03-21T04:59:41.5227765Z            Test:           TestReplicaReadAccessPathByGenError
2024-03-21T04:59:41.5227982Z            Messages:       enable_forwarding: false
2024-03-21T04:59:41.5228190Z                            version: v2
2024-03-21T04:59:41.5228343Z                            {
2024-03-21T04:59:41.5228538Z                                    req: Get
2024-03-21T04:59:41.5228780Z                                    read_type: leader
2024-03-21T04:59:41.5229087Z                                    stale_read: false
2024-03-21T04:59:41.5229306Z                                    timeout: 0s
2024-03-21T04:59:41.5229568Z                                    busy_threshold_ms: 0
2024-03-21T04:59:41.5229757Z                                    label:
2024-03-21T04:59:41.5230632Z                                    access_err: ReadIndexNotReadyErr, NotLeaderWithNewLeader3Err, DeadLineExceededErr, RegionNotFoundErr
2024-03-21T04:59:41.5232655Z                                    access_path: {addr: store1, replica-read: false, stale-read: false}, {addr: store1, replica-read: false, stale-read: false}, {addr: store3, replica-read: false, stale-read: false}, {addr: store2, replica-read: false, stale-read: false}
2024-03-21T04:59:41.5232862Z                                    resp_err:
2024-03-21T04:59:41.5233246Z                                    resp_region_err: region_not_found:<>
2024-03-21T04:59:41.5233473Z                                    backoff_cnt: 2
2024-03-21T04:59:41.5233893Z                                    backoff_detail: regionScheduling+1, tikvRPC+1
2024-03-21T04:59:41.5234173Z                                    region_is_valid: false
2024-03-21T04:59:41.5234323Z                            }
2024-03-21T04:59:41.5239573Z                            sender: {rpcError:context deadline exceeded,replicaSelector: replicaSelectorV2{replicaReadType: leader, attempts: 4, cacheRegionIsValid: false, replicaStatus: [peer: 4, store: 1, isEpochStale: false, attempts: 2, replica-epoch: 63022, store-epoch: 63022, store-state: resolved, store-liveness-state: reachable peer: 5, store: 2, isEpochStale: false, attempts: 1, replica-epoch: 29596, store-epoch: 29596, store-state: resolved, store-liveness-state: reachable peer: 6, store: 3, isEpochStale: true, attempts: 1, replica-epoch: 23167, store-epoch: 23168, store-state: needCheck, store-liveness-state: unreachable]}}

Does this have anything to do with this PR?

@zyguan PTAL

Signed-off-by: zyguan <zhongyangguan@gmail.com>
@cfzjywxk cfzjywxk merged commit 05aaba6 into tikv:master Mar 22, 2024
10 checks passed
zyguan added a commit to zyguan/client-go that referenced this pull request May 22, 2024
* tikvrpc: avoid data race on `XxxRequest.Context`

Signed-off-by: zyguan <zhongyangguan@gmail.com>

* fix grammar of codegen comment

Signed-off-by: zyguan <zhongyangguan@gmail.com>

* address comments

Signed-off-by: zyguan <zhongyangguan@gmail.com>

* check diff of go generate

Signed-off-by: zyguan <zhongyangguan@gmail.com>

* fix a typo

Signed-off-by: zyguan <zhongyangguan@gmail.com>

---------

Signed-off-by: zyguan <zhongyangguan@gmail.com>
cfzjywxk pushed a commit that referenced this pull request May 28, 2024
* tikvrpc: avoid data race on `XxxRequest.Context`



* fix grammar of codegen comment



* address comments



* check diff of go generate



* fix a typo



---------

Signed-off-by: zyguan <zhongyangguan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants