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
Context added TSA #55278
Context added TSA #55278
Conversation
This is an automated comment for commit 0e176fa with description of existing statuses. It's updated for the latest CI running ✅ Click here to open a full report in a separate page Successful checks
|
6d08e28
to
0034e0d
Compare
The direction is good but TBH the
I believe that 2. is quite a special case throughout the codebase, and that a non-generic (= |
|
Additionally: |
After checking your PR in more detail, I think it is okay to merge (after incorporating the feedback).
True, that was a glitch on my end.
In my PR, TSA doesn't care what is inside both new lock classes. It only looks at the interface of the lock class (which is the same as in your PR - the constructor and destructor with annotation). There was a little bit of extra overhead but I don't think it would be significant.
Not sure if there are "a lot" of other places but it is a good to make custom logic around
That was where my PR took a different approach (to reduce complexity) but you are right that it also allowed to evade the custom logic.
I added a comment about SharedLockGuard into the PR. |
1b8f500
to
bcbeac3
Compare
This comment was marked as outdated.
This comment was marked as outdated.
bcbeac3
to
0e176fa
Compare
@rschu1ze can you please check this pull request ? I want to continue with adding TSA coverage for non shared Context, on top of this pull request. |
porting ClickHouse/ClickHouse#55278 on Oct 25
Follow-up to #55121
Changelog category (leave one):