diff options
author | Gabriel Mazetto <brodock@gmail.com> | 2018-11-23 07:33:07 +0300 |
---|---|---|
committer | Gabriel Mazetto <brodock@gmail.com> | 2018-11-27 01:39:05 +0300 |
commit | fe2e6c6dc046748f8760bfbb3c74185bdbf1359b (patch) | |
tree | 2e95576786cb3936b7442c420a5ccf42ddfa266d | |
parent | deaf3af7e5f357f3e8d91f7f2d49ad3ce001ba68 (diff) |
Remove Site Statistic
This approach caused many different problems as we tightened
the query execution timeout.
-rw-r--r-- | app/models/project.rb | 3 | ||||
-rw-r--r-- | app/models/site_statistic.rb | 76 | ||||
-rw-r--r-- | changelogs/unreleased/53778-remove-site-statistics.yml | 5 | ||||
-rw-r--r-- | db/migrate/20181123042307_drop_site_statistics.rb | 22 | ||||
-rw-r--r-- | db/schema.rb | 4 | ||||
-rw-r--r-- | lib/tasks/gitlab/site_statistics.rake | 15 | ||||
-rw-r--r-- | spec/factories/site_statistics.rb | 6 | ||||
-rw-r--r-- | spec/models/project_spec.rb | 16 | ||||
-rw-r--r-- | spec/models/site_statistic_spec.rb | 81 | ||||
-rw-r--r-- | spec/tasks/gitlab/site_statistics_rake_spec.rb | 23 |
10 files changed, 27 insertions, 224 deletions
diff --git a/app/models/project.rb b/app/models/project.rb index 4d1917b9ab2..c25d76647f4 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -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? } diff --git a/app/models/site_statistic.rb b/app/models/site_statistic.rb deleted file mode 100644 index 3a7912ed53a..00000000000 --- a/app/models/site_statistic.rb +++ /dev/null @@ -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 diff --git a/changelogs/unreleased/53778-remove-site-statistics.yml b/changelogs/unreleased/53778-remove-site-statistics.yml new file mode 100644 index 00000000000..fe006e43671 --- /dev/null +++ b/changelogs/unreleased/53778-remove-site-statistics.yml @@ -0,0 +1,5 @@ +--- +title: Removed Site Statistics optimization as it was causing problems +merge_request: 23314 +author: +type: removed diff --git a/db/migrate/20181123042307_drop_site_statistics.rb b/db/migrate/20181123042307_drop_site_statistics.rb new file mode 100644 index 00000000000..8986374ef65 --- /dev/null +++ b/db/migrate/20181123042307_drop_site_statistics.rb @@ -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 diff --git a/db/schema.rb b/db/schema.rb index acabd7b442b..3c02469250d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -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" diff --git a/lib/tasks/gitlab/site_statistics.rake b/lib/tasks/gitlab/site_statistics.rake deleted file mode 100644 index d97f11b2ed5..00000000000 --- a/lib/tasks/gitlab/site_statistics.rake +++ /dev/null @@ -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 diff --git a/spec/factories/site_statistics.rb b/spec/factories/site_statistics.rb deleted file mode 100644 index 2533d0eecc2..00000000000 --- a/spec/factories/site_statistics.rb +++ /dev/null @@ -1,6 +0,0 @@ -FactoryBot.define do - factory :site_statistics, class: 'SiteStatistic' do - id 1 - repositories_count 999 - end -end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 51278836604..59cd69c55ae 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -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) diff --git a/spec/models/site_statistic_spec.rb b/spec/models/site_statistic_spec.rb deleted file mode 100644 index 0e739900065..00000000000 --- a/spec/models/site_statistic_spec.rb +++ /dev/null @@ -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 diff --git a/spec/tasks/gitlab/site_statistics_rake_spec.rb b/spec/tasks/gitlab/site_statistics_rake_spec.rb deleted file mode 100644 index c43ce25a540..00000000000 --- a/spec/tasks/gitlab/site_statistics_rake_spec.rb +++ /dev/null @@ -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 |