Fix incorrect fallback to skip cache in case of very high concurrency level#40420
Fix incorrect fallback to skip cache in case of very high concurrency level#40420kssenii merged 3 commits intoClickHouse:masterfrom
Conversation
robot-clickhouse
added
the
pr-bugfix
| assert(is_downloaded); | ||
| break; | ||
| } | ||
| case State::DOWNLOADING: |
There was a problem hiding this comment.
And what does it mean? DOWNLOADING state doesn't look like something "completed".
There was a problem hiding this comment.
Yes, so in this case nothing is actually done as you can see, only one assertion and then we return. This case is here for the case when completeWithoutState -> completeBasedOnCurrentState is used - which is called from FileSegmentHolder's destructor which is called always in destructor of read buffers. We see that state is DOWNLOADING and is_last_holder == false && is_downloader == false and do nothing. For completeWithState it will throw exception that it is not possible to complete with DOWNLOADING state.
| case State::PARTIALLY_DOWNLOADED: | ||
| case State::PARTIALLY_DOWNLOADED_NO_CONTINUATION: | ||
| { | ||
| if (is_last_holder) |
There was a problem hiding this comment.
I don't understand this if. So if we are not the last holder than last holder will do it?
There was a problem hiding this comment.
I'm asking because it's unclear that eventually this code will be called.
There was a problem hiding this comment.
So if we are not the last holder than last holder will do it?
Yes, because completeWithoutState is always called for any user of the file segment - from destructor of FileSegmentsHolder -- file segments are always put in a special holder which calls complete() for the in destructor. E.g. eventually is_last_holder will be true.
| download_state = FileSegment::State::SKIP_CACHE; | ||
| } | ||
|
|
||
| download_state = file_segment->wait(); |
There was a problem hiding this comment.
The main point is here. In complete() method we always called notify, but complete() could be called when the state did not change, so we returned from wait() for no reason. E.g. the problem was in calling cv.notify regardless which is called from file segment holder destructor, e.g. from destructor of each read buffer which tries to hold this file segment.
alesapin
left a comment
alesapin
left a comment
There was a problem hiding this comment.
Ok, only last owner now call notify.
3 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix incorrect fallback to skip cache which happened on very high concurrency level