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

Avoid loading all symbols for each partition destination #2825

Merged
merged 7 commits into from Dec 18, 2023

Conversation

cyriltovena
Copy link
Contributor

Sounds like partition.init doesn't care and will always load all symbols for the current partition.

It seems like an oversight.

@cyriltovena
Copy link
Contributor Author

The tests is showing that Load and query path is not friendly. Thinking about how to solve this, basically we shouldn't not use the symbols for the read path if Load is used.

@cyriltovena cyriltovena marked this pull request as ready for review December 12, 2023 09:37
@cyriltovena cyriltovena requested a review from a team as a code owner December 12, 2023 09:37
Copy link
Collaborator

@kolesnikovae kolesnikovae left a comment

Choose a reason for hiding this comment

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

Nice catch, thanks for fixing this!

The tests is showing that Load and query path is not friendly. Thinking about how to solve this, basically we shouldn't not use the symbols for the read path if Load is used.

Agreed: the only use case for Load is compaction, and it should never be used in our regular queries (however, loaded blocks should be queryable). Moreover, the test itself is not quite useful – it's worth counting object storage client calls to ensure that a partition is only fetched once at Load call. I'll take care of the test when will optimize symbols rewriter (this week hopefully)

pkg/phlaredb/symdb/block_reader.go Outdated Show resolved Hide resolved
@cyriltovena cyriltovena enabled auto-merge (squash) December 18, 2023 09:28
@cyriltovena cyriltovena merged commit c5ce1c9 into main Dec 18, 2023
19 checks passed
@cyriltovena cyriltovena deleted the bugfix/compact-load-symbols branch December 18, 2023 09:33
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.

None yet

2 participants