Commit 6d36c3b5 authored by Kamil Trzciński's avatar Kamil Trzciński

Merge branch 'ci-pipeline-status-query' into 'master'

Optimise getting the pipeline status of commits

See merge request gitlab-org/gitlab-ce!15332
parents 69dd59ae ab16a6fb
...@@ -57,6 +57,7 @@ class Projects::CommitsController < Projects::ApplicationController ...@@ -57,6 +57,7 @@ class Projects::CommitsController < Projects::ApplicationController
@repository.commits(@ref, path: @path, limit: @limit, offset: @offset) @repository.commits(@ref, path: @path, limit: @limit, offset: @offset)
end end
@commits = @commits.with_pipeline_status
@commits = prepare_commits_for_rendering(@commits) @commits = prepare_commits_for_rendering(@commits)
end end
end end
...@@ -80,7 +80,8 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo ...@@ -80,7 +80,8 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
def commits def commits
# Get commits from repository # Get commits from repository
# or from cache if already merged # or from cache if already merged
@commits = prepare_commits_for_rendering(@merge_request.commits) @commits =
prepare_commits_for_rendering(@merge_request.commits.with_pipeline_status)
render json: { html: view_to_html_string('projects/merge_requests/_commits') } render json: { html: view_to_html_string('projects/merge_requests/_commits') }
end end
......
...@@ -149,34 +149,70 @@ module Ci ...@@ -149,34 +149,70 @@ module Ci
end end
end end
# ref can't be HEAD or SHA, can only be branch/tag name
scope :latest, ->(ref = nil) do
max_id = unscope(:select)
.select("max(#{quoted_table_name}.id)")
.group(:ref, :sha)
if ref
where(ref: ref, id: max_id.where(ref: ref))
else
where(id: max_id)
end
end
scope :internal, -> { where(source: internal_sources) } scope :internal, -> { where(source: internal_sources) }
# Returns the pipelines in descending order (= newest first), optionally
# limited to a number of references.
#
# ref - The name (or names) of the branch(es)/tag(s) to limit the list of
# pipelines to.
def self.newest_first(ref = nil)
relation = order(id: :desc)
ref ? relation.where(ref: ref) : relation
end
def self.latest_status(ref = nil) def self.latest_status(ref = nil)
latest(ref).status newest_first(ref).pluck(:status).first
end end
def self.latest_successful_for(ref) def self.latest_successful_for(ref)
success.latest(ref).order(id: :desc).first newest_first(ref).success.take
end end
def self.latest_successful_for_refs(refs) def self.latest_successful_for_refs(refs)
success.latest(refs).order(id: :desc).each_with_object({}) do |pipeline, hash| relation = newest_first(refs).success
relation.each_with_object({}) do |pipeline, hash|
hash[pipeline.ref] ||= pipeline hash[pipeline.ref] ||= pipeline
end end
end end
# Returns a Hash containing the latest pipeline status for every given
# commit.
#
# The keys of this Hash are the commit SHAs, the values the statuses.
#
# commits - The list of commit SHAs to get the status for.
# ref - The ref to scope the data to (e.g. "master"). If the ref is not
# given we simply get the latest status for the commits, regardless
# of what refs their pipelines belong to.
def self.latest_status_per_commit(commits, ref = nil)
p1 = arel_table
p2 = arel_table.alias
# This LEFT JOIN will filter out all but the newest row for every
# combination of (project_id, sha) or (project_id, sha, ref) if a ref is
# given.
cond = p1[:sha].eq(p2[:sha])
.and(p1[:project_id].eq(p2[:project_id]))
.and(p1[:id].lt(p2[:id]))
cond = cond.and(p1[:ref].eq(p2[:ref])) if ref
join = p1.join(p2, Arel::Nodes::OuterJoin).on(cond)
relation = select(:sha, :status)
.where(sha: commits)
.where(p2[:id].eq(nil))
.joins(join.join_sources)
relation = relation.where(ref: ref) if ref
relation.each_with_object({}) do |row, hash|
hash[row[:sha]] = row[:status]
end
end
def self.truncate_sha(sha) def self.truncate_sha(sha)
sha[0...8] sha[0...8]
end end
......
...@@ -80,6 +80,7 @@ class Commit ...@@ -80,6 +80,7 @@ class Commit
@raw = raw_commit @raw = raw_commit
@project = project @project = project
@statuses = {}
end end
def id def id
...@@ -236,11 +237,13 @@ class Commit ...@@ -236,11 +237,13 @@ class Commit
end end
def status(ref = nil) def status(ref = nil)
@statuses ||= {}
return @statuses[ref] if @statuses.key?(ref) return @statuses[ref] if @statuses.key?(ref)
@statuses[ref] = pipelines.latest_status(ref) @statuses[ref] = project.pipelines.latest_status_per_commit(id, ref)[id]
end
def set_status_for_ref(ref, status)
@statuses[ref] = status
end end
def signature def signature
......
# frozen_string_literal: true
# A collection of Commit instances for a specific project and Git reference.
class CommitCollection
include Enumerable
attr_reader :project, :ref, :commits
# project - The project the commits belong to.
# commits - The Commit instances to store.
# ref - The name of the ref (e.g. "master").
def initialize(project, commits, ref = nil)
@project = project
@commits = commits
@ref = ref
end
def each(&block)
commits.each(&block)
end
# Sets the pipeline status for every commit.
#
# Setting this status ahead of time removes the need for running a query for
# every commit we're displaying.
def with_pipeline_status
statuses = project.pipelines.latest_status_per_commit(map(&:id), ref)
each do |commit|
commit.set_status_for_ref(ref, statuses[commit.id])
end
self
end
def respond_to_missing?(message, inc_private = false)
commits.respond_to?(message, inc_private)
end
# rubocop:disable GitlabSecurity/PublicSend
def method_missing(message, *args, &block)
commits.public_send(message, *args, &block)
end
end
...@@ -284,8 +284,10 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -284,8 +284,10 @@ class MergeRequestDiff < ActiveRecord::Base
def load_commits def load_commits
commits = st_commits.presence || merge_request_diff_commits commits = st_commits.presence || merge_request_diff_commits
commits = commits.map { |commit| Commit.from_hash(commit.to_hash, project) }
commits.map { |commit| Commit.from_hash(commit.to_hash, project) } CommitCollection
.new(merge_request.source_project, commits, merge_request.source_branch)
end end
def save_diffs def save_diffs
......
...@@ -132,7 +132,8 @@ class Repository ...@@ -132,7 +132,8 @@ class Repository
commits = Gitlab::Git::Commit.where(options) commits = Gitlab::Git::Commit.where(options)
commits = Commit.decorate(commits, @project) if commits.present? commits = Commit.decorate(commits, @project) if commits.present?
commits
CommitCollection.new(project, commits, ref)
end end
def commits_between(from, to) def commits_between(from, to)
...@@ -148,11 +149,14 @@ class Repository ...@@ -148,11 +149,14 @@ class Repository
end end
raw_repository.gitaly_migrate(:commits_by_message) do |is_enabled| raw_repository.gitaly_migrate(:commits_by_message) do |is_enabled|
if is_enabled commits =
find_commits_by_message_by_gitaly(query, ref, path, limit, offset) if is_enabled
else find_commits_by_message_by_gitaly(query, ref, path, limit, offset)
find_commits_by_message_by_shelling_out(query, ref, path, limit, offset) else
end find_commits_by_message_by_shelling_out(query, ref, path, limit, offset)
end
CommitCollection.new(project, commits, ref)
end end
end end
......
---
title: Optimise getting the pipeline status of commits
merge_request:
author:
type: performance
...@@ -625,38 +625,29 @@ describe Ci::Pipeline, :mailer do ...@@ -625,38 +625,29 @@ describe Ci::Pipeline, :mailer do
shared_context 'with some outdated pipelines' do shared_context 'with some outdated pipelines' do
before do before do
create_pipeline(:canceled, 'ref', 'A') create_pipeline(:canceled, 'ref', 'A', project)
create_pipeline(:success, 'ref', 'A') create_pipeline(:success, 'ref', 'A', project)
create_pipeline(:failed, 'ref', 'B') create_pipeline(:failed, 'ref', 'B', project)
create_pipeline(:skipped, 'feature', 'C') create_pipeline(:skipped, 'feature', 'C', project)
end end
def create_pipeline(status, ref, sha) def create_pipeline(status, ref, sha, project)
create(:ci_empty_pipeline, status: status, ref: ref, sha: sha) create(
:ci_empty_pipeline,
status: status,
ref: ref,
sha: sha,
project: project
)
end end
end end
describe '.latest' do describe '.newest_first' do
include_context 'with some outdated pipelines' include_context 'with some outdated pipelines'
context 'when no ref is specified' do it 'returns the pipelines from new to old' do
let(:pipelines) { described_class.latest.all } expect(described_class.newest_first.pluck(:status))
.to eq(%w[skipped failed success canceled])
it 'returns the latest pipeline for the same ref and different sha' do
expect(pipelines.map(&:sha)).to contain_exactly('A', 'B', 'C')
expect(pipelines.map(&:status))
.to contain_exactly('success', 'failed', 'skipped')
end
end
context 'when ref is specified' do
let(:pipelines) { described_class.latest('ref').all }
it 'returns the latest pipeline for ref and different sha' do
expect(pipelines.map(&:sha)).to contain_exactly('A', 'B')
expect(pipelines.map(&:status))
.to contain_exactly('success', 'failed')
end
end end
end end
...@@ -664,20 +655,14 @@ describe Ci::Pipeline, :mailer do ...@@ -664,20 +655,14 @@ describe Ci::Pipeline, :mailer do
include_context 'with some outdated pipelines' include_context 'with some outdated pipelines'
context 'when no ref is specified' do context 'when no ref is specified' do
let(:latest_status) { described_class.latest_status } it 'returns the status of the latest pipeline' do
expect(described_class.latest_status).to eq('skipped')
it 'returns the latest status for the same ref and different sha' do
expect(latest_status).to eq(described_class.latest.status)
expect(latest_status).to eq('failed')
end end
end end
context 'when ref is specified' do context 'when ref is specified' do
let(:latest_status) { described_class.latest_status('ref') } it 'returns the status of the latest pipeline for the given ref' do
expect(described_class.latest_status('ref')).to eq('failed')
it 'returns the latest status for ref and different sha' do
expect(latest_status).to eq(described_class.latest_status('ref'))
expect(latest_status).to eq('failed')
end end
end end
end end
...@@ -686,7 +671,7 @@ describe Ci::Pipeline, :mailer do ...@@ -686,7 +671,7 @@ describe Ci::Pipeline, :mailer do
include_context 'with some outdated pipelines' include_context 'with some outdated pipelines'
let!(:latest_successful_pipeline) do let!(:latest_successful_pipeline) do
create_pipeline(:success, 'ref', 'D') create_pipeline(:success, 'ref', 'D', project)
end end
it 'returns the latest successful pipeline' do it 'returns the latest successful pipeline' do
...@@ -698,8 +683,13 @@ describe Ci::Pipeline, :mailer do ...@@ -698,8 +683,13 @@ describe Ci::Pipeline, :mailer do
describe '.latest_successful_for_refs' do describe '.latest_successful_for_refs' do
include_context 'with some outdated pipelines' include_context 'with some outdated pipelines'
let!(:latest_successful_pipeline1) { create_pipeline(:success, 'ref1', 'D') } let!(:latest_successful_pipeline1) do
let!(:latest_successful_pipeline2) { create_pipeline(:success, 'ref2', 'D') } create_pipeline(:success, 'ref1', 'D', project)
end
let!(:latest_successful_pipeline2) do
create_pipeline(:success, 'ref2', 'D', project)
end
it 'returns the latest successful pipeline for both refs' do it 'returns the latest successful pipeline for both refs' do
refs = %w(ref1 ref2 ref3) refs = %w(ref1 ref2 ref3)
...@@ -708,6 +698,62 @@ describe Ci::Pipeline, :mailer do ...@@ -708,6 +698,62 @@ describe Ci::Pipeline, :mailer do
end end
end end
describe '.latest_status_per_commit' do
let(:project) { create(:project) }
before do
pairs = [
%w[success ref1 123],
%w[manual master 123],
%w[failed ref 456]
]
pairs.each do |(status, ref, sha)|
create(
:ci_empty_pipeline,
status: status,
ref: ref,
sha: sha,
project: project
)
end
end
context 'without a ref' do
it 'returns a Hash containing the latest status per commit for all refs' do
expect(described_class.latest_status_per_commit(%w[123 456]))
.to eq({ '123' => 'manual', '456' => 'failed' })
end
it 'only includes the status of the given commit SHAs' do
expect(described_class.latest_status_per_commit(%w[123]))
.to eq({ '123' => 'manual' })
end
context 'when there are two pipelines for a ref and SHA' do
it 'returns the status of the latest pipeline' do
create(
:ci_empty_pipeline,
status: 'failed',
ref: 'master',
sha: '123',
project: project
)
expect(described_class.latest_status_per_commit(%w[123]))
.to eq({ '123' => 'failed' })
end
end
end
context 'with a ref' do
it 'only includes the pipelines for the given ref' do
expect(described_class.latest_status_per_commit(%w[123 456], 'master'))
.to eq({ '123' => 'manual' })
end
end
end
describe '.internal_sources' do describe '.internal_sources' do
subject { described_class.internal_sources } subject { described_class.internal_sources }
......
require 'spec_helper'
describe CommitCollection do
let(:project) { create(:project, :repository) }
let(:commit) { project.commit }
describe '#each' do
it 'yields every commit' do
collection = described_class.new(project, [commit])
expect { |b| collection.each(&b) }.to yield_with_args(commit)
end
end
describe '#with_pipeline_status' do
it 'sets the pipeline status for every commit so no additional queries are necessary' do
create(
:ci_empty_pipeline,
ref: 'master',
sha: commit.id,
status: 'success',
project: project
)
collection = described_class.new(project, [commit])
collection.with_pipeline_status
recorder = ActiveRecord::QueryRecorder.new do
expect(commit.status).to eq('success')
end
expect(recorder.count).to be_zero
end
end
describe '#respond_to_missing?' do
it 'returns true when the underlying Array responds to the message' do
collection = described_class.new(project, [])
expect(collection.respond_to?(:last)).to eq(true)
end
it 'returns false when the underlying Array does not respond to the message' do
collection = described_class.new(project, [])
expect(collection.respond_to?(:foo)).to eq(false)
end
end
describe '#method_missing' do
it 'delegates undefined methods to the underlying Array' do
collection = described_class.new(project, [commit])
expect(collection.length).to eq(1)
expect(collection.last).to eq(commit)
expect(collection).not_to be_empty
end
end
end
...@@ -351,12 +351,19 @@ eos ...@@ -351,12 +351,19 @@ eos
end end
it 'gives compound status from latest pipelines if ref is nil' do it 'gives compound status from latest pipelines if ref is nil' do
expect(commit.status(nil)).to eq(Ci::Pipeline.latest_status) expect(commit.status(nil)).to eq(pipeline_from_fix.status)
expect(commit.status(nil)).to eq('failed')
end end
end end
end end
describe '#set_status_for_ref' do
it 'sets the status for a given reference' do
commit.set_status_for_ref('master', 'failed')
expect(commit.status('master')).to eq('failed')
end
end
describe '#participants' do describe '#participants' do
let(:user1) { build(:user) } let(:user1) { build(:user) }
let(:user2) { build(:user) } let(:user2) { build(:user) }
......
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