TestRunner: Remove step stopping loop in RunMonitor#126
Conversation
Codecov Report
@@ Coverage Diff @@
## main #126 +/- ##
==========================================
- Coverage 63.92% 63.81% -0.11%
==========================================
Files 164 164
Lines 10321 10304 -17
==========================================
- Hits 6598 6576 -22
- Misses 3007 3013 +6
+ Partials 716 715 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
Only one place where conditional variable is used is left will remove it next |
| targets map[string]*targetState // Target state lookup map | ||
| steps []*stepState // The pipeline, in order of execution | ||
| targets map[string]*targetState // Target state lookup map | ||
| stepsTargetsCount []int32 // Holds counter of the amount of targets that will use this step |
There was a problem hiding this comment.
seems like step related data, why doesnt this go into stepState?
There was a problem hiding this comment.
currently stepState doesn't contain information about the counters.
Let me think if putting them there will make the code look better
There was a problem hiding this comment.
len(steps) == len(stepsTargetsCount), and theyre indexed by the target index, so it looks to me like it could be joined
|
|
||
| for _, tgs := range tr.targets { | ||
| for i := 0; i < tgs.CurStep; i++ { | ||
| tr.stepsTargetsCount[i]-- |
There was a problem hiding this comment.
some comments regading the logic here would be nice; it took me some reading to understand how this is supposed to work
|
|
||
| for _, tgs := range tr.targets { | ||
| for i := 0; i < tgs.CurStep; i++ { | ||
| tr.stepsTargetsCount[i]-- |
There was a problem hiding this comment.
how would this work with the future "inject new target from plugin" feature? i suppose there would need to be some api to increment the expected target count per following steps (from the one that injects)
There was a problem hiding this comment.
Could you please tell me more about that feature?
If steps will generate targets I guess we will not close input channels for steps at all or reinitialise.
There was a problem hiding this comment.
Anyway currently the only thing that loop in runMonitor does is just watching if all targets went through the step + watch for errors. Better to keep explicit ref count and watch for errors in another place.
There was a problem hiding this comment.
it's prob enough to provide an api to inject targets from a step (and since this one step is running, following must be running or not started). this api would also increment expected targets counts for the following steps, so the close of in chan should not be affected.
| for i := lastDecremented + 1; i < len(tr.stepsTargetsCount); i++ { | ||
| if atomic.AddInt32(&tr.stepsTargetsCount[i], -1) == 0 { | ||
| ctx.Infof("Stop step: %d", i) | ||
| tr.steps[i].Stop() |
There was a problem hiding this comment.
can you extract this? i dont like the coupling between a function that is run per target (handler) and manipulating test runner state (which is technically a parent for this code here; tr shouldnt even be a pointer receiver here but go doesnt have immutability concepts...).
There was a problem hiding this comment.
note, i understand that the monitorCond had the risk of deadlocking if it wasnt signaled in the right places, but it did have some separation of concerns that youre missing here
There was a problem hiding this comment.
But targetHandler already manipulates tr.steps[i] when it Runs it or injects target.
There was a problem hiding this comment.
that should prob go thru some func api as well. im ok with leaving this as a todo
rihter007
force-pushed
the
refactor/remove_step_stopping_loop
branch
from
c5974e6 to
ad10ec4
Compare
…red in stepsTargetsCount Signed-off-by: Ilya <rihter007@inbox.ru>
rihter007
force-pushed
the
refactor/remove_step_stopping_loop
branch
from
ad10ec4 to
e9a9bd1
Compare
3 participants
Fixes #62