Handle skipped stage by piped #3525
Conversation
|
Code coverage for golang is
|
| continue | ||
| } | ||
| status = model.StageStatus_STAGE_SKIPPED | ||
| ctxWithTimeout.Done() |
There was a problem hiding this comment.
If this stage has already been skipped, all of the analysis should be stopped gracefully.
Thus, call ctxWithTimeout.Done() in order to stop all stages of the analysis.
| defer close(doneCh) | ||
| go func() { | ||
| ticker := time.NewTicker(5 * time.Second) | ||
| defer ticker.Stop() |
There was a problem hiding this comment.
If all stages of the analysis have already been done, this should be stopped gracefully too.
|
let me fix it to save the commander who skipped the stage as metadata. |
|
/hold cancel |
|
Code coverage for golang is
|
|
Code coverage for golang is
|
There was a problem hiding this comment.
@nghialv @khanhtc1202
Sorry, I fixed some points.
So, PTAL when you have enough time.
|
Sure. |
| // Sync the skip command. | ||
| var ( | ||
| status = model.StageStatus_STAGE_NOT_STARTED_YET | ||
| doneCh = make(chan struct{}) | ||
| ) | ||
| defer close(doneCh) | ||
| go func() { | ||
| ticker := time.NewTicker(5 * time.Second) | ||
| defer ticker.Stop() |
There was a problem hiding this comment.
Since the errgroup is being used so how about running the checker inside a new goroutine of that errgroup instead of using a raw goroutine + doneCh?
https://pkg.go.dev/golang.org/x/sync/errgroup#Group.Go
the checker can return a "skipError" to check at Wait function.
There was a problem hiding this comment.
I'm sorry if my recognition is wrong.
I dare to generate a goroutine separately from the errgroup since the goroutine generated by the errgroup waits until all routines are completed.
Hence, I think it seems to be difficult to stop gracefully the goroutine which runs checkStopped in that implementation. wdyt?
There was a problem hiding this comment.
https://pkg.go.dev/golang.org/x/sync/errgroup#Group.Go
The first call to return a non-nil error cancels the group; its error will be returned by Wait.
And the Wait waits for the completion of all its goroutines.
But I think that is what we are expecting.
Once it got a "SKIP" command and the executing goroutines receive the cancel signal and stop their work.
And then the stage waits for the canceling of all its analyses before returning a completion status.
There was a problem hiding this comment.
We can do that when we received the skipped command but I wonder how to handle simply whether the all of analyses were already done or not in order to pass through the Wait section.
There was a problem hiding this comment.
I got your point. It would be complex to cancel the goroutine which is handling the command checker once all analyses are completed normally.
There was a problem hiding this comment.
I got your point. It would be complex to cancel the goroutine which is handling the command checker once all analyses are completed normally.
knanao
force-pushed
the
handle-skip-stage
branch
from
761169b
to
36f431a
Compare
|
Code coverage for golang is
|
|
Nice adding! /lgtm |
1 similar comment
|
Nice adding! /lgtm |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
This makes piped enable to handle the skipped stage.
And add the notification of this in another PR.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: