vertexai(test): Run corpora test in go coroutine to reduce test runtime #10841
Conversation
happy-qiao
changed the title
| worker := func() { | ||
| for corpora := range corporaChan { | ||
| if ucr.shouldSkip(corpora.Name) { | ||
| fmt.Printf("Skipping file: %s\n", corpora.Name) |
There was a problem hiding this comment.
fmt is not usually concurrency-safe (nothing bad will happen but output can get mixed up between goroutines).
Use log.Printf inside goroutines instead
| } | ||
| doneChan <- true |
There was a problem hiding this comment.
I think this code will work fine, but since your workers don't actually return any results, I recommend a slightly different pattern, using a WorkGroup.
This example shows how: https://gobyexample.com/waitgroups
Then you won't need the loop clearing the doneChan in the main goroutine; you'll just do a wait on the group.
| if err != nil { | ||
| log.Fatalf("Failed to decode bytes: %v", err) | ||
| } | ||
| workerCountChan := make(chan int, 10) |
There was a problem hiding this comment.
I'm not sure I understand what this channel is for -- can you please explain?
There was a problem hiding this comment.
I use this channel to manage up to 10 corpora run simultaneously, preventing them from all launching at once.
There was a problem hiding this comment.
I see, thanks for clarifying.
In this case the naming or a comment should be used to make this clearer. Also, for channels like this it's customary to have them carry no information, e.g. chan struct{} -- this further clarifies that the contents of the channel have no meaning.
The channel can be named something like workLimiter or concurrencyLimiter, or if you prefer to keep your channel name, just add a comment explaining what it's for.
There was a problem hiding this comment.
Done. Thanks for helping me improve it!
eliben
added
the
automerge
gcf-merge-on-green
bot
removed
the
automerge
No description provided.