output/cloudv2: Drain the SampleBuffer on stop and abort once #3105
Conversation
| func (o *Output) collectSamples() { | ||
| samples := o.GetBufferedSamples() | ||
| if len(samples) < 1 { | ||
| return | ||
| } |
There was a problem hiding this comment.
I removed the check based on the assumption we don't expect to invoke this and get an empty buffer because the aggregation period should be wide enough, and in the case, we get it then there are two next operations:
- a for range: so it is skipped
- The expired check: we go over the list one more time but if we get some expired, then they will be removed from the list.
There was a problem hiding this comment.
Even if those assumptions are valid, I don't see how an early return if there's no work to be done would hurt. If nothing else it would simplify understanding this logic without being aware of these nuances.
But it's up to you 🙂
There was a problem hiding this comment.
I don't see how an early return if there's no work to be done would hurt.
@imiric Then the code is not obvious and I think we need to arrange it in another way. The reason why I removed it is because we need to flush the SampleBuffer + the collector's buffer. If we return we can't execute the drain of the collecort's buffer. Do you think it makes sense?
The alternative is doing something one of the following in StopWithTestError:
output.collectSamples()
output.collector.collectSamples(nil)
this has the downside to call two times the collector.CollectSamples method, so another way could be:
output.collector.collectSamples(o.GetBufferedSamples())
There was a problem hiding this comment.
OK, I was misunderstanding where the flush is happening.
I still don't have a full grasp of the code here, but that's on me, so I don't think refactoring would help. 🙂
| func (o *Output) collectSamples() { | ||
| samples := o.GetBufferedSamples() | ||
| if len(samples) < 1 { | ||
| return | ||
| } |
There was a problem hiding this comment.
Even if those assumptions are valid, I don't see how an early return if there's no work to be done would hurt. If nothing else it would simplify understanding this logic without being aware of these nuances.
But it's up to you 🙂
It forces the drain not only of the collector but also of the SampleBuffer when it stops gracefully. Added also a sync once controller to have mutex protection around close otherwise potentially we could have a race around the select case closing multiple times.