Let destroy old job instances in batch #108
Conversation
to reduce the effect which estimated on production service
bin/cleanup_old_instances.rb
Outdated
| old_instances.destroy_all | ||
| .order(id: :asc) | ||
| .in_batches do |old_instances| | ||
| count += old_instances.count |
There was a problem hiding this comment.
in_batches returns ActiveRecord::Relation, so I think count is better, isn't it?
There was a problem hiding this comment.
ah yes correct; the following lines just run DELETE statement. Please disregard
There was a problem hiding this comment.
💭 I forgot how Rails destroy_all behaves... if it tries to load records, .size might be better
There was a problem hiding this comment.
If we can reduce a count query by calling size after/before calling destroy_all, using size looks great. Otherwise, we should use count as normal. In addition, we should leave a comment about that behavior.
There was a problem hiding this comment.
in_batches yields loaded active record relation 🙈
so, not to send count query, fix to use .size f59f230
There was a problem hiding this comment.
relation is loaded already
bin/cleanup_old_instances.rb
Outdated
| old_instances.destroy_all | ||
| .order(id: :asc) | ||
| .in_batches do |old_instances| | ||
| count += old_instances.count |
There was a problem hiding this comment.
If we can reduce a count query by calling size after/before calling destroy_all, using size looks great. Otherwise, we should use count as normal. In addition, we should leave a comment about that behavior.
riseshia
force-pushed
the
let-handle-many
branch
from
4998c32 to
f59f230
Compare
|
ci for ruby 2.5 is failing, this is not a problem of the project, but from outside... |
| count += old_instances.size | ||
| Kuroko2::JobInstance.transaction do | ||
| old_instances.destroy_all | ||
| end |
There was a problem hiding this comment.
which situation are you worried? 🤔
There was a problem hiding this comment.
first, destroy_all is just call destroy to each instance.
https://github.com/rails/rails/blob/008538081b87940ad1a4fd29df730ec5d0421ee5/activerecord/lib/active_record/relation.rb#L379-L381
second, destroy seems to use the transaction which includes only itself.
https://github.com/rails/rails/blob/6aa5cf03ea8232180ffbbae4c130b051f813c670/activerecord/lib/active_record/persistence.rb#L336-L347
Based on these, we might have 2 options:
- Just unwrap
Kuroko2::JobInstance.transaction old_instances.each { |i| Kuroko2::JobInstance.transaction { i.destroy } }- I'm doubt to do it worth 🤔
how do you think? @eisuke
There was a problem hiding this comment.
I think that "Just unwrap Kuroko2::JobInstance.transaction" is good.
There was a problem hiding this comment.
#109 Ready for review XD
Follow up of #107
#107 script will affect another batch execution if it has old job instances a lot.
To reduce the effect estimated on production service, let the deletion process be worked in batch(per 1000).
@cookpad/dev-infra please review this :)