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

optimize file segment PARTIALLY_DOWNLOADED get read buffer #37338

Merged
merged 2 commits into from
May 20, 2022

Conversation

xiedeyantu
Copy link
Contributor

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

optimize file segment PARTIALLY_DOWNLOADED get read buffer

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

@robot-clickhouse robot-clickhouse added the pr-improvement Pull request with some product improvements label May 19, 2022
@kssenii kssenii self-assigned this May 19, 2022
@kssenii kssenii added the can be tested Allows running workflows for external contributors label May 19, 2022

auto downloader_id = file_segment->getOrSetDownloader();
Copy link
Member

Choose a reason for hiding this comment

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

This would not be fully correct.
It is possible that can_start_from_cache is false, then it can become true right before calling file_segment->getOrSetDownlaoder() which can also turn out as true. As a result we will use a buffer which reads from remote fs, but in fact should start with cached buffer. So we anyway need this can_start_from_cache check after if (downloader_id == file_segment->getCallerId()).

Let's leave this can_start_from_cache check in both places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got it, I will change my code. please review later.

@xiedeyantu xiedeyantu requested a review from kssenii May 19, 2022 12:17
@kssenii kssenii merged commit 5b179bd into ClickHouse:master May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
can be tested Allows running workflows for external contributors pr-improvement Pull request with some product improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants