Remove Site Statistic
This approach caused many different problems as we tightened the query execution timeout.
This commit is contained in:
Gabriel Mazetto
parent
deaf3af7e5
commit
fe2e6c6dc0
10 changed files with 27 additions and 224 deletions
|
|
@ -88,9 +88,6 @@ class Project < ActiveRecord::Base
|
|||
|
||||
after_create :create_project_feature, unless: :project_feature
|
||||
|
||||
after_create -> { SiteStatistic.track(STATISTICS_ATTRIBUTE) }
|
||||
before_destroy -> { SiteStatistic.untrack(STATISTICS_ATTRIBUTE) }
|
||||
|
||||
after_create :create_ci_cd_settings,
|
||||
unless: :ci_cd_settings,
|
||||
if: proc { ProjectCiCdSetting.available? }
|
||||
|
|
|
|||
|
|
@ -1,76 +0,0 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
class SiteStatistic < ActiveRecord::Base
|
||||
# prevents the creation of multiple rows
|
||||
default_value_for :id, 1
|
||||
|
||||
COUNTER_ATTRIBUTES = %w(repositories_count).freeze
|
||||
REQUIRED_SCHEMA_VERSION = 20180629153018
|
||||
|
||||
# Tracks specific attribute
|
||||
#
|
||||
# @param [String] raw_attribute must be one of the values listed in COUNTER_ATTRIBUTES
|
||||
def self.track(raw_attribute)
|
||||
with_statistics_available(raw_attribute) do |attribute|
|
||||
SiteStatistic.update_all(["#{attribute} = #{attribute}+1"])
|
||||
end
|
||||
end
|
||||
|
||||
# Untracks specific attribute
|
||||
#
|
||||
# @param [String] raw_attribute must be one of the values listed in COUNTER_ATTRIBUTES
|
||||
def self.untrack(raw_attribute)
|
||||
with_statistics_available(raw_attribute) do |attribute|
|
||||
SiteStatistic.update_all(["#{attribute} = #{attribute}-1 WHERE #{attribute} > 0"])
|
||||
end
|
||||
end
|
||||
|
||||
# Wrapper for track/untrack operations with basic validations and enforced requirements
|
||||
#
|
||||
# @param [String] raw_attribute must be one of the values listed in COUNTER_ATTRIBUTES
|
||||
# @yield [String] attribute quoted to be used inside SQL / Arel query
|
||||
def self.with_statistics_available(raw_attribute)
|
||||
unless raw_attribute.in?(COUNTER_ATTRIBUTES)
|
||||
raise ArgumentError, "Invalid attribute: '#{raw_attribute}' to '#{caller_locations(1, 1)[0].label}' method. " \
|
||||
"Valid attributes are: #{COUNTER_ATTRIBUTES.join(', ')}"
|
||||
end
|
||||
|
||||
return unless available?
|
||||
|
||||
self.fetch # make sure record exists
|
||||
|
||||
attribute = self.connection.quote_column_name(raw_attribute)
|
||||
|
||||
# will be running on its own transaction context
|
||||
yield(attribute)
|
||||
end
|
||||
|
||||
# Returns a site statistic record with tracked information
|
||||
#
|
||||
# @return [SiteStatistic] record with tracked information
|
||||
def self.fetch
|
||||
transaction(requires_new: true) do
|
||||
SiteStatistic.first_or_create!
|
||||
end
|
||||
rescue ActiveRecord::RecordNotUnique
|
||||
retry
|
||||
end
|
||||
|
||||
# Return whether required schema change is available
|
||||
#
|
||||
# This is needed in order to degrade gracefully when testing schema migrations
|
||||
#
|
||||
# @return [Boolean] whether schema is available
|
||||
def self.available?
|
||||
@available_flag ||= ActiveRecord::Migrator.current_version >= REQUIRED_SCHEMA_VERSION
|
||||
end
|
||||
|
||||
# Resets cached column information
|
||||
#
|
||||
# This is called during schema migration specs, in order to reset internal cache state
|
||||
def self.reset_column_information
|
||||
@available_flag = nil
|
||||
|
||||
super
|
||||
end
|
||||
end
|
||||
5
changelogs/unreleased/53778-remove-site-statistics.yml
Normal file
5
changelogs/unreleased/53778-remove-site-statistics.yml
Normal file
|
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
title: Removed Site Statistics optimization as it was causing problems
|
||||
merge_request: 23314
|
||||
author:
|
||||
type: removed
|
||||
22
db/migrate/20181123042307_drop_site_statistics.rb
Normal file
22
db/migrate/20181123042307_drop_site_statistics.rb
Normal file
|
|
@ -0,0 +1,22 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
|
||||
# for more information on how to write migrations for GitLab.
|
||||
|
||||
class DropSiteStatistics < ActiveRecord::Migration[5.0]
|
||||
include Gitlab::Database::MigrationHelpers
|
||||
|
||||
DOWNTIME = false
|
||||
|
||||
def up
|
||||
drop_table :site_statistics
|
||||
end
|
||||
|
||||
def down
|
||||
create_table :site_statistics do |t|
|
||||
t.integer :repositories_count, default: 0, null: false
|
||||
end
|
||||
|
||||
execute('INSERT INTO site_statistics (id) VALUES(1)')
|
||||
end
|
||||
end
|
||||
|
|
@ -1885,10 +1885,6 @@ ActiveRecord::Schema.define(version: 20181126150622) do
|
|||
t.index ["name"], name: "index_shards_on_name", unique: true, using: :btree
|
||||
end
|
||||
|
||||
create_table "site_statistics", force: :cascade do |t|
|
||||
t.integer "repositories_count", default: 0, null: false
|
||||
end
|
||||
|
||||
create_table "snippets", force: :cascade do |t|
|
||||
t.string "title"
|
||||
t.text "content"
|
||||
|
|
|
|||
|
|
@ -1,15 +0,0 @@
|
|||
namespace :gitlab do
|
||||
desc "GitLab | Refresh Site Statistics counters"
|
||||
task refresh_site_statistics: :environment do
|
||||
puts 'Updating Site Statistics counters: '
|
||||
|
||||
print '* Repositories... '
|
||||
SiteStatistic.transaction do
|
||||
# see https://gitlab.com/gitlab-org/gitlab-ce/issues/48967
|
||||
ActiveRecord::Base.connection.execute('SET LOCAL statement_timeout TO 0') if Gitlab::Database.postgresql?
|
||||
SiteStatistic.update_all('repositories_count = (SELECT COUNT(*) FROM projects)')
|
||||
end
|
||||
puts 'OK!'.color(:green)
|
||||
puts
|
||||
end
|
||||
end
|
||||
|
|
@ -1,6 +0,0 @@
|
|||
FactoryBot.define do
|
||||
factory :site_statistics, class: 'SiteStatistic' do
|
||||
id 1
|
||||
repositories_count 999
|
||||
end
|
||||
end
|
||||
|
|
@ -109,22 +109,6 @@ describe Project do
|
|||
end
|
||||
end
|
||||
|
||||
context 'Site Statistics' do
|
||||
context 'when creating a new project' do
|
||||
it 'tracks project in SiteStatistic' do
|
||||
expect { create(:project) }.to change { SiteStatistic.fetch.repositories_count }.by(1)
|
||||
end
|
||||
end
|
||||
|
||||
context 'when deleting a project' do
|
||||
it 'untracks project in SiteStatistic' do
|
||||
project = create(:project)
|
||||
|
||||
expect { project.destroy }.to change { SiteStatistic.fetch.repositories_count }.by(-1)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context 'updating cd_cd_settings' do
|
||||
it 'does not raise an error' do
|
||||
project = create(:project)
|
||||
|
|
|
|||
|
|
@ -1,81 +0,0 @@
|
|||
require 'spec_helper'
|
||||
|
||||
describe SiteStatistic do
|
||||
describe '.fetch' do
|
||||
context 'existing record' do
|
||||
it 'returns existing SiteStatistic model' do
|
||||
statistics = create(:site_statistics)
|
||||
|
||||
expect(described_class.fetch).to be_a(described_class)
|
||||
expect(described_class.fetch).to eq(statistics)
|
||||
end
|
||||
end
|
||||
|
||||
context 'non existing record' do
|
||||
it 'creates a new SiteStatistic model' do
|
||||
expect(described_class.first).to be_nil
|
||||
expect(described_class.fetch).to be_a(described_class)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
describe '.track' do
|
||||
context 'with allowed attributes' do
|
||||
let(:statistics) { create(:site_statistics) }
|
||||
|
||||
it 'increases the attribute counter' do
|
||||
expect { described_class.track('repositories_count') }.to change { statistics.reload.repositories_count }.by(1)
|
||||
end
|
||||
|
||||
it 'doesnt increase the attribute counter when an exception happens during transaction' do
|
||||
expect do
|
||||
begin
|
||||
described_class.transaction do
|
||||
described_class.track('repositories_count')
|
||||
|
||||
raise StandardError
|
||||
end
|
||||
rescue StandardError
|
||||
# no-op
|
||||
end
|
||||
end.not_to change { statistics.reload.repositories_count }
|
||||
end
|
||||
end
|
||||
|
||||
context 'with not allowed attributes' do
|
||||
it 'returns error' do
|
||||
expect { described_class.track('something_else') }.to raise_error(ArgumentError).with_message(/Invalid attribute: \'something_else\' to \'track\' method/)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
describe '.untrack' do
|
||||
context 'with allowed attributes' do
|
||||
let(:statistics) { create(:site_statistics) }
|
||||
|
||||
it 'decreases the attribute counter' do
|
||||
expect { described_class.untrack('repositories_count') }.to change { statistics.reload.repositories_count }.by(-1)
|
||||
end
|
||||
|
||||
it 'doesnt decrease the attribute counter when an exception happens during transaction' do
|
||||
expect do
|
||||
begin
|
||||
described_class.transaction do
|
||||
described_class.track('repositories_count')
|
||||
|
||||
raise StandardError
|
||||
end
|
||||
rescue StandardError
|
||||
# no-op
|
||||
end
|
||||
end.not_to change { described_class.fetch.repositories_count }
|
||||
end
|
||||
end
|
||||
|
||||
context 'with not allowed attributes' do
|
||||
it 'returns error' do
|
||||
expect { described_class.untrack('something_else') }.to raise_error(ArgumentError).with_message(/Invalid attribute: \'something_else\' to \'untrack\' method/)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
@ -1,23 +0,0 @@
|
|||
# frozen_string_literal: true
|
||||
require 'rake_helper'
|
||||
|
||||
describe 'rake gitlab:refresh_site_statistics' do
|
||||
before do
|
||||
Rake.application.rake_require 'tasks/gitlab/site_statistics'
|
||||
|
||||
create(:project)
|
||||
SiteStatistic.fetch.update(repositories_count: 0)
|
||||
end
|
||||
|
||||
let(:task) { 'gitlab:refresh_site_statistics' }
|
||||
|
||||
it 'recalculates existing counters' do
|
||||
run_rake_task(task)
|
||||
|
||||
expect(SiteStatistic.fetch.repositories_count).to eq(1)
|
||||
end
|
||||
|
||||
it 'displays message listing counters' do
|
||||
expect { run_rake_task(task) }.to output(/Updating Site Statistics counters:.* Repositories\.\.\. OK!/m).to_stdout
|
||||
end
|
||||
end
|
||||
Loading…
Reference in a new issue