Per-artifact build results v2: use channels as futures to send results from builders to runner #2077
Conversation
nkubala
requested review from
balopat,
dgageot,
priyawadhwa and
tejal29
as code owners
There was a problem hiding this comment.
I am little bit worried about the "InSequence" flow.
I found this interesting book with Go Design Patterns
A snippet of [book] (https://learning.oreilly.com/library/view/go-design-patterns/9781786466204/ch09.html
) here:--
"Now that we are familiar with the concepts of concurrency and parallelism, and we have understood how to achieve them by using Go's concurrency primitives, we can see some patterns regarding concurrent work and parallel execution. In this chapter we'll see the following patterns:
- Barrier is a very common pattern, especially when we have to wait for more than one response from different Goroutines before letting the program continue.
- Future pattern allows us to write an algorithm that will be executed eventually in time (or not) by the same Goroutine or a different one
- Pipeline is a powerful pattern to build complex synchronous flows of Goroutines that are connected with each other according to some logic"
Looks like we are merging 2 patterns
- Barrier (since runner waits for the responses for all build)
- Future (fire and forget pattern)
Trying to think how we can simplify this.
nkubala
force-pushed
the
build-res-channel
branch
2 times, most recently
from
9c7de4c
to
bb8a577
Compare
Codecov Report
@@ Coverage Diff @@
## master #2077 +/- ##
==========================================
+ Coverage 56.12% 56.33% +0.21%
==========================================
Files 180 180
Lines 7756 7810 +54
==========================================
+ Hits 4353 4400 +47
- Misses 2987 2993 +6
- Partials 416 417 +1
Continue to review full report at Codecov.
|
nkubala
force-pushed
the
build-res-channel
branch
2 times, most recently
from
0df4928
to
94b5376
Compare
| func InSequence(ctx context.Context, out io.Writer, tags tag.ImageTags, artifacts []*latest.Artifact, buildArtifact artifactBuilder) ([]Artifact, error) { | ||
| var builds []Artifact | ||
| func InSequence(ctx context.Context, out io.Writer, tags tag.ImageTags, artifacts []*latest.Artifact, buildArtifact artifactBuilder) ([]chan Result, error) { | ||
| resultChans := make([]chan Result, len(artifacts)) |
There was a problem hiding this comment.
I'd argue that this does the same:
resultChans := make([]chan Result, len(artifacts))
for i, a := range artifacts {
resultChan := make(chan Result, 1)
resultChans[i] = resultChan
resultChan <- doBuild(ctx, out, tags, a, buildArtifact)
}
return resultChans, nil
|
@balopat and @nkubala, I have this prototype to implement this using a single channel Let me know what you think. |
|
I think it's worthwhile to experiment with it. I'll check it out. |
| var built []Artifact | ||
|
|
||
| for i, artifact := range artifacts { | ||
| for i := range artifacts { |
There was a problem hiding this comment.
Because we left this here, the parallel builder won't return until all the outputs are consumed - which means that the eventing logic is going to be "batched" until all builds are done.
So:
- we should write a test to ensure "realtimeness" of the events somehow
- we should move the output handling to another go routine
|
| if len(artifacts) == 1 { | ||
| return InSequence(ctx, out, tags, artifacts, buildArtifact) | ||
| } | ||
|
|
||
| resultChans := make([]chan Result, len(artifacts)) | ||
|
|
||
| ctx, cancel := context.WithCancel(ctx) | ||
| defer cancel() |
There was a problem hiding this comment.
This is a subtle bug here: we need to make sure that this cancel() happens only after all the builds are happened. Otherwise we might run into issues that this method returns while the builds are happening, and they are going to get cancelled. I fixed it in #2091.
|
After multiple days invested in this direction I still feel that it is not worthwhile to go down this direction: the complexity of adding go channels adds a lot of complexity and the design more error prone for subtle bugs. The current arguments for going towards this direction is the Event API redesign to centralize event management in the Runner. This in itself is a valuable idea but at this point the current design is simpler to reason about this new design with the go channels, plus if we want to extend the events sent by builders, e.g. progress bar events, we will have to send data through the go channels anyway that represents those events and convert them to events, or directly call the Thank you @nkubala and @tejal29 for all the hard work in exploring this avenue in detail in code with testing and thoughtful conversations! |
NOTE: this PR is an extension of #2000, which will be closed in favor of this. please read the description and comments before reading this for context!
this change uses channels as makeshift futures to send build results back from builders to the runner asynchronously. this has the advantage of giving the runner the ability to immediately dispatch events when results are received, rather than needing to wait for all build threads to finish before dispatching events.
the event changes are NOT included in this PR: they will come in a follow up change.
the signature for the builder methods now becomes
the runner receives a slice of channels back from the builder, each promising to return a result for its corresponding build. the runner will block until it has received a result from each channel, and then continue with the rest of the pipeline.