diff options
author | Andreas Brandl <abrandl@gitlab.com> | 2018-03-06 22:09:01 +0300 |
---|---|---|
committer | Andreas Brandl <abrandl@gitlab.com> | 2018-03-16 15:35:25 +0300 |
commit | 754272e392c0da088200a1b56156600973f63267 (patch) | |
tree | 21fdb2f633deff884d39d89f7672f230f1d6c143 | |
parent | a0abb904782970de456dae5539ad5de2afef0e05 (diff) |
Atomic generation of internal ids for issues.
-rw-r--r-- | app/models/concerns/atomic_internal_id.rb | 19 | ||||
-rw-r--r-- | app/models/internal_id.rb | 84 | ||||
-rw-r--r-- | app/models/issue.rb | 2 | ||||
-rw-r--r-- | app/models/project.rb | 2 | ||||
-rw-r--r-- | db/migrate/20180305095250_create_internal_ids_table.rb | 35 | ||||
-rw-r--r-- | db/schema.rb | 9 | ||||
-rw-r--r-- | spec/factories/internal_ids.rb | 6 | ||||
-rw-r--r-- | spec/models/internal_id_spec.rb | 87 |
8 files changed, 243 insertions, 1 deletions
diff --git a/app/models/concerns/atomic_internal_id.rb b/app/models/concerns/atomic_internal_id.rb new file mode 100644 index 00000000000..3cc9ce7f03f --- /dev/null +++ b/app/models/concerns/atomic_internal_id.rb @@ -0,0 +1,19 @@ +module AtomicInternalId + extend ActiveSupport::Concern + + included do + before_validation(on: :create) do + set_iid + end + + validates :iid, presence: true, numericality: true + end + + def set_iid + self.iid = InternalId.generate_next(self.project, :issues) if iid.blank? + end + + def to_param + iid.to_s + end +end diff --git a/app/models/internal_id.rb b/app/models/internal_id.rb new file mode 100644 index 00000000000..24c7cbf988f --- /dev/null +++ b/app/models/internal_id.rb @@ -0,0 +1,84 @@ +# An InternalId is a strictly monotone sequence of integers +# for a given project and usage (e.g. issues). +# +# For possible usages, see InternalId#usage enum. +class InternalId < ActiveRecord::Base + belongs_to :project + + enum usage: { issues: 0 } + + validates :usage, presence: true + validates :project_id, presence: true + + # Increments #last_value and saves the record + # + # The operation locks the record and gathers + # a `ROW SHARE` lock (in PostgreSQL). As such, + # the increment is atomic and safe to be called + # concurrently. + def increment_and_save! + lock! + self.last_value = (last_value || 0) + 1 + save! + last_value + end + + before_create :calculate_last_value! + + # Calculate #last_value by counting the number of + # existing records for this usage. + def calculate_last_value! + return if last_value + + parent = project # ??|| group + self.last_value = parent.send(usage.to_sym).maximum(:iid) || 0 # rubocop:disable GitlabSecurity/PublicSend + end + + class << self + # Generate next internal id for a given project and usage. + # + # For currently supported usages, see #usage enum. + # + # The method implements a locking scheme that has the following properties: + # 1) Generated sequence of internal ids is unique per (project, usage) + # 2) The method is thread-safe and may be used in concurrent threads/processes. + # 3) The generated sequence is gapless. + # 4) In the absence of a record in the internal_ids table, one will be created + # and last_value will be calculated on the fly. + def generate_next(project, usage) + raise 'project not set - this is required' unless project + + project.transaction do + # Create a record in internal_ids if one does not yet exist + id = (lookup(project, usage) || create_record(project, usage)) + + # This will lock the InternalId record with ROW SHARE + # and increment #last_value + id.increment_and_save! + end + end + + private + + # Retrieve InternalId record for (project, usage) combination, if it exists + def lookup(project, usage) + project.internal_ids.find_by(usage: usages[usage.to_s]) + end + + # Create InternalId record for (project, usage) combination, if it doesn't exist + # + # We blindly insert without any synchronization. If another process + # was faster in doing this, we'll realize once we hit the unique key constraint + # violation. We can safely roll-back the nested transaction and perform + # a lookup instead to retrieve the record. + def create_record(project, usage) + begin + project.transaction(requires_new: true) do + create!(project: project, usage: usages[usage.to_s]) + end + rescue ActiveRecord::RecordNotUnique + lookup(project, usage) + end + end + end +end diff --git a/app/models/issue.rb b/app/models/issue.rb index cca6224d65c..cf547106122 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -1,7 +1,7 @@ require 'carrierwave/orm/activerecord' class Issue < ActiveRecord::Base - include NonatomicInternalId + include AtomicInternalId include Issuable include Noteable include Referable diff --git a/app/models/project.rb b/app/models/project.rb index a291ad7eed5..93b0da5cce1 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -188,6 +188,8 @@ class Project < ActiveRecord::Base has_many :todos has_many :notification_settings, as: :source, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent + has_many :internal_ids + has_one :import_data, class_name: 'ProjectImportData', inverse_of: :project, autosave: true has_one :project_feature, inverse_of: :project has_one :statistics, class_name: 'ProjectStatistics' diff --git a/db/migrate/20180305095250_create_internal_ids_table.rb b/db/migrate/20180305095250_create_internal_ids_table.rb new file mode 100644 index 00000000000..19c1904bb43 --- /dev/null +++ b/db/migrate/20180305095250_create_internal_ids_table.rb @@ -0,0 +1,35 @@ +class CreateInternalIdsTable < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + create_table :internal_ids do |t| + t.references :project + t.integer :usage, null: false + t.integer :last_value, null: false + end + + unless index_exists?(:internal_ids, [:usage, :project_id]) + add_index :internal_ids, [:usage, :project_id], unique: true + end + + unless foreign_key_exists?(:internal_ids, :project_id) + add_concurrent_foreign_key :internal_ids, :projects, column: :project_id, on_delete: :cascade + end + end + + def down + drop_table :internal_ids + end + + private + + def foreign_key_exists?(table, column) + foreign_keys(table).any? do |key| + key.options[:column] == column.to_s + end + end +end diff --git a/db/schema.rb b/db/schema.rb index ab4370e2754..3785bf14d5c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -866,6 +866,14 @@ ActiveRecord::Schema.define(version: 20180309160427) do add_index "identities", ["user_id"], name: "index_identities_on_user_id", using: :btree + create_table "internal_ids", force: :cascade do |t| + t.integer "project_id" + t.integer "usage", null: false + t.integer "last_value", null: false + end + + add_index "internal_ids", ["usage", "project_id"], name: "index_internal_ids_on_usage_and_project_id", unique: true, using: :btree + create_table "issue_assignees", id: false, force: :cascade do |t| t.integer "user_id", null: false t.integer "issue_id", null: false @@ -2058,6 +2066,7 @@ ActiveRecord::Schema.define(version: 20180309160427) do add_foreign_key "gpg_signatures", "gpg_keys", on_delete: :nullify add_foreign_key "gpg_signatures", "projects", on_delete: :cascade add_foreign_key "group_custom_attributes", "namespaces", column: "group_id", on_delete: :cascade + add_foreign_key "internal_ids", "projects", name: "fk_f7d46b66c6", on_delete: :cascade add_foreign_key "issue_assignees", "issues", name: "fk_b7d881734a", on_delete: :cascade add_foreign_key "issue_assignees", "users", name: "fk_5e0c8d9154", on_delete: :cascade add_foreign_key "issue_metrics", "issues", on_delete: :cascade diff --git a/spec/factories/internal_ids.rb b/spec/factories/internal_ids.rb new file mode 100644 index 00000000000..b4c14d22a29 --- /dev/null +++ b/spec/factories/internal_ids.rb @@ -0,0 +1,6 @@ +FactoryBot.define do + factory :internal_id do + project + usage { InternalId.usages.keys.first } + end +end diff --git a/spec/models/internal_id_spec.rb b/spec/models/internal_id_spec.rb new file mode 100644 index 00000000000..b953b6a2df8 --- /dev/null +++ b/spec/models/internal_id_spec.rb @@ -0,0 +1,87 @@ +require 'spec_helper' + +describe InternalId do + let(:project) { create(:project) } + let(:usage) { :issues } + + context 'validations' do + it { is_expected.to validate_presence_of(:usage) } + it { is_expected.to validate_presence_of(:project_id) } + end + + describe '.generate_next' do + context 'in the absence of a record' do + subject { described_class.generate_next(project, usage) } + + it 'creates a record if not yet present' do + expect { subject }.to change { described_class.count }.from(0).to(1) + end + + it 'stores record attributes' do + subject + + described_class.first.tap do |record| + expect(record.project).to eq(project) + expect(record.usage).to eq(usage.to_s) # TODO + end + end + + context 'with existing issues' do + before do + rand(10).times { create(:issue, project: project) } + end + + it 'calculates last_value values automatically' do + expect(subject).to eq(project.issues.size + 1) + end + end + end + + it 'generates a strictly monotone, gapless sequence' do + seq = (0..rand(1000)).map do + described_class.generate_next(project, usage) + end + normalized = seq.map { |i| i - seq.min } + expect(normalized).to eq((0..seq.size - 1).to_a) + end + end + + describe '#increment_and_save!' do + let(:id) { create(:internal_id) } + subject { id.increment_and_save! } + + it 'returns incremented iid' do + value = id.last_value + expect(subject).to eq(value + 1) + end + + it 'saves the record' do + subject + expect(id.changed?).to be_falsey + end + + context 'with last_value=nil' do + let(:id) { build(:internal_id, last_value: nil) } + + it 'returns 1' do + expect(subject).to eq(1) + end + end + end + + describe '#calculate_last_value! (for issues)' do + subject do + build(:internal_id, project: project, usage: :issues) + end + + context 'with existing issues' do + before do + rand(10).times { create(:issue, project: project) } + end + + it 'counts related issues and saves' do + expect { subject.calculate_last_value! }.to change { subject.last_value }.from(nil).to(project.issues.size) + end + end + end +end |