From 5c025a89c8572fbe389c5673d4d6b9d29599351c Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Mon, 21 May 2018 13:31:21 -0300 Subject: Clean-up phase for MR metrics population background migration This finishes any old running background migration which populates MR metrics with events data. Which solves https://gitlab.com/gitlab-org/gitlab-ce/issues/41587. --- app/serializers/merge_request_widget_entity.rb | 29 +--------- .../41587-osw-mr-metrics-migration-cleanup.yml | 5 ++ ...g_mr_metrics_populating_background_migration.rb | 33 ++++++++++++ db/schema.rb | 2 +- ...metrics_populating_background_migration_spec.rb | 25 +++++++++ .../merge_request_widget_entity_spec.rb | 61 ++-------------------- 6 files changed, 68 insertions(+), 87 deletions(-) create mode 100644 changelogs/unreleased/41587-osw-mr-metrics-migration-cleanup.yml create mode 100644 db/migrate/20180521162137_migrate_remaining_mr_metrics_populating_background_migration.rb create mode 100644 spec/migrations/migrate_remaining_mr_metrics_populating_background_migration_spec.rb diff --git a/app/serializers/merge_request_widget_entity.rb b/app/serializers/merge_request_widget_entity.rb index d0165c148eb..6aa99850e59 100644 --- a/app/serializers/merge_request_widget_entity.rb +++ b/app/serializers/merge_request_widget_entity.rb @@ -19,11 +19,7 @@ class MergeRequestWidgetEntity < IssuableEntity merge_request.project.merge_requests_ff_only_enabled end - expose :metrics do |merge_request| - metrics = build_metrics(merge_request) - - MergeRequestMetricsEntity.new(metrics).as_json - end + expose :metrics, with: MergeRequestMetricsEntity expose :rebase_commit_sha expose :rebase_in_progress?, as: :rebase_in_progress @@ -222,27 +218,4 @@ class MergeRequestWidgetEntity < IssuableEntity @presenters ||= {} @presenters[merge_request] ||= MergeRequestPresenter.new(merge_request, current_user: current_user) end - - # Once SchedulePopulateMergeRequestMetricsWithEventsData fully runs, - # we can remove this method and just serialize MergeRequest#metrics - # instead. See https://gitlab.com/gitlab-org/gitlab-ce/issues/41587 - def build_metrics(merge_request) - # There's no need to query and serialize metrics data for merge requests that are not - # merged or closed. - return unless merge_request.merged? || merge_request.closed? - return merge_request.metrics if merge_request.merged? && merge_request.metrics&.merged_by_id - return merge_request.metrics if merge_request.closed? && merge_request.metrics&.latest_closed_by_id - - build_metrics_from_events(merge_request) - end - - def build_metrics_from_events(merge_request) - closed_event = merge_request.closed_event - merge_event = merge_request.merge_event - - MergeRequest::Metrics.new(latest_closed_at: closed_event&.updated_at, - latest_closed_by: closed_event&.author, - merged_at: merge_event&.updated_at, - merged_by: merge_event&.author) - end end diff --git a/changelogs/unreleased/41587-osw-mr-metrics-migration-cleanup.yml b/changelogs/unreleased/41587-osw-mr-metrics-migration-cleanup.yml new file mode 100644 index 00000000000..f8640e46e78 --- /dev/null +++ b/changelogs/unreleased/41587-osw-mr-metrics-migration-cleanup.yml @@ -0,0 +1,5 @@ +--- +title: Clean-up phase for MR metrics population background migration +merge_request: +author: +type: other diff --git a/db/migrate/20180521162137_migrate_remaining_mr_metrics_populating_background_migration.rb b/db/migrate/20180521162137_migrate_remaining_mr_metrics_populating_background_migration.rb new file mode 100644 index 00000000000..aa617d8425c --- /dev/null +++ b/db/migrate/20180521162137_migrate_remaining_mr_metrics_populating_background_migration.rb @@ -0,0 +1,33 @@ +class MigrateRemainingMrMetricsPopulatingBackgroundMigration < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + BATCH_SIZE = 10_000 + + class MergeRequest < ActiveRecord::Base + self.table_name = 'merge_requests' + + include ::EachBatch + end + + def up + Gitlab::BackgroundMigration.steal('PopulateMergeRequestMetricsWithEventsData') + + metrics_not_exists_clause = + <<-SQL.strip_heredoc + NOT EXISTS (SELECT 1 FROM merge_request_metrics + WHERE merge_request_metrics.merge_request_id = merge_requests.id) + SQL + + MergeRequest.where(metrics_not_exists_clause).each_batch(of: BATCH_SIZE) do |relation, index| + range = relation.pluck('MIN(id)', 'MAX(id)').first + + Gitlab::BackgroundMigration::PopulateMergeRequestMetricsWithEventsData + .new + .perform(*range) + end + end + + def down + end +end diff --git a/db/schema.rb b/db/schema.rb index ed29d202f91..b39389bc8b3 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180512061621) do +ActiveRecord::Schema.define(version: 20180521162137) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/spec/migrations/migrate_remaining_mr_metrics_populating_background_migration_spec.rb b/spec/migrations/migrate_remaining_mr_metrics_populating_background_migration_spec.rb new file mode 100644 index 00000000000..b948be35927 --- /dev/null +++ b/spec/migrations/migrate_remaining_mr_metrics_populating_background_migration_spec.rb @@ -0,0 +1,25 @@ +require 'spec_helper' +require Rails.root.join('db', 'migrate', '20180521162137_migrate_remaining_mr_metrics_populating_background_migration.rb') + +describe MigrateRemainingMrMetricsPopulatingBackgroundMigration, :migration, :sidekiq, :redis do + let(:mr_with_event) { create(:merge_request) } + let!(:merged_event) { create(:event, :merged, target: mr_with_event) } + let!(:closed_event) { create(:event, :closed, target: mr_with_event) } + + before do + # Make sure no metrics are created and kept through after_* callbacks. + mr_with_event.metrics.destroy! + end + + # This is mainly an integration test. PopulateMergeRequestMetricsWithEventsData worker + # already has exclusive unit tests. + it 'migrates remaining MRs without metrics' do + migrate! + + mr_with_event.reload + + expect(mr_with_event.metrics).to have_attributes(latest_closed_by_id: closed_event.author_id, + merged_by_id: merged_event.author_id) + expect(mr_with_event.metrics.latest_closed_at.to_s).to eq(closed_event.updated_at.to_s) + end +end diff --git a/spec/serializers/merge_request_widget_entity_spec.rb b/spec/serializers/merge_request_widget_entity_spec.rb index d2072198d83..b591cf6dd4b 100644 --- a/spec/serializers/merge_request_widget_entity_spec.rb +++ b/spec/serializers/merge_request_widget_entity_spec.rb @@ -36,76 +36,21 @@ describe MergeRequestWidgetEntity do end describe 'metrics' do - context 'when metrics record exists with merged data' do - before do - resource.mark_as_merged! - resource.metrics.update!(merged_by: user) - end - - it 'matches merge request metrics schema' do - expect(subject[:metrics].with_indifferent_access) - .to match_schema('entities/merge_request_metrics') - end - - it 'returns values from metrics record' do - expect(subject.dig(:metrics, :merged_by, :id)) - .to eq(resource.metrics.merged_by_id) - end - end - - context 'when metrics record exists with closed data' do - before do - resource.close! - resource.metrics.update!(latest_closed_by: user) - end - + context 'when metrics record exists' do it 'matches merge request metrics schema' do expect(subject[:metrics].with_indifferent_access) .to match_schema('entities/merge_request_metrics') end - - it 'returns values from metrics record' do - expect(subject.dig(:metrics, :closed_by, :id)) - .to eq(resource.metrics.latest_closed_by_id) - end end context 'when metrics does not exists' do before do - resource.mark_as_merged! resource.metrics.destroy! resource.reload end - context 'when events exists' do - let!(:closed_event) { create(:event, :closed, project: project, target: resource) } - let!(:merge_event) { create(:event, :merged, project: project, target: resource) } - - it 'matches merge request metrics schema' do - expect(subject[:metrics].with_indifferent_access) - .to match_schema('entities/merge_request_metrics') - end - - it 'returns values from events record' do - expect(subject.dig(:metrics, :merged_by, :id)) - .to eq(merge_event.author_id) - - expect(subject.dig(:metrics, :closed_by, :id)) - .to eq(closed_event.author_id) - - expect(subject.dig(:metrics, :merged_at).to_s) - .to eq(merge_event.updated_at.to_s) - - expect(subject.dig(:metrics, :closed_at).to_s) - .to eq(closed_event.updated_at.to_s) - end - end - - context 'when events does not exists' do - it 'matches merge request metrics schema' do - expect(subject[:metrics].with_indifferent_access) - .to match_schema('entities/merge_request_metrics') - end + it 'returns nil' do + expect(subject[:metrics]).to be_nil end end end -- cgit v1.2.1