Fix regression brought by using time.NewTimer() in scale loop #1739
Conversation
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
There was a problem hiding this comment.
this is totally my fault @zroubalik - sorry about it! the fix looks good.
| for { | ||
| tmr := time.NewTimer(pollingInterval) | ||
|
|
There was a problem hiding this comment.
@zroubalik this is a subtle fix (yet obviously important!) - is it possible somehow to add a test for it? the only apparent way to me would be to pull the timer-related code out into a utility function and then test that. thoughts?
There was a problem hiding this comment.
(and if so, I can pick that up in a follow-up PR)
There was a problem hiding this comment.
I am concerned that pulling that part of code to the utility function would make the code less readable and complex. WDYT?
Anyway more tests are always good, so if you come up with some good way how to test this stuff, I wouldn't be mad :)
There was a problem hiding this comment.
ha, that was my idea to make it more testable. I don't have others at the moment, but I'll keep my mind open 👍
Signed-off-by: Zbynek Roubalik zroubali@redhat.com
#1704 changed the way how is the timer in scale loop calculated, but we have missed a bug. The timer was initialized only once (at the start), causing the scale loop not perform any checks :)
Checklist