Skip to content

Fix stress test#46521

Merged
kssenii merged 1 commit intoClickHouse:masterfrom
kssenii:fix-stress-test-logical-errors
Feb 17, 2023
Merged

Fix stress test#46521
kssenii merged 1 commit intoClickHouse:masterfrom
kssenii:fix-stress-test-logical-errors

Conversation

@kssenii
Copy link
Member

@kssenii kssenii commented Feb 17, 2023

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

@robot-ch-test-poll4 robot-ch-test-poll4 added the pr-not-for-changelog This PR should not be mentioned in the changelog label Feb 17, 2023
@antonio2368 antonio2368 self-assigned this Feb 17, 2023
@alexey-milovidov alexey-milovidov self-assigned this Feb 17, 2023
@antonio2368 antonio2368 removed their assignment Feb 17, 2023
@kssenii
Copy link
Member Author

kssenii commented Feb 17, 2023

Screenshot 2023-02-17 at 21 01 08

@kssenii kssenii merged commit a0b28ed into ClickHouse:master Feb 17, 2023
Comment on lines -549 to -550
if (task->reader.valid())
task->reader.wait();
Copy link
Member

Choose a reason for hiding this comment

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

@kssenii I was looking into this too, so just to ensure that I'm we are on the same page, do I understand correctly that the problem was that not all features are kept in the MergeTreePrefetchedReadPool (getTask moves it out), hence some of the got lost and not waited properly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, correct, according to the logs of stress test, the query was killed by the memory overcommit tracker and that is why it was not waited properly for some of the tasks which were already collected with getTask, but the same can also happen if some exception is thrown after the thread collected the task with getTask and before extracting the readers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants