Dynamic partitioning: sync backlog counts#10874
Conversation
carlydf
left a comment
carlydf
left a comment
There was a problem hiding this comment.
needs some tests, but mostly looks good!
| // always keep min of 1 even if we have no Ups/Downs | ||
| target = max(target, 1) |
There was a problem hiding this comment.
is the idea that min should now be 1 + backlogTarget, because backlogTarget is serving a separate "need" than addTarget?
(vs doing "min of 1" on totalTarget := addTarget + backlogTarget)
There was a problem hiding this comment.
If there are any ups/downs at all then we already have a min of 1. This just makes it true even if we have no ups/downs, which seems more consistent. No ups/downs is sort of unrealistic, except it's what the functional test happens to do, since it's focused on backlog.
But actually we do need to do it here and not on totalTarget, essentially because of the separate "need": if we do the min here, then with one partition > backlogbase, we get two total. And then the other one fills up and we get three, etc. If we do it on totalTarget:
Initially we get zero from addTarget, zero from backlogTarget -> one (by min on total), then it fills up and we get.. still zero from addTarget, one from backlogTarget -> one. And then even if it fills up more we can never increase.
So addTarget needs an independent min of one so that backlogTarget is always additive. And this change just ensures that happens without ups/downs.
| // TestOnTasksFloorsAddTargetAtOne verifies that with no rate windows configured | ||
| // the add-based target is floored at 1 (never 0, which would disable scaling). | ||
| // This baseline is what lets backlog-based scaling grow. | ||
| func TestOnTasksFloorsAddTargetAtOne(t *testing.T) { |
There was a problem hiding this comment.
Same topic as my comment above, I just want to make sure I understand the why of this.
It seems like if addTarget = 0, we would still compute a backlogTarget. So, what would be wrong with doing max(totalTarget, 1) after summing the two?
| await.RequireTrue(t, scalerBacklogEmpty(s, s.Tv(), 0, 1, 2, 3), 30*time.Second, time.Second) | ||
| } | ||
|
|
||
| // TODO: test disabling scaler |
There was a problem hiding this comment.
for this I meant something like: turn on the scaler, have it do some stuff, turn it off, then ensure that things go back to using dynamic config. as a functional test.
## What changed? Include backlog counts (quanitzed) in partition scale state and info, and use them in simple scaler. ## Why? Scaling based on large backlogs, and later load balancing based on backlog too. ## How did you test it? - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [x] added new unit test(s) - [x] added new functional test(s)
2 participants
What changed?
Include backlog counts (quanitzed) in partition scale state and info, and use
them in simple scaler.
Why?
Scaling based on large backlogs, and later load balancing based on backlog too.
How did you test it?