-
Notifications
You must be signed in to change notification settings - Fork 12
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
Supporting batching resources scaling #32
Supporting batching resources scaling #32
Conversation
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.
lgtm
minor comments regarding loggings, try make it more consistent
@@ -67,7 +67,7 @@ func Run(kubeconfigPath string, | |||
} | |||
|
|||
func createDLX(resourceScaler scaler_types.ResourceScaler, options scaler_types.DLXOptions) (*dlx.DLX, error) { | |||
rootLogger, err := nucliozap.NewNuclioZap("dlx", "console", os.Stdout, os.Stderr, nucliozap.DebugLevel) | |||
rootLogger, err := nucliozap.NewNuclioZap("scaler", "console", os.Stdout, os.Stderr, nucliozap.DebugLevel) |
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.
why?
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.
Talked offline
pkg/dlx/dlx.go
Outdated
resourceScaler scaler_types.ResourceScaler, | ||
options scaler_types.DLXOptions) (*DLX, error) { | ||
resourceStarter, err := NewResourceStarter(logger, resourceScaler, options.Namespace, options.ResourceReadinessTimeout) | ||
childLogger := parentLogger.GetChild("autoscaler") | ||
childLogger.InfoWith("Creating DLX", |
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.
one line
pkg/dlx/dlx.go
Outdated
resourceScaler scaler_types.ResourceScaler, | ||
options scaler_types.DLXOptions) (*DLX, error) { | ||
resourceStarter, err := NewResourceStarter(logger, resourceScaler, options.Namespace, options.ResourceReadinessTimeout) | ||
childLogger := parentLogger.GetChild("autoscaler") |
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.
logger name should be dlx
pkg/dlx/dlx.go
Outdated
listenAddress: options.ListenAddress, | ||
handler: handler, | ||
}, nil | ||
} | ||
|
||
func (d *DLX) Start() error { | ||
d.logger.InfoWith("Starting", | ||
d.logger.DebugWith("Starting", |
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.
one line
In each cycle of the autoscaler it will first iterate all resources and check which should be scaled to zero, then will scale to zero all of them together. (before this PR the behavior was to trigger scale to zero on every resource separately)
Added some logs improvements for better readability