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

Parquet: Make FindTraceByID honor buffer and caching settings #1697

Merged
merged 5 commits into from
Aug 29, 2022

Conversation

joe-elliott
Copy link
Member

@joe-elliott joe-elliott commented Aug 26, 2022

What this PR does:

  • Skip bloom filters when loading for trace by id search. (Skipping page index caused an nil pointer panic)
  • Honor cache and buffer settings when searching for traces by id.

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
@mdisibio
Copy link
Contributor

mdisibio commented Aug 26, 2022

Skip bloom filters when loading for trace by id search. (Skipping page index caused an nil pointer panic)

The nil panic is probably due to how trace by id search inspects page bounds to do a binary search for the trace, minimizes reads of the page bodies. Do you have any more info about the benefits of skipping bloom filters?

@joe-elliott
Copy link
Member Author

joe-elliott commented Aug 29, 2022

Do you have any more info about the benefits of skipping bloom filters?

I can actually detect no difference between SkipBloomFilters true or false. I'm going to remove.

Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott joe-elliott changed the title Parquet: Querier Find Trace By ID improvements Parquet: Make FindTraceByID honor buffer and caching settings Aug 29, 2022
@joe-elliott joe-elliott enabled auto-merge (squash) August 29, 2022 15:01
@joe-elliott joe-elliott merged commit e177ad0 into grafana:main Aug 29, 2022
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.

2 participants