Commit 92383cf6 authored by Shinya Maeda's avatar Shinya Maeda
Browse files

Introduce `use_fast_destroy` helper for parent associations. Rename method...

Introduce `use_fast_destroy` helper for parent associations.  Rename method names in build_trace_chunks. Forbid `destroy` method when `FastDestroyAll` included.
parent d76355420d50
...@@ -6,7 +6,7 @@ class BuildTraceChunk < ActiveRecord::Base ...@@ -6,7 +6,7 @@ class BuildTraceChunk < ActiveRecord::Base
belongs_to :build, class_name: "Ci::Build", foreign_key: :build_id belongs_to :build, class_name: "Ci::Build", foreign_key: :build_id
default_value_for :data_store, :redis default_value_for :data_store, :redis
fast_destroy_all_with :delete_all_redis_data, :redis_all_data_keys fast_destroy_all_with :redis_delete_data, :redis_data_keys
WriteError = Class.new(StandardError) WriteError = Class.new(StandardError)
...@@ -26,17 +26,17 @@ def redis_data_key(build_id, chunk_index) ...@@ -26,17 +26,17 @@ def redis_data_key(build_id, chunk_index)
"gitlab:ci:trace:#{build_id}:chunks:#{chunk_index}" "gitlab:ci:trace:#{build_id}:chunks:#{chunk_index}"
end end
def redis_all_data_keys def redis_data_keys
redis.pluck(:build_id, :chunk_index).map do |data| redis.pluck(:build_id, :chunk_index).map do |data|
redis_data_key(data.first, data.second) redis_data_key(data.first, data.second)
end end
end end
def delete_all_redis_data(redis_keys) def redis_delete_data(keys)
if redis_keys.any? return if keys.empty?
Gitlab::Redis::SharedState.with do |redis|
redis.del(redis_keys) Gitlab::Redis::SharedState.with do |redis|
end redis.del(keys)
end end
end end
end end
...@@ -80,7 +80,8 @@ def use_database! ...@@ -80,7 +80,8 @@ def use_database!
break unless size > 0 break unless size > 0
self.update!(raw_data: data, data_store: :db) self.update!(raw_data: data, data_store: :db)
redis_delete_data key = self.class.redis_data_key(build_id, chunk_index)
self.class.redis_delete_data([key])
end end
end end
...@@ -138,12 +139,6 @@ def redis_set_data(data) ...@@ -138,12 +139,6 @@ def redis_set_data(data)
end end
end end
def redis_delete_data
Gitlab::Redis::SharedState.with do |redis|
redis.del(self.class.redis_data_key(build_id, chunk_index))
end
end
def redis_lock_key def redis_lock_key
"trace_write:#{build_id}:chunks:#{chunk_index}" "trace_write:#{build_id}:chunks:#{chunk_index}"
end end
......
...@@ -4,7 +4,7 @@ ...@@ -4,7 +4,7 @@
# In general, `destroy_all` is inefficient because it calls each callback with `DELETE` queries i.e. O(n), whereas, # In general, `destroy_all` is inefficient because it calls each callback with `DELETE` queries i.e. O(n), whereas,
# `delete_all` is efficient as it deletes all rows with a single `DELETE` query. # `delete_all` is efficient as it deletes all rows with a single `DELETE` query.
# #
# It's better to use `delete_all` as much as possible, however, in the real cases, it's hard to adopt because # It's better to use `delete_all` as much as possible, however, in the real cases, it's hard to adopt because
# in some cases, external data (in ObjectStorage/FileStorage/Redis) is assosiated with rows. # in some cases, external data (in ObjectStorage/FileStorage/Redis) is assosiated with rows.
# #
# This module introduces a protocol to support the adoption with easy way. # This module introduces a protocol to support the adoption with easy way.
...@@ -12,12 +12,18 @@ ...@@ -12,12 +12,18 @@
# - When calling `destroy_all` explicitly # - When calling `destroy_all` explicitly
# e.g. `job.trace_chunks.fast_destroy_all`` # e.g. `job.trace_chunks.fast_destroy_all``
# - When a parent record is deleted and all children are deleted subsequently (cascade delete) # - When a parent record is deleted and all children are deleted subsequently (cascade delete)
# e.g. `before_destroy -> { run_after_commit(&build_trace_chunks.delete_all_external_data_proc) }`` # e.g. `before_destroy -> { run_after_commit(&build_trace_chunks.after_commit_cleanup_proc) }``
module FastDestroyAll module FastDestroyAll
extend ActiveSupport::Concern extend ActiveSupport::Concern
ForbiddenActionError = Class.new(StandardError)
included do included do
class_attribute :_delete_method, :_delete_params_generator class_attribute :_delete_method, :_delete_params_generator
before_destroy do
raise ForbiddenActionError, '`destroy` is forbbiden, please use `fast_destroy_all`'
end
end end
class_methods do class_methods do
...@@ -35,13 +41,13 @@ def fast_destroy_all_with(delete_method, delete_params_generator) ...@@ -35,13 +41,13 @@ def fast_destroy_all_with(delete_method, delete_params_generator)
# This method generates a proc to delete external data. # This method generates a proc to delete external data.
# It's useful especially when you want to hook parent record's deletion event. # It's useful especially when you want to hook parent record's deletion event.
# #
# e.g. before_destroy -> { run_after_commit(&build_trace_chunks.delete_all_external_data_proc) } # e.g. before_destroy -> { run_after_commit(&build_trace_chunks.after_commit_cleanup_proc) }
def delete_all_external_data_proc def after_commit_cleanup_proc
params = send self._delete_params_generator params = send self._delete_params_generator # rubocop:disable GitlabSecurity/PublicSend
callee = self # Preserve the callee class, otherwise `proc` uses a different class subject = self # Preserve the subject class, otherwise `proc` uses a different class
proc do proc do
callee.send callee._delete_method, params subject.send subject._delete_method, params # rubocop:disable GitlabSecurity/PublicSend
true true
end end
...@@ -52,10 +58,32 @@ def delete_all_external_data_proc ...@@ -52,10 +58,32 @@ def delete_all_external_data_proc
# Before deleting the rows, it generates a proc to delete external data. # Before deleting the rows, it generates a proc to delete external data.
# So it won't lose the track of deleting external data, even if it happened after rows had been deleted. # So it won't lose the track of deleting external data, even if it happened after rows had been deleted.
def fast_destroy_all def fast_destroy_all
delete_all_external_data_proc.tap do |delete_all_external_data| after_commit_cleanup_proc.tap do |delete_all_external_data|
delete_all delete_all
delete_all_external_data.call delete_all_external_data.call
end end
end end
end end
module Helpers
extend ActiveSupport::Concern
class_methods do
##
# This is a helper method for performaning fast_destroy_all when parent relations are deleted
# Their children must include `FastDestroyAll` module.
#
# `use_fast_destroy` must be defined **before** `has_many` and `has_one`, such as `has_many :relation, depenedent: :destroy`
# Otherwise `use_fast_destroy` performs against **deleted** rows, which fails to get identifiers of external data
#
# e.g. use_fast_destroy :build_trace_chunks
def use_fast_destroy(relation)
before_destroy do
subject = public_send(relation)
after_commit_proc = subject.after_commit_cleanup_proc
run_after_commit(&after_commit_proc)
end
end
end
end
end end
...@@ -22,6 +22,7 @@ class Project < ActiveRecord::Base ...@@ -22,6 +22,7 @@ class Project < ActiveRecord::Base
include DeploymentPlatform include DeploymentPlatform
include ::Gitlab::Utils::StrongMemoize include ::Gitlab::Utils::StrongMemoize
include ChronicDurationAttribute include ChronicDurationAttribute
include FastDestroyAll::Helpers
extend Gitlab::ConfigHelper extend Gitlab::ConfigHelper
...@@ -79,10 +80,10 @@ class Project < ActiveRecord::Base ...@@ -79,10 +80,10 @@ class Project < ActiveRecord::Base
before_destroy :remove_private_deploy_keys before_destroy :remove_private_deploy_keys
# This has to be defined before `has_many :builds, depenedent: :destroy`, ##
# otherwise we will not delete any data, due to trace chunks # `use_fast_destroy` must be defined **before** `has_many` and `has_one` such as `has_many :relation, depenedent: :destroy`
# going through :builds # Otherwise `use_fast_destroy` performs against **deleted** rows, which fails to get identifiers of external data
before_destroy -> { run_after_commit(&build_trace_chunks.delete_all_external_data_proc) } use_fast_destroy :build_trace_chunks
after_destroy -> { run_after_commit { remove_pages } } after_destroy -> { run_after_commit { remove_pages } }
after_destroy :remove_exports after_destroy :remove_exports
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment