-
Notifications
You must be signed in to change notification settings - Fork 460
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
Refactor Distributor.push()
#6978
Conversation
Extracted methods, removed needless receivers, moved code to mimirpb. This should not change any logic. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
We already have a WriteRequest allocated, there's no need to create another one. Also the signature is shorter now. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
e2be173
to
e5aff2b
Compare
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.
Seems ok; I have a question about naming.
pkg/distributor/distributor.go
Outdated
return nil | ||
// getSeriesAndMetadataTokensForBatchRequest returns a slice of tokens for the series and metadata from the request in this specific order. | ||
// Metadata tokens start at initialMetadataIndex. | ||
func getSeriesAndMetadataTokensForBatchRequest(userID string, req *mimirpb.WriteRequest) (keys []uint32, initialMetadataIndex int) { |
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.
What makes it a "BatchRequest" ? This is called with the entire request; I thought the batches were what DoBatch splits it into.
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.
My reasoning was: since we pass this to ring.DoBatch ... but if this raised you a question, then it's not helping. I'm going to remove the ForBatchRequest
suffix from this method.
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
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.
If you want to further simplify i think we can, but it looks good as-is too. Thanks for the cleanup
for _, i := range indexes { | ||
if i >= initialMetadataIndex { | ||
metadataCount++ | ||
} else { | ||
timeseriesCount++ | ||
} | ||
} |
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.
can't we do this with binary search?
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 could, but it's not necessarily faster. We'd need a benchmark, and I need to stop somewhere :D
What this PR does
Makes
Distributor.push
shorter and hopefully cleaner.Which issue(s) this PR fixes or relates to
None.
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]
.about-versioning.md
updated with experimental features.