Skip to content
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

Merged
merged 5 commits into from
Feb 25, 2022

Conversation

sandeepsukhani
Copy link
Contributor

@sandeepsukhani sandeepsukhani commented Feb 24, 2022

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:

benchmark                                     old ns/op      new ns/op      delta
Benchmark_QueriesMatchingLargeNumOfRows-8     2027105841     1298895890     -35.92%

benchmark                                     old allocs     new allocs     delta
Benchmark_QueriesMatchingLargeNumOfRows-8     29321207       10141758       -65.41%

benchmark                                     old bytes      new bytes     delta
Benchmark_QueriesMatchingLargeNumOfRows-8     1032313592     507791808     -50.81%

This PR also adds the benchmarks to the repository.

Checklist

  • Tests updated

@sandeepsukhani sandeepsukhani requested a review from a team as a code owner February 24, 2022 12:15
Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@kavirajk kavirajk left a 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) {
Copy link
Contributor

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?

Copy link
Contributor Author

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
Copy link
Contributor

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.

Copy link
Contributor

@kavirajk kavirajk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants