Skip to content
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

HTTP dynamic worker scaling is broken in 1.0 #735

Closed
Syraxius opened this issue Feb 19, 2020 · 1 comment · Fixed by #833
Closed

HTTP dynamic worker scaling is broken in 1.0 #735

Syraxius opened this issue Feb 19, 2020 · 1 comment · Fixed by #833
Labels
bug Something isn't working Priority: Medium

Comments

@Syraxius
Copy link

Syraxius commented Feb 19, 2020

Hi!

I'd like to bring your attention to the following code section.

	if o.config.workersMax != o.config.workersMin {
		workersCount := int(atomic.LoadInt64(&o.activeWorkers))

		if len(o.queue) > workersCount {
			extraWorkersReq := len(o.queue) - workersCount + 1
			maxWorkersAvailable := o.config.workersMax - workersCount
			if extraWorkersReq > maxWorkersAvailable {
				extraWorkersReq = maxWorkersAvailable
			}
			if extraWorkersReq > 0 {
				o.needWorker <- extraWorkersReq
			}
		}
	}

If not specified, the default values for the worker parameters are as follows:

At this point, since they're both the same value, then the o.config.workersMax != o.config.workersMin condition will evaluate to false and dynamic scaling is disabled.

Even if we assume the suggested defaults of workersMax = 0, workersMin = 1, we would enter this section but maxWorkersAvailable will always be negative, causing extraWorkersReq to be negative and o.needWorker will never be increased.

Instead, I would suggest this fix:

	if o.config.workersMax != o.config.workersMin || o.config.workersMax == 0 {
		workersCount := int(atomic.LoadInt64(&o.activeWorkers))

		if len(o.queue) > workersCount {
			extraWorkersReq := len(o.queue) - workersCount + 1
			maxWorkersAvailable := o.config.workersMax - workersCount
			if extraWorkersReq > maxWorkersAvailable && o.config.workersMax != 0 {
				extraWorkersReq = maxWorkersAvailable
			}
			if extraWorkersReq > 0 {
				o.needWorker <- extraWorkersReq
			}
		}
	}

Let me know if you're fine with this and I'll submit a PR.

@urbanishimwe
Copy link
Collaborator

@Syraxius we're okay with your help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Priority: Medium
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants