feature: add MetricsUpdateIntervalSeconds args option in trimaran plugins #905
Conversation
k8s-ci-robot
added
kind/feature
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: googs1025 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/S
✅ Deploy Preview for kubernetes-sigs-scheduler-plugins canceled.
|
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
size/M
…gins Signed-off-by: googs1025 <[email protected]>
|
/cc @ffromani @Huang-Wei |
There was a problem hiding this comment.
thanks for the PR. I see the reason and makes sense, but more code means more maintenance burden, so: do we have user request for this change or is it a "just in case" change?
in addition, I think we need unit tests and please please integration tests somehow
| @@ -88,6 +88,8 @@ type TrimaranSpec struct { | |||
| MetricProvider MetricProviderSpec | |||
| // Address of load watcher service | |||
| WatcherAddress string | |||
| // Interval in seconds for periodic metrics updates. Default is 30 seconds if not set. | |||
| MetricsUpdateIntervalSeconds int32 | |||
There was a problem hiding this comment.
usually optional values are pointers to let disambiguate between "value not set" and "value explicitely set to zero". In this case, does the zero value make sense? do we have the possible ambiguity? if not we can probably use the value (vs the pointer) but the API side will need more consideration.
| @@ -69,6 +69,11 @@ func NewCollector(logger klog.Logger, trimaranSpec *pluginConfig.TrimaranSpec) ( | |||
| client, _ = loadwatcherapi.NewLibraryClient(opts) | |||
| } | |||
|
|
|||
| metricsUpdateIntervalSeconds := trimaranSpec.MetricsUpdateIntervalSeconds | |||
| if metricsUpdateIntervalSeconds == 0 { | |||
There was a problem hiding this comment.
int32 can be negative. Do we have some validation preventing negative values?
What type of PR is this?
/kind feature
What this PR does / why we need it:
This change introduces a new field
MetricsUpdateIntervalSeconds, in the TrimaranSpecArgs. This allows users to define their own interval for periodic metrics updates instead of relying on a hardcoded value.Benefits:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?