Improve entry deduplication. #2302
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR removes mostcommon and sort insert function in the heap iterator. I discovered while working on grafana#2293 that those are actually not helping since we're deduping those lines anyways. There were no tests checking if deduping was correctly working so I did added those. Bonus point this means deduping will run faster and the code is less complex. The only side effect is that the order of entries that are at the same timestamp, before the most common entry would appear first, now we keep the same order as we stored them, which I think is better. I also change the label ordering, I think whether we are forward or backward we should keep the same aphabetical labels ordering not sure why direction was altering this before. Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
owen-d
reviewed
Jul 8, 2020
pkg/iter/iterator_test.go
Outdated
| NewStreamIterator(foo), | ||
| }, logproto.BACKWARD) | ||
| // first reverse streams, they should already be correctly ordered for the heap iterator to work. | ||
| for i, j := 0, len(foo.Entries)-1; i < j; i, j = i+1, j-1 { |
There was a problem hiding this comment.
This is actually mutating the internal entries used by the stream iter as they reference the same slice. I think you'll want a function which creates the same []stream to guarantee we don't mutate the same underlying data. Basically the testware and the HeapIter should use identical but separate copies of the same data.
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #2302 +/- ##
==========================================
+ Coverage 61.03% 61.04% +0.01%
==========================================
Files 158 158
Lines 12778 12751 -27
==========================================
- Hits 7799 7784 -15
+ Misses 4394 4379 -15
- Partials 585 588 +3
|
owen-d
approved these changes
Jul 8, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR removes
mostcommonandsort insertfunction in the heap iterator. I discovered while working on #2293 that those are actually not helping since we're deduping those lines anyways. There were no tests checking if deduping was correctly working so I did added those.Bonus point this means deduping will run faster and the code is less complex. The only side effect is that the order of entries that are at the same timestamp, before the most common entry would appear first, now we keep the same order as we stored them, which I think is better.
I also change the label ordering, I think whether we are forward or backward we should keep the same alphabetical labels ordering not sure why direction was altering this before.
Signed-off-by: Cyril Tovena cyril.tovena@gmail.com