Spike: Configurable concurrency #52
Conversation
alpe
force-pushed
the
12_configurable_concurrency
branch
from
ea7eb45
to
4b8f8ca
Compare
| concurrencyMtx sync.RWMutex | ||
| // concurrency is the max number of in progress items. |
There was a problem hiding this comment.
What is the reason for having both concurrency and concurrencyPerReplica?
There was a problem hiding this comment.
concurrency seems store total queue size whereas concurrencyPerReplica is the per replica.
So if there are 3 replicas and concurrency per replica is 100 then concurrency would be 300.
Maybe we should rename concurrency to size OR maxConcurrentRequests?
There was a problem hiding this comment.
@samos123 is correct. Let's have better names. I have updated the doc already:
// // concurrency is the max number of in progress items on all replicas. Same as `number_replicas * concurrencyPerReplica`
// concurrencyPerReplica is the max number of in progress items that a single replica instance can handle.
| concurrencyMtx sync.RWMutex | ||
| // concurrency is the max number of in progress items. |
There was a problem hiding this comment.
concurrency seems store total queue size whereas concurrencyPerReplica is the per replica.
So if there are 3 replicas and concurrency per replica is 100 then concurrency would be 300.
Maybe we should rename concurrency to size OR maxConcurrentRequests?
| return | ||
| } | ||
|
|
||
| replicas := q.concurrency / int(q.concurrencyPerReplica) |
There was a problem hiding this comment.
Shouldn't we get number of replicas from the system? The number of replicas fluctuates frequently and I think replicas counted this way would often not match the replicas in the actual system.
Maybe it should take a 2nd parameter: replicas int, because the deploy manager has the actual current replicas that kubernetes reports.
There was a problem hiding this comment.
Two different processes are involved that can change the max of total concurrent in flight requests. The endpoint and the deployment reconciler. Both are used in the autoscaler but are otherwise independent. The state shared was in the queue type.
By calculating the new total by deriving the (best known) replica count, each process uses the data they own.
But we can also store the number of replicas for a deployment and not do the math in the queue type. This would be a cleaner separation.
| // SetCurrencyPerReplica updates the concurrency value per replica for a specific deployment. | ||
| // If the provided value is 0, it sets the concurrency value to the default value. | ||
| // This function updates the concurrency value directly on the queue associated with the deployment. | ||
| func (r *Manager) SetCurrencyPerReplica(deploymentName string, newPerReplica uint32) { |
There was a problem hiding this comment.
typo? should this be SetConcurrencyPerReplica instead of SetCurrencyPerReplica?
| m.queues[deploymentName] = q | ||
| go q.Start() | ||
| } | ||
| return q | ||
| } | ||
|
|
||
| func (r *Manager) GetCurrencyPerReplica(deployment string) uint32 { |
There was a problem hiding this comment.
typo? should this be GetConcurrencyPerReplica instead of GetCurrencyPerReplica?
馃毀 Spike - early version for feedback
Resolves #12