Skip to content

Detach threads from thread group#43781

Merged
tavplubix merged 3 commits intomasterfrom
fix_assertion_in_thread_status
Nov 30, 2022
Merged

Detach threads from thread group#43781
tavplubix merged 3 commits intomasterfrom
fix_assertion_in_thread_status

Conversation

@tavplubix
Copy link
Copy Markdown
Member

@tavplubix tavplubix commented Nov 28, 2022

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

It is supposed to fix issues like #43737, #43120, #37649

@robot-clickhouse robot-clickhouse added the pr-not-for-changelog This PR should not be mentioned in the changelog label Nov 28, 2022
@alesapin alesapin mentioned this pull request Nov 29, 2022
Merged
@tavplubix
Copy link
Copy Markdown
Member Author

tavplubix commented Nov 29, 2022

Okay, it does not break anything (at least in an obvious way), and generally, the changes make sense.
Explanation: #43737 (comment)

@tavplubix tavplubix marked this pull request as ready for review November 29, 2022 17:09
@tavplubix
Copy link
Copy Markdown
Member Author

tavplubix commented Nov 29, 2022

Integration tests (release) [3/4] - #41145 (comment)
Stateless tests (tsan, s3 storage) [2/5] - #43809
Stress test (debug) - #41369
Stress test (tsan) - #43810
Stress test (ubsan) - #41218

@antonio2368 antonio2368 self-assigned this Nov 30, 2022
antonio2368
antonio2368 approved these changes Nov 30, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@antonio2368 antonio2368 left a comment

Choose a reason for hiding this comment

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

Makes sense to me, it's more correct for sure.
Found only one questionable place which wasn't covered with this PR:

  • BackgroundShcedulePool calls attachTo inside attachToThreadGroup method and I don't see any detach calls

@tavplubix
Copy link
Copy Markdown
Member Author

tavplubix commented Nov 30, 2022

AFAIK threads are never detached from BackgroundSchedulePool until server shutdown, so I guess it's not an issue. But let's add it for consistency.

@tavplubix
Copy link
Copy Markdown
Member Author

tavplubix commented Nov 30, 2022

AST fuzzer (tsan) - #43204
Stateless tests (debug, s3 storage) [6/6] - it's funny - a bug from 2015 - #43847
Stateless tests (tsan) [4/5] - #43659
Stress test (tsan) - #37664

@tavplubix
Copy link
Copy Markdown
Member Author

tavplubix commented Nov 30, 2022

I hate Mergeable Check (and spot instances)

@tavplubix tavplubix added the skip mergeable check The label to omit checks for required statuses. Does not help with `pr-feature` new docs check label Nov 30, 2022
@tavplubix tavplubix merged commit cd8a407 into master Nov 30, 2022
@tavplubix tavplubix deleted the fix_assertion_in_thread_status branch November 30, 2022 20:53
This was referenced Dec 2, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@antonio2368 antonio2368 antonio2368 approved these changes

Assignees

@antonio2368 antonio2368

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog skip mergeable check The label to omit checks for required statuses. Does not help with `pr-feature` new docs check

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tavplubix @antonio2368 @robot-clickhouse