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

[Autoscaling] Add setting to customize autoscaling API polling frequency #4264

Merged
merged 2 commits into from
Feb 24, 2021

Conversation

barkbay
Copy link
Contributor

@barkbay barkbay commented Feb 22, 2021

This PR allows the user to customize the default autoscaling API polling frequency.

@barkbay barkbay added >feature Adds or discusses adding a feature to the product autoscaling v1.5.0 labels Feb 22, 2021
@barkbay barkbay mentioned this pull request Feb 22, 2021
13 tasks
Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Maybe adjust naming.

@@ -52,6 +57,10 @@ type AutoscalingPolicy struct {
// +kubebuilder:object:generate=false
type AutoscalingSpec struct {
AutoscalingPolicySpecs AutoscalingPolicySpecs `json:"policies"`

// SyncPeriod is the period at which to synchronize and poll the Elasticsearch autoscaling API.
SyncPeriod *metav1.Duration `json:"syncPeriod"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it simply be called pollingFrequency or pollingPeriod? If I read sync I wonder what it is being synchronized with what.

var defaultRequeue = reconcile.Result{
// licenseCheckRequeue is the default duration used to retry a licence check if the cluster is supposed to be managed by
// the autoscaling controller and if the licence is not valid.
var licenseCheckRequeue = reconcile.Result{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the motivation for using a different interval on invalid licenses? To avoid a bad experience for users that set syncPeriod to a really high value? Initially I thought it would be to protect the operator from users that set it to a really low value because the license check is relatively expensive. But I realise that the check runs on every reconciliation loop anyway. Maybe worth a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid a bad experience for users that set syncPeriod to a really high value?

Yes exactly, my initial thought was that if the license is invalid it might be a user mistake, and we may want to trigger a new loop sooner than later, regardless of the syncPeriod value which serves an other purpose.

@barkbay barkbay merged commit e04f78d into elastic:master Feb 24, 2021
@barkbay barkbay mentioned this pull request Mar 8, 2021
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autoscaling >feature Adds or discusses adding a feature to the product v1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants