Conversation
|
I have run the CI/CD tests on this PR multiple times, and one time on tests failed that I did not saw before: Could be because of these changes? can you please check it out? |
|
Checked it out. Can't see how this is related. The goroutine dump in the CI suggest some problem in the CI runner. Note the IO waits etc. |
martinconic
requested review from
akrem-chabchoub,
gacevicljubisa,
janos and
martinconic
pkg/storer/reserve.go
Outdated
| db.events.Trigger(reserveOverCapacity) | ||
| } | ||
|
|
||
| close(ready) |
There was a problem hiding this comment.
Even if it not read, and we are sure it is created, maybe it is better to put:
if ready != nil {
close(ready)
}
pkg/storer/reserve_test.go
Outdated
| storer.StartReserveWorker(context.Background(), pullerMock.NewMockRateReporter(0), networkRadiusFunc(0)) | ||
| readyC := make(chan struct{}) | ||
| storer.StartReserveWorker(context.Background(), pullerMock.NewMockRateReporter(0), networkRadiusFunc(0), readyC) | ||
| <-readyC |
There was a problem hiding this comment.
It would be good to avoid naked reads from the channel here. For example to have select on on t.Context().Done()?
This goes for all instances in this pr.
There was a problem hiding this comment.
why? it is a test. i can't see why you'd like to have a graceful context cancellation here. i can't really clearly see when a test context would be cancelled (ctrl-c? 600s timeout?), but in any case, if we are stuck on the read from the channel i'd actually like to see an ugly test failure with a goroutine dump (all goroutines blocked message) rather than having the test exit gracefully.
There was a problem hiding this comment.
Test failure on timeout would be much easier to read than to investigate the panic goroutine dump.
5 participants
Description
A data race in the reserve worker's startup was created in a way that it may get scheduled after the dependent tests already started. I've added a channel to synchronize this behavior. In
node.goI am not using this synchronization as it appears to affect the node startup time in the integration tests.This fixes the flakes in the TestReserveEvict and the TestReplaceOldIndex tests.