-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unordered chunks #4001
Unordered chunks #4001
Conversation
if ordered { | ||
it = iter.NewNonOverlappingSampleIterator(its, "") | ||
} else { | ||
it = iter.NewHeapSampleIterator(ctx, its) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might need variant of HeapIterator that just reorder samples/logs depending on how that affects performance.
I'm guessing this only happening for chunk in memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the unordered version only happens on the ingesters. Since we reorder chunks before flushing to storage (if needed), the unoptimized heapIter versions are only run on the ingesters themselves and only if the ingester is receiving out of order data.
edit: I should look at refactoring the order detection into a helper fn. These methods are way too complex for my tastes :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks fantastic. I have one question.
Since we now allow out of order writes across multiple chunks (new and previous one) I think this means we'll end up with overlapping chunks in the storage.
Does this means we need to modify the function below
https://github.com/grafana/loki/blob/main/pkg/storage/batch.go#L412-L428
which currently seems to assume chunk from the same stream are never overlapping.
We shouldn't need to change the Note: this is already accounted for due to replication |
This is handling overlapping across different stream I think. I'll write a test to clear my suspicion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM,
Let's GOOOOOOOOOOO ! 🥳
* merge feature/unordered-replay * interoperable head chunks * memchunk block interop * retain ordered memchunk optimizations when possible * tests+bench for unordered chunk reads * reorder on chunk close * [wip] ingester stream unorderd * unordered writes default in testware config, fixes OOO bug & removes unused lastChunkTimestamp var * validity window is 1/2 of max age & fixes old transfer test * more consistent headblock checking/creation * more cohesive encoding tests * unordered stream test with validity bounds * compat - unordered * reinstates memchunk defaults when rebounding & updates storage test compatibility * lint * reorder across blocks doesnt overflow * respect chunk configs during rebounding when possible * only sync checks on ordered writes (cherry picked from commit 56256bf)
This PR:
MemChunks
.Dependent on: #3995
ref: #1544