Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Offer dependent: :destroy_async for associations#40157

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@adrianna-chang-shopify
Copy link
Contributor

@adrianna-chang-shopifyadrianna-chang-shopify commentedSep 1, 2020
edited
Loading

Revised version of#36912,#39149

Summary

Allows associations taking thedependent key to specifydependent: :destroy_async, which will enqueue a job to destroy associations asynchronously.

cc@rafaelfranca - I've removedActiveRecord::Base.destroy_later from this PR, so it only contains the implementation of the asynchronous association deletion. Will rebase + squash if it looks okay to you.

elle, ryoung, imageaid, harrylewis, fuerstenberg, kidlab, blanks88, and fkoosan reacted with thumbs up emojihahmed, zaratan, sauloperez, toddsiegel, saiqulhaq, ecleel, tobidelius, Martouta, lmumar, geoffharcourt, and 22 more reacted with heart emojiignacio-chiazzo reacted with rocket emoji
@adrianna-chang-shopifyadrianna-chang-shopifyforce-pushed thedependent-destroy-async branch 2 times, most recently fromaee646b to66c4747CompareSeptember 21, 2020 13:46
@adrianna-chang-shopifyadrianna-chang-shopify marked this pull request as ready for reviewSeptember 21, 2020 13:56
@adrianna-chang-shopifyadrianna-chang-shopifyforce-pushed thedependent-destroy-async branch 3 times, most recently fromee96146 toc112d8eCompareSeptember 24, 2020 12:57
@adrianna-chang-shopifyadrianna-chang-shopify changed the titleOffer dependent: :destroy_later for associationsOffer dependent: :destroy_async for associationsSep 24, 2020
Sometimes cascading association deletions can cause timeouts due toan IO issue. Perhaps a model has associations that are destroyed ondeletion which in turn trigger other deletions and this can continuedown a complex tree. Along this tree you may also hit other IOoperations. Such deep deletions can lead to server timeouts whileawaiting completion and really the user may not notice all thechanges on their side immediately making them wait unnecesarially orworse causing a timeout during the operation.We now allow associations supporting the `dependent:` key to take `:destroy_async`,which schedules a background job to destroy associations.Co-authored-by: Adrianna Chang <adrianna.chang@shopify.com>Co-authored-by: Rafael Mendonça França <rafael@franca.dev>Co-authored-by: Cory Gwin@gwincr11 <gwincr11@github.com>
@gwincr11
Copy link
Contributor

adrianna-chang-shopify, sinsoku, pkayokay, thealiilman, and ryoung reacted with heart emojiadrianna-chang-shopify, pkayokay, shivamvinsol, geoffharcourt, katestud, and ryoung reacted with rocket emoji

@kylefox
Copy link

This is awesome 👍

But just curious: why not usedependent: :destroy_later?

That seems more consistent with other delayed methods:purge_later,deliver_later,perform_later, etc.

Is there something about the behavior that makes:destroy_async more appropriate than:destroy_later?

jrochkind, avokhmin, datanoise, kingink, lapitsky, rainerborene, commitshappen, qinmingyuan, andreimoment, ledowong, and 14 more reacted with thumbs up emoji

valid=super +[:counter_cache,:join_table,:index_errors,:ensuring_owner_was]
valid +=[:as,:foreign_type]ifoptions[:as]
valid +=[:through,:source,:source_type]ifoptions[:through]
valid +=[:ensuring_owner_was]ifoptions[:dependent] ==:destroy_async
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't it a duplicate with the addition 3 lines above?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, good catch@vendethiel ! Feel free to open a PR ❤️


defself.valid_options(options)
valid=super +[:counter_cache,:join_table,:index_errors]
valid=super +[:counter_cache,:join_table,:index_errors,:ensuring_owner_was]
Copy link
Contributor

Choose a reason for hiding this comment

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

As@vendethiel said, here the key is always included, which invalidates line 13, no?

Copy link
Member

Choose a reason for hiding this comment

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

As@adrianna-chang-shopify said, feel free to open a PR 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

@kylefox
Copy link

@rafaelfranca@adrianna-chang-shopify I don't want to be a pest 😅 but I think it would be useful for the community to understand why this option is named:destroy_async vs:destroy_later as mentionedhere. If there's something about this behavior that makes it different from the rest of the*_later functionality built into Rails, it would be useful to know why it's different. 💕

mejibyte, adaam2, Samuelodan, baonguyen1320, and davidalejandroaguilar reacted with thumbs up emoji

@rafaelfranca
Copy link
Member

rafaelfranca commentedNov 19, 2020
edited
Loading

The reason for the different name if because we wanted to keep thedestroy_later namefor the class level feature that we wanted to introduce. In that feature, we cared when exactly the destroy happened.

For the feature implemented here we don't care if the destroy happen in 1s or in 10 minutes. It just need to happen async, without blocking the current request.

So the behavior ofdependent: :destroy_async is similar to_later methods, the only difference is that you can't configure when it will happen. That, and we wanting to keep the concept different from thedestroy_later class level feature, was enough to not use the_later suffix.

wbotelhos, gwincr11, ryoung, tatsuyafw, LucasGG, roseliux, AlexanderUlitin, brandoncc, khrisnagunanasurya, ibnutoriq, and 2 more reacted with thumbs up emoji

@gwincr11gwincr11 mentioned this pull requestDec 27, 2020
gwincr11 added a commit to gwincr11/rails that referenced this pull requestDec 27, 2020
Motivation:  - Now that destroy_async is an option on association, a natural next    step seems to be adding this method to standard objects for a more    consistent offering.Related Issues:  -rails#40157  -rails#36912Changes:  - Add a destroy_async method to persistence.  - Add a rail tie to setup the active job configs.  - Add a default destroy async job.  - Test
@tomrossi7
Copy link
Contributor

Would a PR to adddependent: :delete_async be beneficial to others? We have some cases where we would prefer to usedelete rather thandestroy.

jarthod and plantoeat reacted with thumbs up emoji

@jarthod
Copy link

jarthod commentedOct 31, 2023
edited
Loading

@tomrossi7 👍 on:delete_async for me, I do have a case where I would need this (don't need the slower:destroy because there's no callbacks or dependencies, yet:delete is still too slow to do synchronously due to database fragmentation)

And also thanks for this:destroy_async feature in Rails, it's a great addition 🎉

gwincr11 reacted with heart emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@eugeneiuseugeneiuseugeneius left review comments

@rafaelfrancarafaelfrancaAwaiting requested review from rafaelfranca

+2 more reviewers

@wbotelhoswbotelhoswbotelhos left review comments

@vendethielvendethielvendethiel left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@adrianna-chang-shopify@gwincr11@kylefox@rafaelfranca@tomrossi7@jarthod@wbotelhos@vendethiel@eugeneius@georgeclaghorn

[8]ページ先頭

©2009-2025 Movatter.jp