Skip to content
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

Avoid clobbering version metadata when calculating assets summary #1557

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

danlamanna
Copy link
Contributor

Changes to version metadata that came in while version_aggregate_assets_summary is running can be overwritten. This change will only update the assetsSummary if the metadata is the same at save time, otherwise, it's a noop and the summary aggregation is retried.

@danlamanna danlamanna force-pushed the agg-summary-race-condition branch 3 times, most recently from 374b0af to 05c20cc Compare April 10, 2023 19:13
mvandenburgh
mvandenburgh previously approved these changes Apr 10, 2023
@danlamanna
Copy link
Contributor Author

This is dependent on #1638.

@yarikoptic
Copy link
Member

yarikoptic commented Nov 17, 2023

#1638 was merged in July. If this PR is still pertinent -- please re-base/resolve conflicts @danlamanna

@waxlamp
Copy link
Member

waxlamp commented Dec 1, 2023

This was approved but never merged. Was there something blocking it, @danlamanna?

Additionally, it's now out of date, so if this is still a valid thing to merge (I believe it is) let's get it in shape to merge. @danlamanna do you want to do that yourself, or can you delegate to someone else?

@mvandenburgh mvandenburgh dismissed their stale review December 4, 2023 21:17

There's some outstanding issues with this PR (will expand on this in a comment)

@mvandenburgh mvandenburgh marked this pull request as draft December 4, 2023 21:18
@danlamanna
Copy link
Contributor Author

This was approved but never merged. Was there something blocking it, @danlamanna?

Additionally, it's now out of date, so if this is still a valid thing to merge (I believe it is) let's get it in shape to merge. @danlamanna do you want to do that yourself, or can you delegate to someone else?

I had thought this was replacing a Version.save with a Version.update, but it's not. So it's still "broken" in the sense that we're missing the hooks called in .save, but it fixes the potential data loss bug with version changes occurring simultaneously with aggregate summaries, so we should merge it.

@mvandenburgh Did you see something else wrong with it?

@mvandenburgh mvandenburgh marked this pull request as ready for review December 5, 2023 14:19
@mvandenburgh
Copy link
Member

This was approved but never merged. Was there something blocking it, @danlamanna?
Additionally, it's now out of date, so if this is still a valid thing to merge (I believe it is) let's get it in shape to merge. @danlamanna do you want to do that yourself, or can you delegate to someone else?

I had thought this was replacing a Version.save with a Version.update, but it's not. So it's still "broken" in the sense that we're missing the hooks called in .save, but it fixes the potential data loss bug with version changes occurring simultaneously with aggregate summaries, so we should merge it.

@mvandenburgh Did you see something else wrong with it?

Oh, i thought the same thing 🤦

No, I agree this should be merged

@danlamanna danlamanna merged commit 61a35a3 into master Dec 5, 2023
10 checks passed
@danlamanna danlamanna deleted the agg-summary-race-condition branch December 5, 2023 14:48
@dandibot
Copy link
Member

🚀 PR was released in v0.3.67 🚀

@dandibot dandibot added the released This issue/pull request has been released. label Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants