Skip to content

Add BenchmarkChunkQueryableFromTar#1473

Merged
tomwilkie merged 2 commits intocortexproject:masterfrom
codesome:tar-bench
Jun 28, 2019
Merged

Add BenchmarkChunkQueryableFromTar#1473
tomwilkie merged 2 commits intocortexproject:masterfrom
codesome:tar-bench

Conversation

@codesome
Copy link
Contributor

BenchmarkChunkQueryableFromTar is same as TestChunkTar in pkg/querier/chunk_tar_test.go. This benchmark would come handy in investigating a query offline.

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
@bboreham
Copy link
Contributor

Can you factor out the common code?

@codesome
Copy link
Contributor Author

Done

@bboreham
Copy link
Contributor

What is the reason the remaining code is different between the benchmark and the test?
Looks like a limit and a timeout.

@codesome
Copy link
Contributor Author

Yes, I was not sure if I should alter those limits (samples and timeout) for TestChunkTar. And I was hitting both the limits when I was doing some benchmarks locally, so increased the limits for the benchmark.

Also, I would need calling b.ResetTimer() and b.ReportAllocs() for the benchmark, so the common code would be the one before that.

@bboreham
Copy link
Contributor

If you can explain why the limits need to be different, please put that in a comment.
If not, remove the difference.

I don't usually put ReportAllocs() in the code because it is available as a command-line option -benchmem

@codesome
Copy link
Contributor Author

Made the limits same, and removed b.ReportAllocs().

@bboreham
Copy link
Contributor

so more of the code can be shared now?

@codesome
Copy link
Contributor Author

b.ResetTimer() was in between. I will try sharing more code.

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
@codesome
Copy link
Contributor Author

And done!

Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

🎉

@tomwilkie
Copy link
Contributor

👍 LGTM

@tomwilkie tomwilkie merged commit a544ff3 into cortexproject:master Jun 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants