Dynamic replication batch size #4301
Conversation
Pull Request Test Coverage Report for Build 2a76fb8d-1110-483a-acad-b0b1184ceb47
💛 - Coveralls |
| @@ -52,6 +53,8 @@ var ( | |||
| errUnknownQueueTask = errors.New("unknown task type") | |||
| errUnknownReplicationTask = errors.New("unknown replication task") | |||
| defaultHistoryPageSize = 1000 | |||
| minReadTaskSize = 20 | |||
| maxReplicationLatency = int64(40) | |||
There was a problem hiding this comment.
I suggest adding time unit suffix (I guess Seconds) or even better use time.Duration instead. As of now it is not clear what 40 means.
| t.taskLock.Lock() | ||
| taskLatency := int64(time.Now().Sub(t.lastTaskCreationTime) / time.Second) | ||
| t.taskLock.Unlock() |
There was a problem hiding this comment.
Trying to understand how useful this lock is: it feels like having and not having the lock here are the same to me. Could you you explain a bit what it is achieving here?
Are the time assignments in go not atomic that if we try to read its value while assigning somewhere else we get a crash?
There was a problem hiding this comment.
Most likely it will not be used. I added it to prevent concurrent calls to the method. Updated it with atomic.Value
…into dynamic-batch-size
…into dynamic-batch-size
common/dynamicconfig/constants.go
Outdated
| // ReplicatorUpperLatency indicates the max allowed replication latency between clusters | ||
| // KeyName: history.replicatorUpperLatencyInSeconds | ||
| // Value type: Duration | ||
| // Default value: 40 |
| rateLimiter: rateLimiter, | ||
| retryPolicy: retryPolicy, | ||
| lastTaskCreationTime: atomic.Value{}, | ||
| maxAllowedLatencyFn: config.ReplicatorUpperLatencyInSeconds, |
There was a problem hiding this comment.
config.ReplicatorUpperLatencyInSeconds is no longer defined I think.
| if t.lastTaskCreationTime.Load() == nil { | ||
| return defaultBatchSize | ||
| } | ||
| taskLatency := now.Sub(t.lastTaskCreationTime.Load().(time.Time)) / time.Second |
There was a problem hiding this comment.
We don't need to / time.Second here I think.
What changed?
Dynamic replication batch size
Why?
The replication batch size should be calculated based on the backlog in a particular shard. This could help with hot shards
How did you test it?
Unit tests
TODO: bench tests
Potential risks
Release notes
Documentation Changes