-
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
Stream query responses from boltdb index client #5468
Stream query responses from boltdb index client #5468
Conversation
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
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. Suggested few minor nits
@@ -20,7 +20,7 @@ import ( | |||
chunk_util "github.com/grafana/loki/pkg/storage/chunk/util" | |||
) | |||
|
|||
func AddRecordsToDB(t *testing.T, path string, dbClient *local.BoltIndexClient, start, numRecords int, bucketName []byte) { | |||
func AddRecordsToDB(t testing.TB, path string, dbClient *local.BoltIndexClient, start, numRecords int, bucketName []byte) { |
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.
do we need testing.TB
here?
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, since we use it with both tests and benchmarks.
}) | ||
|
||
if err != nil { | ||
return err |
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.
may be worth wrapping this error? Because from the send
method(), the error can be simply "timedout sending response". which I believe lacks context when I see this error in practice.
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 👍
What this PR does / why we need it:
Previously, we sent just one row per batch in the index query response callback from the boltdb index client.
This, in turn, makes us send just one row at a time per grpc response from the index gateway server.
This PR improves it by streaming the response from the boltdb index client, which would, in turn, lets us send back a larger batch in grpc response from the index gateway server.
Here is the benchmark comparing the performance difference:
This PR also adds the benchmarks to the repository.
Checklist