lostluck commented on code in PR #38736:
URL: https://github.com/apache/beam/pull/38736#discussion_r3325833061
##########
sdks/go/pkg/beam/core/runtime/exec/datasampler_test.go:
##########
@@ -104,16 +104,9 @@ func TestDataSampler(t *testing.T) {
for _, sample := range test.samples {
dataSampler.SendSample(sample.PCollectionID,
sample.Element, sample.Timestamp)
}
- var samplesCount = -1
- var samples map[string][]*DataSample
- for i := 0; i < 5; i++ {
- samples = dataSampler.GetSamples(test.pids)
- if len(samples) == len(test.want) {
- samplesCount = len(samples)
- break
- }
- time.Sleep(time.Second)
- }
+ time.Sleep(1 * time.Second)
+ samples := dataSampler.GetSamples(test.pids)
+ samplesCount := len(samples)
Review Comment:
OK, we could probably use synctest, but outside of the academic value of
doing so, I don't think there's a good point at this stage.
This is still better than it was before (one sleep instead of several lossy
samples. The main issue with the previous test is the lossiness since I guess
it assumed that it was all or nothing and didn't maintain the intermediate
samples during it's five checks. That said this feels like it might be just as
flaky as before...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
