-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
Consider lightweight deleted rows when selecting parts to merge #57648
Consider lightweight deleted rows when selecting parts to merge #57648
Conversation
This is an automated comment for commit fd46056 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
|
76bec64
to
ac00f6e
Compare
ac00f6e
to
e34c13b
Compare
@alexey-milovidov Hi, CI checks had all passed, could you please assign a reviewer for this PR? |
Sorry, this has been reverted because another reviewer (not me, I didn't read the code) has found an obvious race condition in the code. Let's fix this race condition and resubmit. |
Also, we have to add a test to make this race condition exposed. |
@alexey-milovidov could you please specify what race condition we have here? do we have a note somewhere I missed? |
Also, grabbing a mutex in |
@davenger saving count is definitely better way of dealing with this but it requires metadata modification one way or another. |
@davenger Actually my initial thought was to add an |
@alexey-milovidov @yakov-olkhovskiy what are your thoughts about this approach? |
I think it's better than lazy calculation, but backward compatibility should be taken into account. |
@alexey-milovidov Could you please provide more information about the race condition in this PR? |
I don't know what the race condition is, but my colleagues told me that it is there... I will clarify. |
@alexey-milovidov thanks |
They said it was around |
I don't see any race conditions here... @yakov-olkhovskiy Do you have any clue? |
@jewelzqiu, My colleagues said it is related to |
@alexey-milovidov Thanks, I got it, looks like |
resolves #56728
Changelog category:
Changelog entry:
Consider lightweight deleted rows when selecting parts to merge if enabled