Improve lock mechanism in tso.KeyspaceGroupManager #6305

binshi-bing · 2023-04-11T21:27:18Z

What problem does this PR solve?

Issue Number: Ref #6232

What is changed and how does it work?

Use the RWMutex instead of individual atomic types to better protect the state of the keyspace group manager

Check List

Tests

Unit test

Release note

None.

…the state of the keyspace group manager Signed-off-by: Bin Shi <[email protected]>

ti-chi-bot · 2023-04-11T21:27:20Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JmPotato
rleungx

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Signed-off-by: Bin Shi <[email protected]>

pkg/tso/keyspace_group_manager.go

rleungx · 2023-04-12T02:19:25Z

pkg/tso/keyspace_group_manager.go

 		return
 	}

 	assignedToMe := kgm.isAssignedToMe(group)
 	if assignedToMe {
-		if kgm.ams[group.ID].Load() != nil {
+		if kgm.ams[group.ID] != nil {


Do we need a lock here?

No need.

There are three writers -- initialization (Load the initial keyspace group assignment from storage), KeyspaceGroupsMetaWatchLoop (the code shown above is in this single goroutine), and shutdown. They are serialized, i.e., there are no concurrent writers. Specifically, we start KeyspaceGroupsMetaWatchLoop after static initialization, and the system exits from KeyspaceGroupsMetaWatchLoop before write access in shutdown.

In summary, KeyspaceGroupsMetaWatchLoop can read without lock, but it must require lock for write so that keyspace group manager can handle concurrent reads when processing requests.

But I suggest adding one to prevent the potential problem, it doesn't cost too much I think.

It has cost. If I add a lock it needs to be a write lock and adding here will increase the length of the critical section.

The lock isn't re-entrant. If I'm right, with sync.RWMutex, I even can't require a read lock then upgrade it the write lock.

good suggestion, actually.

changed to (kgm.getAllocatorManager requires the read lock)

if oldAM, oldGroup := kgm.getAllocatorManager(group.ID); oldAM != nil { log.Info("keyspace group already initialized, so update meta only", zap.Uint32("keyspace-group-id", group.ID)) kgm.updateKeyspaceGroupMembership(oldGroup, group) return }

rleungx · 2023-04-12T02:20:04Z

pkg/tso/keyspace_group_manager.go

 }

 // deleteKeyspaceGroup deletes the given keyspace group.
 func (kgm *KeyspaceGroupManager) deleteKeyspaceGroup(groupID uint32) {
-	kg := kgm.kgs[groupID].Swap(nil)
+	kg := kgm.kgs[groupID]


good catch. You saved me. I remember I added the lock here, but somehow it's gone.

kgm.Lock() defer kgm.Unlock()

Confirm that no other places need lock

JmPotato

The rest LGTM.

pkg/tso/allocator_manager.go

pkg/tso/keyspace_group_manager.go

Signed-off-by: Bin Shi <[email protected]>

codecov · 2023-04-12T06:21:28Z

Codecov Report

Patch coverage: 94.28% and project coverage change: -0.03 ⚠️

Comparison is base (3e6ac89) 75.13% compared to head (24f3faa) 75.11%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6305      +/-   ##
==========================================
- Coverage   75.13%   75.11%   -0.03%     
==========================================
  Files         404      404              
  Lines       39686    39697      +11     
==========================================
- Hits        29819    29818       -1     
- Misses       7267     7270       +3     
- Partials     2600     2609       +9

Flag	Coverage Δ
unittests	`75.11% <94.28%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/tso/keyspace_group_manager.go	`82.95% <94.20%> (+3.01%)`	⬆️
pkg/tso/allocator_manager.go	`62.40% <100.00%> (-3.55%)`	⬇️

... and 32 files with indirect coverage changes

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

JmPotato · 2023-04-12T06:25:45Z

pkg/tso/keyspace_group_manager.go

@@ -415,6 +462,8 @@ func (kgm *KeyspaceGroupManager) watchKeyspaceGroupsMetaChange(revision int64) (
 				case clientv3.EventTypeDelete:
 					kgm.deleteKeyspaceGroup(id)
 				}
+
+				kgm.storageChangeEventsApplied.Add(1)


Could the counter overflow? Do you think we can only add it during the test? Maybe a failpoint could help.

I already changed before saw your comment. Basically, I required read lock in the test. Check the commit 5.

Signed-off-by: Bin Shi <[email protected]> Fix go fmt error Signed-off-by: Bin Shi <[email protected]>

rleungx

The rest LGTM

JmPotato · 2023-04-12T06:55:28Z

/merge

ti-chi-bot · 2023-04-12T06:55:30Z

@JmPotato: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

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 ti-community-infra/tichi repository.

ti-chi-bot · 2023-04-12T06:55:31Z

This pull request has been accepted and is ready to merge.

Commit hash: 24f3faa

ref #6232, ref #6305 follow-up #6305 Add read lock at one place for protection and better structure Signed-off-by: Bin Shi <[email protected]> Co-authored-by: Ti Chi Robot <[email protected]>

Use the RWMutex instead of individual atomic types to better protect …

6f30ebf

…the state of the keyspace group manager Signed-off-by: Bin Shi <[email protected]>

ti-chi-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Apr 11, 2023

ti-chi-bot requested review from nolouch and rleungx April 11, 2023 21:27

binshi-bing requested review from JmPotato and lhy1024 and removed request for nolouch April 11, 2023 21:27

binshi-bing mentioned this pull request Apr 11, 2023

Support multi-keyspace-group and sharding in tso grpc service #6232

Closed

13 tasks

renaming

612a93f

Signed-off-by: Bin Shi <[email protected]>

rleungx reviewed Apr 12, 2023

View reviewed changes

JmPotato reviewed Apr 12, 2023

View reviewed changes

pkg/tso/allocator_manager.go Outdated Show resolved Hide resolved

pkg/tso/keyspace_group_manager.go Outdated Show resolved Hide resolved

Handled feedback. Fixed bugs.

5bc37f9

Signed-off-by: Bin Shi <[email protected]>

binshi-bing force-pushed the improve-kgm-lock branch from 97c21e3 to 5bc37f9 Compare April 12, 2023 05:30

Add more comments at a critical place

93a5694

Signed-off-by: Bin Shi <[email protected]>

JmPotato reviewed Apr 12, 2023

View reviewed changes

binshi-bing force-pushed the improve-kgm-lock branch 2 times, most recently from 6ddd516 to 37ec9f9 Compare April 12, 2023 06:34

Fix test failure

24f3faa

Signed-off-by: Bin Shi <[email protected]> Fix go fmt error Signed-off-by: Bin Shi <[email protected]>

binshi-bing force-pushed the improve-kgm-lock branch from 37ec9f9 to 24f3faa Compare April 12, 2023 06:35

rleungx approved these changes Apr 12, 2023

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 12, 2023

JmPotato approved these changes Apr 12, 2023

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 12, 2023

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 12, 2023

ti-chi-bot merged commit f6a4090 into tikv:master Apr 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve lock mechanism in tso.KeyspaceGroupManager #6305

Improve lock mechanism in tso.KeyspaceGroupManager #6305

binshi-bing commented Apr 11, 2023

ti-chi-bot commented Apr 11, 2023 •

edited

Loading

rleungx Apr 12, 2023

binshi-bing Apr 12, 2023

rleungx Apr 12, 2023

binshi-bing Apr 12, 2023

binshi-bing Apr 12, 2023

binshi-bing Apr 12, 2023

rleungx Apr 12, 2023

binshi-bing Apr 12, 2023

binshi-bing Apr 12, 2023

binshi-bing Apr 12, 2023

JmPotato left a comment

codecov bot commented Apr 12, 2023 •

edited

Loading

JmPotato Apr 12, 2023

binshi-bing Apr 12, 2023

rleungx left a comment

JmPotato commented Apr 12, 2023

ti-chi-bot commented Apr 12, 2023

ti-chi-bot commented Apr 12, 2023

Improve lock mechanism in tso.KeyspaceGroupManager #6305

Improve lock mechanism in tso.KeyspaceGroupManager #6305

Conversation

binshi-bing commented Apr 11, 2023

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JmPotato left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 12, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleungx left a comment

Choose a reason for hiding this comment

JmPotato commented Apr 12, 2023

ti-chi-bot commented Apr 12, 2023

ti-chi-bot commented Apr 12, 2023

ti-chi-bot commented Apr 11, 2023 •

edited

Loading

codecov bot commented Apr 12, 2023 •

edited

Loading