Ensure we check iterators errors when deduplicating #828
Conversation
| @@ -261,7 +261,9 @@ func skipDuplicates(ctx context.Context, its []MergeIterator) error { | |||
| } | |||
| span.LogFields(otlog.Int("duplicates", duplicates)) | |||
| span.LogFields(otlog.Int("total", total)) | |||
|
|
|||
| for _, it := range its { | |||
| errors.Add(it.Err()) | |||
There was a problem hiding this comment.
Not fully sure if that will really cover all bases, in theory Err needs to be called every time Next() of the particular iterator is false. So that probably should happen as part of moveNext() and kept in the tree struct.
Still better than having no errors at all. 🙂
There was a problem hiding this comment.
Yeah this is a very good point. I think We can fix that too let me see.
There was a problem hiding this comment.
Checking for each iterators errors on a false next might be expensive if we do this outside of the loser tree.
I'm wondering if we should change the tree to be aware of Err() which it is not right now. @bboreham wanted to ultimately have the same library for all project, but some doesn't have this Err() and this could impact performance.
I'm inclined to have two version and may be use that new version with Err() management in Pyroscope.
WDYT ?
There was a problem hiding this comment.
Yes I think it is worth having the extra check. I guess at least Next is most times true, before it is once false for every iterator.
There was a problem hiding this comment.
I'll replace that PR with another one then
I suspect this could be the cause for couple of missing errors.