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

Offer dependent: :destroy_async for associations #40157

Merged

Conversation

adrianna-chang-shopify
Copy link
Contributor

@adrianna-chang-shopify adrianna-chang-shopify commented Sep 1, 2020

Revised version of #36912, #39149

Summary

Allows associations taking the dependent key to specify dependent: :destroy_async, which will enqueue a job to destroy associations asynchronously.

cc @rafaelfranca - I've removed ActiveRecord::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.

@adrianna-chang-shopify adrianna-chang-shopify force-pushed the dependent-destroy-async branch 2 times, most recently from aee646b to 66c4747 Compare Sep 21, 2020
@adrianna-chang-shopify adrianna-chang-shopify marked this pull request as ready for review Sep 21, 2020
activerecord/CHANGELOG.md Outdated Show resolved Hide resolved
activerecord/CHANGELOG.md Outdated Show resolved Hide resolved
@adrianna-chang-shopify adrianna-chang-shopify force-pushed the dependent-destroy-async branch 3 times, most recently from ee96146 to c112d8e Compare Sep 24, 2020
@adrianna-chang-shopify adrianna-chang-shopify changed the title Offer dependent: :destroy_later for associations Offer dependent: :destroy_async for associations Sep 24, 2020
Sometimes cascading association deletions can cause timeouts due to
an IO issue. Perhaps a model has associations that are destroyed on
deletion which in turn trigger other deletions and this can continue
down a complex tree. Along this tree you may also hit other IO
operations. Such deep deletions can lead to server timeouts while
awaiting completion and really the user may not notice all the
changes on their side immediately making them wait unnecesarially or
worse 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

@kylefox
Copy link

kylefox commented Oct 5, 2020

This is awesome 👍

But just curious: why not use dependent: :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?

valid += [:as, :foreign_type] if options[:as]
valid += [:through, :source, :source_type] if options[:through]
valid += [:ensuring_owner_was] if options[: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
Contributor Author

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 ❤️

@@ -7,14 +7,15 @@ def self.macro
end

def self.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

kylefox commented Nov 19, 2020

@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 mentioned here. 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. 💕

@rafaelfranca
Copy link
Member

rafaelfranca commented Nov 19, 2020

The reason for the different name if because we wanted to keep the destroy_later name for 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 of dependent: :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 the destroy_later class level feature, was enough to not use the _later suffix.

gwincr11 added a commit to gwincr11/rails that referenced this pull request Dec 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#36912

Changes:
  - Add a destroy_async method to persistence.
  - Add a rail tie to setup the active job configs.
  - Add a default destroy async job.
  - Test
@gwincr11 gwincr11 mentioned this pull request Dec 27, 2020
gwincr11 added a commit to gwincr11/rails that referenced this pull request Dec 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#36912

Changes:
  - Add a destroy_async method to persistence.
  - Add a rail tie to setup the active job configs.
  - Add a default destroy async job.
  - Test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants