-
Notifications
You must be signed in to change notification settings - Fork 54
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
Set the max number of driver instances to CPU cores #25
Conversation
vmarkovtsev
commented
Jun 14, 2017
- Add the environment variable
41f4a82
to
5ed231b
Compare
pool.go
Outdated
return MovingAverage(10, MinMax(1, 10, AIMD(1, 0.5))) | ||
maxInstances := 0 | ||
// Try to read maxInstances from the environment variable | ||
maxInstancesEnv := os.Getenv("BBLFSH_SCALING_MAX_INSTANCES") |
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.
I think it'd be better to move this context dependant code outside and pass them as arguments explicitely
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.
I see that DefaultScalingPolicy()
is used in many places, including the tests. What shall I do with all of those?
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.
@abeaumont that can be done by not using DefaultScalingPolicy()
and instantiated it instead.
Nonetheless, this code should be in func init()
, initializing a global variable (e.g. DefaulMaxInstancesPerDriver
), and then using that variable here.
But you're right that at some point we should move this kind of configuration to server.go
and propagate it up to the cli
package. I think would leave that to a further PR though.
66086af
to
739d5d0
Compare
pool.go
Outdated
return MovingAverage(10, MinMax(1, 10, AIMD(1, 0.5))) | ||
maxInstances := 0 | ||
// Try to read maxInstances from the environment variable | ||
maxInstancesEnv := os.Getenv("BBLFSH_SCALING_MAX_INSTANCES") |
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.
@abeaumont that can be done by not using DefaultScalingPolicy()
and instantiated it instead.
Nonetheless, this code should be in func init()
, initializing a global variable (e.g. DefaulMaxInstancesPerDriver
), and then using that variable here.
But you're right that at some point we should move this kind of configuration to server.go
and propagate it up to the cli
package. I think would leave that to a further PR though.
Codecov Report
@@ Coverage Diff @@
## master #25 +/- ##
==========================================
- Coverage 61.03% 60.49% -0.54%
==========================================
Files 13 13
Lines 675 681 +6
==========================================
Hits 412 412
- Misses 225 229 +4
- Partials 38 40 +2
Continue to review full report at Codecov.
|
f9ec4bc
to
96f8939
Compare
@smola Did as you said, changed the corresponding test to pass. |