Bug 1917367: Refactor periodic.go #302
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
approved
0sewa0
force-pushed
the
Refactor_periodic_go
branch
from
44d58b4 to
81eacb7
Compare
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
/retest |
pkg/controller/periodic/periodic.go
Outdated
| defer closeFn() | ||
|
|
||
| interval := c.config.Config().Interval | ||
| if c.initialDelay > 0 { |
There was a problem hiding this comment.
What happens if the initialDelay = 0? Will the gathering be executed immediately? It doesn't seem so....but maybe I am missing something
There was a problem hiding this comment.
Yep. It looks weird. AFIK, the Gather will not be executed if the initialDelay is less or eq to zero. I think that this behavior is not desired. Can we put some default value for it?
There was a problem hiding this comment.
InitialDelay has the default value 0 - https://github.com/openshift/insights-operator/blob/master/pkg/controller/operator.go#L63
There was a problem hiding this comment.
if initialDelay == 0 then the gathering will start as normal at https://github.com/openshift/insights-operator/pull/302/files#diff-dec12f0cf28a91a837b46e755b46c081c9014121cdb770b2cd8ac916af29c8deR133
There was a problem hiding this comment.
or do we want to start a gather immediately if no initialDelay is set ?
There was a problem hiding this comment.
updated it
it should run a gather immediately if initialDelay is 0 or less
pkg/controller/periodic/periodic.go
Outdated
| defer closeFn() | ||
|
|
||
| interval := c.config.Config().Interval | ||
| if c.initialDelay > 0 { |
There was a problem hiding this comment.
Yep. It looks weird. AFIK, the Gather will not be executed if the initialDelay is less or eq to zero. I think that this behavior is not desired. Can we put some default value for it?
|
/retest |
0sewa0
force-pushed
the
Refactor_periodic_go
branch
from
dbe8cc5 to
edaf512
Compare
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
openshift-ci-robot
added
bugzilla/severity-high
|
@0sewa0: This pull request references Bugzilla bug 1917367, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/retest |
5 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
I experimented with that locally and it seems to work OK. Thanks |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 0sewa0, tremes The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@0sewa0: All pull requests linked via external trackers have merged: Bugzilla bug 1917367 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Refactors
periodic.goin a way that it doesn't use overly complicated workload management (it should be the responsibility of the gatherers to manage the workloads) and as it was written ~2 years ago and wasn't changed since so it doesn't fit how the actual codebase developed since then. (we only have 1 Gatherer but that has multiple independent gather functions, yetperiodic.gotries to manage multiple Gatherers)Categories
Sample archive
Documentation
Unit Tests
Privacy
Has no effect on privacy
References
None