Remove collisions on PrepareRun stage #546
Conversation
strangeman
changed the title
api/tasks/pool.go
Outdated
| if t := p.queue[0]; t.task.Status != taskFailStatus { | ||
| if t.prepared { | ||
| fmt.Println("Running a task.") | ||
| resourceLocker <- &resourceLock{lock: true, holder: t} |
There was a problem hiding this comment.
this can be outside of the conditional since we do it on both sides of the branch
There was a problem hiding this comment.
yeah, you're right
api/tasks/pool.go
Outdated
| resourceLocker <- &resourceLock{lock: true, holder: t} | ||
| go t.run() | ||
| p.queue = p.queue[1:] | ||
| } else { |
There was a problem hiding this comment.
maybe it makes sense to reverse the logic and put the prepare inside the branch then make run the default behavior that occurs after, outside of a branch
There was a problem hiding this comment.
Yes, this makes sense. In this case, we will get more straightforward logic: "prepare task if it not prepared and then launch it"
|
is there any way we could write a test to prove this works as well? Would be cool to start trying to add tests for this stuff so we can ensure we don't get regressions. |
strangeman
force-pushed
the
545-prepare-collisions
branch
from
089d9c3
to
a817d2d
Compare
|
Uhh, I'm not sure how we can cover it via tests :( |
| resourceLocker <- &resourceLock{lock: true, holder: t} | ||
| if !t.prepared { | ||
| go t.prepareRun() | ||
| continue |
There was a problem hiding this comment.
why do we continue here and not just directly run the task after we prepare it? The next tick will just run the task anyway right?
There was a problem hiding this comment.
nope, without continue it trying to run a task at the same tick with prepare because it checking the lock before this branch
There was a problem hiding this comment.
I should have had a coffee before reviewing this pr 😆
approved
|
ok no worries re: tests, i'll try to add some for it in the coming weeks |
Fix for #545
listPlaybookHostsfor tasks withprojectconcurrency because it not needed for it