-
Notifications
You must be signed in to change notification settings - Fork 48
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
Refactor batch system #223
Conversation
Each peer can bind to a worker, and we can bind a peer to another worker if we need to. Currently, there is no scheduler to rebalance worker, simpler use the region id to bind a worker. |
tikv/raftstore/config.go
Outdated
@@ -193,7 +193,7 @@ func NewDefaultConfig() *Config { | |||
ApplyMaxBatchSize: 1024, | |||
ApplyPoolSize: 2, | |||
StoreMaxBatchSize: 1024, | |||
StorePoolSize: 2, | |||
StorePoolSize: 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change the default value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For benchmark comparison, forgot to change it back, I will make it configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
} | ||
|
||
func (pr *peerRouter) get(regionID uint64) *peerState { | ||
pr.mu.RLock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using RWMutex in this scenario may cause some performance issue? 🤔
We can do some benchmark later.
ref: golang/go#17973
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can shard the map based on the region ID for lower contention later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sync.Map
or some kinds of COW map may also good
The old batch system for multi-thread raft is too complex.
Replace it with a simpler multi-thread model.