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

Compute asset summary asynchronously for draft versions #1431

Merged
merged 1 commit into from
Jan 30, 2023

Conversation

danlamanna
Copy link
Contributor

For some background: modifying assets requires modifying the draft version to update the asset summary. This creates write contention on the version when assets that belong to the same version are being modified concurrently. The problem is complicated because the asset summary can take several seconds on larger dandisets and the time grows linearly with the asset count. To date this has only been a problem with our largest dandiset.

This PR moves the asset summary computation to an asynchronous task, similar to version metadata validation.

@mvandenburgh Can you take a look at this PR closely since it touches some of your recently modified publishing code?

This is a draft for right now, I want to look closer into the version lifecycle before I merge.

Copy link
Member

@mvandenburgh mvandenburgh left a comment

Choose a reason for hiding this comment

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

I don't see any issues here, looks good. At some point we'll also need to update the frontend to indicate when assets summary is being calculated.

@waxlamp
Copy link
Member

waxlamp commented Jan 18, 2023

I don't see any issues here, looks good. At some point we'll also need to update the frontend to indicate when assets summary is being calculated.

Ideally we'd add that change to this PR, but (1) it's a separate (but intimately related) change and (2) I wouldn't want to hold up merging this PR, but it's important that the frontend changes are done soon after if not alongside this PR.

@mvandenburgh, I'd like to be involved with designing the frontend changes. Please schedule a meeting with me and/or Dan to discuss.

@danlamanna danlamanna force-pushed the asynchronous-asset-summaries branch 3 times, most recently from e3c9474 to 64d6639 Compare January 23, 2023 19:20
@danlamanna danlamanna marked this pull request as ready for review January 23, 2023 20:04
@waxlamp
Copy link
Member

waxlamp commented Jan 27, 2023

@danlamanna, what's the current status of this PR? You had wanted to hold off before merging, but I see the PR is out of draft mode now.

@mvandenburgh, did you have a chance to do a full review? (You left a comment-review rather than an approval, so just checking in with your status on this PR as well.)

@mvandenburgh
Copy link
Member

@waxlamp we discussed this PR earlier this week, and Dan made some changes based on that, he's just waiting on my review now (which I will do today).

@dandibot
Copy link
Member

🚀 PR was released in v0.3.14 🚀

@dandibot dandibot added the released This issue/pull request has been released. label Feb 10, 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.

4 participants