Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Test UpdateStatus Hook and add UniterParams #2586
Conversation
wallyworld
reviewed
Jun 18, 2015
| @@ -28,12 +28,30 @@ func inactiveMetricsTimer(_, _ time.Time, _ time.Duration) <-chan time.Time { | ||
| return nil | ||
| } | ||
| +type timerChooser struct { |
wallyworld
Jun 18, 2015
Owner
not exported, but a comment as to what this is for i think would be good
wallyworld
reviewed
Jun 18, 2015
| + return t.inactive | ||
| +} | ||
| + | ||
| +func NewMetricsTimerChooser() *timerChooser { |
wallyworld
reviewed
Jun 18, 2015
| @@ -276,7 +276,7 @@ func ModeAbide(u *Uniter) (next Mode, err error) { | ||
| // idleWaitTime is the time after which, if there are no uniter events, | ||
| // the agent state becomes idle. | ||
| -var idleWaitTime = 2 * time.Second | ||
| +var idleWaitTime = 10 * time.Second |
wallyworld
Jun 18, 2015
Owner
Why did we change this? 10s is too long IMO. A hook will show executing long after it has finished.
fwereade
Jun 18, 2015
Contributor
Because the underlying event resolution is 5s, and I think that 5s is the absolute minimum; and I'd rather see units take a bit longer in yellow and then go solid green, rather than to flicker optimistically back and forth. I could maybe be argued down closer to 5s?
wallyworld
reviewed
Jun 18, 2015
| @@ -204,6 +206,21 @@ func (s *UniterSuite) TestUniterInstallHook(c *gc.C) { | ||
| }) | ||
| } | ||
| +func (s *UniterSuite) TestUniterUpdateStatusHook(c *gc.C) { |
wallyworld
Jun 18, 2015
Owner
Is this sufficient? The status update hook also fires after significant events eg config-changed hook etc. Perhaps there are other tests I've forgotten about.
perrito666
Jun 18, 2015
Contributor
It is suficcient, the tests for the additional calls of update status are in the following patch which, additionally has those calls :)
fwereade
reviewed
Jun 18, 2015
| @@ -46,6 +46,8 @@ type UniterExecutionObserver interface { | ||
| HookFailed(hookName string) | ||
| } | ||
| +type simpleUniterFunc func(*Uniter) error |
fwereade
reviewed
Jun 18, 2015
| + LeadershipManager coreleadership.LeadershipManager | ||
| + DataDir string | ||
| + HookLock *fslock.Lock | ||
| + MetricsTimer *timerChooser |
fwereade
reviewed
Jun 18, 2015
| + DataDir string | ||
| + HookLock *fslock.Lock | ||
| + MetricsTimer *timerChooser | ||
| + UpdateStatusSignal TimedSignal |
fwereade
Jun 18, 2015
Contributor
Did we actually want/need this? I thought we could do it entirely within the loop func.
(To be clear, I'm quite happy to have the timing managed by some type; but I don't think it's instance-scoped, I think it's only relevant within the abide loop.)
fwereade
reviewed
Jun 18, 2015
| + metricsTimer: uniterParams.MetricsTimer, | ||
| + // TODO(perrito666) make these signals directly fetched from | ||
| + // the timer. | ||
| + collectMetricsAt: uniterParams.MetricsTimer.defaultTimer(), |
fwereade
Jun 18, 2015
Contributor
why defaultTimer and not inactiveTimer? we're making an explicit choice to use an inactive timer at this point, not because it's default but because it's not safe to read the data that would allow us to choose otherwise; and later we will make an explicit choice between active and inactive timers depending on the charm's features. There's nothing default about inactivity.
|
Apart from some naming quibbles, this LGTM with agreement from ian on idle timing. |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
added a commit
that referenced
this pull request
Jun 19, 2015
perrito666 commentedJun 17, 2015
Added proper testing for update-status hook and also
converted uniter to use UniterParams instead of a lot
of arguments, making it cleaner to use.
A proper way to change metrics timer was also added.