From ccdfe030e82989650a4489afde46538bf03fd330 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 2 Oct 2025 00:35:55 +0800 Subject: [PATCH] Use the sharding keys to determine the owner We also update that instead of storing only the changes, we store the record as well, so that we can use the record to load the sharding keys. This also simplifies some codes because we can get data from the record instead of passing a few values around. --- app/models/application_record.rb | 4 ++ app/models/concerns/cells/inspect_changes.rb | 14 +++++- .../concerns/cells/transaction_callback.rb | 6 ++- app/models/concerns/cells/unique.rb | 9 ++-- app/models/email.rb | 2 +- app/models/gpg_key.rb | 2 +- app/models/gpg_key_subkey.rb | 2 +- app/models/key.rb | 2 +- app/models/namespace.rb | 1 + app/models/organizations/organization.rb | 2 +- app/models/route.rb | 2 +- app/models/user.rb | 2 +- .../topology_service_client/claim_service.rb | 49 +++++++++++-------- spec/models/concerns/cells/unique_spec.rb | 4 +- spec/models/email_spec.rb | 2 +- spec/models/gpg_key_spec.rb | 2 +- spec/models/gpg_key_subkey_spec.rb | 2 +- spec/models/key_spec.rb | 2 +- .../models/organizations/organization_spec.rb | 2 +- spec/models/route_spec.rb | 2 +- spec/models/user_spec.rb | 2 +- .../cells/unique_shared_examples.rb | 4 +- 22 files changed, 76 insertions(+), 43 deletions(-) diff --git a/app/models/application_record.rb b/app/models/application_record.rb index 7cec15efa8d468..8195ee8d5f08f1 100644 --- a/app/models/application_record.rb +++ b/app/models/application_record.rb @@ -165,6 +165,10 @@ def self.sharding_keys @sharding_keys ||= Gitlab::Database::Dictionary.entry(table_name).sharding_key || {} end + def sharding_keys + @sharding_keys ||= self.class.sharding_keys.transform_keys(&method(:public_send)).invert + end + def readable_by?(user) Ability.allowed?(user, :"read_#{to_ability_name}", self) end diff --git a/app/models/concerns/cells/inspect_changes.rb b/app/models/concerns/cells/inspect_changes.rb index 8a9cffe0119052..0d4c96d8d34f93 100644 --- a/app/models/concerns/cells/inspect_changes.rb +++ b/app/models/concerns/cells/inspect_changes.rb @@ -2,6 +2,18 @@ module Cells class InspectChanges + class Visit + extend Forwardable + + attr_reader :record + def_delegators :@map, :[], :[]=, :filter_map + + def initialize(new_record) + @record = new_record + @map = {} + end + end + attr_reader :record def initialize(new_record, destroy: false) @@ -43,7 +55,7 @@ def inspected end def prepare_inspecting - visited[inspect_changes_key] = {} + visited[inspect_changes_key] = Visit.new(record) end def inspect_record diff --git a/app/models/concerns/cells/transaction_callback.rb b/app/models/concerns/cells/transaction_callback.rb index 69047adbeac274..74b1f7462770aa 100644 --- a/app/models/concerns/cells/transaction_callback.rb +++ b/app/models/concerns/cells/transaction_callback.rb @@ -8,6 +8,10 @@ def self.visited visited_stack.first # Flatten to the first end + def self.last_visited + visited_stack.last + end + def self.visited_stack Thread.current.thread_variable_get(THREAD_VARIABLE_NAME) || Thread.current.thread_variable_set(THREAD_VARIABLE_NAME, []) @@ -29,7 +33,7 @@ def commit_db_transaction # and when that happens, we'll roll it back, and we need to look at # the stack to know if it's a unbalanced ROLLBACK or not, therefore # we only pop after successful COMMIT or ROLLBACk. - Unique.claim_and_commit(TransactionCallback.visited_stack.last) do + Unique.claim_and_commit(TransactionCallback.last_visited.values) do super TransactionCallback.visited_stack.pop end diff --git a/app/models/concerns/cells/unique.rb b/app/models/concerns/cells/unique.rb index db8c3c84f39b7d..f68a5290755ce5 100644 --- a/app/models/concerns/cells/unique.rb +++ b/app/models/concerns/cells/unique.rb @@ -11,10 +11,11 @@ module Unique included do after_save :inspect_save_changes before_destroy :inspect_destroy_changes + cells_config end class_methods do - attr_reader :cells_owner_type, :cells_source_table + attr_reader :cells_type, :cells_source_table def cells_unique_attributes @cells_unique_attributes ||= {} @@ -24,8 +25,10 @@ def cells_unique_has_many_associations @cells_unique_has_many_associations ||= [] end - def cells_config(owner_type:, source_table:) - @cells_owner_type = owner_type + def cells_config( + type: OWNER_TYPE::UNSPECIFIED, + source_table: SOURCE_TABLE::UNSPECIFIED) + @cells_type = type @cells_source_table = source_table end diff --git a/app/models/email.rb b/app/models/email.rb index 7edbd89dc21dc7..677b273c3ef879 100644 --- a/app/models/email.rb +++ b/app/models/email.rb @@ -7,7 +7,7 @@ class Email < ApplicationRecord cells_unique_attribute :email, type: BUCKET_TYPE::UNSPECIFIED # TODO: EMAIL? - cells_config owner_type: OWNER_TYPE::USER, source_table: SOURCE_TABLE::UNSPECIFIED + cells_config source_table: SOURCE_TABLE::UNSPECIFIED belongs_to :user, optional: false belongs_to :banned_user, class_name: '::Users::BannedUser', foreign_key: 'user_id', inverse_of: 'emails' diff --git a/app/models/gpg_key.rb b/app/models/gpg_key.rb index 4fa06af9af70a0..a01cd55c69df01 100644 --- a/app/models/gpg_key.rb +++ b/app/models/gpg_key.rb @@ -11,7 +11,7 @@ class GpgKey < ApplicationRecord cells_unique_attribute :fingerprint, type: BUCKET_TYPE::UNSPECIFIED cells_unique_attribute :primary_keyid, type: BUCKET_TYPE::UNSPECIFIED - cells_config owner_type: OWNER_TYPE::UNSPECIFIED, source_table: SOURCE_TABLE::UNSPECIFIED + cells_config source_table: SOURCE_TABLE::UNSPECIFIED cells_unique_has_many :subkeys diff --git a/app/models/gpg_key_subkey.rb b/app/models/gpg_key_subkey.rb index 03cdff865730e3..3c041a6e6b6f79 100644 --- a/app/models/gpg_key_subkey.rb +++ b/app/models/gpg_key_subkey.rb @@ -5,7 +5,7 @@ class GpgKeySubkey < ApplicationRecord include Cells::Unique cells_unique_attribute :fingerprint, type: BUCKET_TYPE::UNSPECIFIED - cells_config owner_type: OWNER_TYPE::UNSPECIFIED, source_table: SOURCE_TABLE::UNSPECIFIED + cells_config source_table: SOURCE_TABLE::UNSPECIFIED sha_attribute :keyid sha_attribute :fingerprint diff --git a/app/models/key.rb b/app/models/key.rb index 8e2581876a65d7..418c1316afbf38 100644 --- a/app/models/key.rb +++ b/app/models/key.rb @@ -13,7 +13,7 @@ class Key < ApplicationRecord cells_unique_attribute :key, type: BUCKET_TYPE::UNSPECIFIED # TODO: KEY? cells_unique_attribute :fingerprint_sha256, type: BUCKET_TYPE::UNSPECIFIED # TODO: KEY_FINGERPRINT? - cells_config owner_type: OWNER_TYPE::UNSPECIFIED, source_table: SOURCE_TABLE::ORGANIZATIONS + cells_config source_table: SOURCE_TABLE::UNSPECIFIED sha256_attribute :fingerprint_sha256 diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 53e5260549fb08..de0ca2239a5636 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -21,6 +21,7 @@ class Namespace < ApplicationRecord include UseSqlFunctionForPrimaryKeyLookups include SafelyChangeColumnDefault include Todoable + include Cells::Unique extend Gitlab::Utils::Override diff --git a/app/models/organizations/organization.rb b/app/models/organizations/organization.rb index 7fdbce80c6d9b9..4843ade7a41029 100644 --- a/app/models/organizations/organization.rb +++ b/app/models/organizations/organization.rb @@ -10,7 +10,7 @@ class Organization < ApplicationRecord cells_unique_attribute :path, type: BUCKET_TYPE::UNSPECIFIED # TODO: PATH? - cells_config owner_type: OWNER_TYPE::ORGANIZATION, source_table: SOURCE_TABLE::ORGANIZATIONS + cells_config type: OWNER_TYPE::ORGANIZATION, source_table: SOURCE_TABLE::ORGANIZATIONS DEFAULT_ORGANIZATION_ID = 1 diff --git a/app/models/route.rb b/app/models/route.rb index aab42fd78f8de4..700c8b5e212c5d 100644 --- a/app/models/route.rb +++ b/app/models/route.rb @@ -8,7 +8,7 @@ class Route < ApplicationRecord cells_unique_attribute :path, type: BUCKET_TYPE::ROUTES - cells_config owner_type: OWNER_TYPE::UNSPECIFIED, source_table: SOURCE_TABLE::ROUTES + cells_config source_table: SOURCE_TABLE::ROUTES belongs_to :source, polymorphic: true, inverse_of: :route # rubocop:disable Cop/PolymorphicAssociations belongs_to :namespace, inverse_of: :namespace_route diff --git a/app/models/user.rb b/app/models/user.rb index daaf0ada131801..eafe3bea9dac98 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -42,7 +42,7 @@ class User < ApplicationRecord cells_unique_has_many :gpg_keys cells_unique_has_many :emails - cells_config owner_type: OWNER_TYPE::USER, source_table: SOURCE_TABLE::UNSPECIFIED + cells_config type: OWNER_TYPE::USER, source_table: SOURCE_TABLE::UNSPECIFIED ignore_column %i[role skype], remove_after: '2025-09-18', remove_with: '18.4' diff --git a/lib/gitlab/topology_service_client/claim_service.rb b/lib/gitlab/topology_service_client/claim_service.rb index 258f795729cdbd..654e2742d338d8 100644 --- a/lib/gitlab/topology_service_client/claim_service.rb +++ b/lib/gitlab/topology_service_client/claim_service.rb @@ -111,58 +111,57 @@ def build_claim_request(changes) end def build_create_records(changes) - changes.flat_map do |(table, id), columns_values| - columns_values.filter_map do |column, (_old_value, new_value)| + changes.flat_map do |visit| + visit.filter_map do |column, (_old_value, new_value)| if new_value # rubocop:disable Style/IfUnlessModifier -- I think this is more clear - build_claim_data(table, id, column, new_value) + build_claim_data(visit.record, column, new_value) end end end end - def build_claim_data(table, id, column, new_value) + def build_claim_data(record, column, new_value) Gitlab::Cells::TopologyService::ClaimData.new( - bucket: build_bucket(table, column, new_value), - owner: build_owner(table, id), - source: build_source(table, id) + bucket: build_bucket(record, column, new_value), + owner: build_owner(record), + source: build_source(record) ) end - def build_bucket(table, column, value) + def build_bucket(record, column, value) Gitlab::Cells::TopologyService::ClaimBucket.new( { - bucket_type: table.classify.constantize.cells_unique_attributes.dig(column.to_sym, :type), + bucket_type: record.class.cells_unique_attributes.dig(column.to_sym, :type), bucket_value: value } ) end - # TODO: How do we tell the owner, when the only thing we have is a - # list of changed tables and columns? We need to somehow retain the - # hierarchy to really tell - def build_owner(table, id) + # TODO: What if this record has more than one sharding keys? + def build_owner(record) + table, id = record.sharding_keys.first Gitlab::Cells::TopologyService::ClaimOwner.new( { - owner_type: table.classify.constantize.cells_owner_type, + owner_type: determine_owner_type_from_table(table), owner_id: id } ) end - def build_source(table, id) + def build_source(record) Gitlab::Cells::TopologyService::ClaimSource.new( { - source_table: table.classify.constantize.cells_source_table, - source_record_id: id + source_table: record.class.cells_source_table, + source_record_id: record.id } ) end def build_destroy_records(changes) - changes.flat_map do |(table, _id), columns_values| - columns_values.filter_map do |column, (old_value, _new_value)| + changes.flat_map do |visit| + visit.filter_map do |column, (old_value, _new_value)| if old_value.present? # rubocop:disable Style/IfUnlessModifier -- I think this is more clear - build_bucket(table, column, old_value) + build_bucket(visit.record, column, old_value) end end end @@ -175,6 +174,16 @@ def build_claim_lease(uuid) ) end + def determine_owner_type_from_table(table) + model_name = table.classify + + if Object.const_defined?(model_name) + model_name.constantize + else + table.capitalize.constantize.const_get(model_name) + end.cells_type + end + def service_class Gitlab::Cells::TopologyService::ClaimService::Stub end diff --git a/spec/models/concerns/cells/unique_spec.rb b/spec/models/concerns/cells/unique_spec.rb index d4121c29301125..b92dab4b9593f9 100644 --- a/spec/models/concerns/cells/unique_spec.rb +++ b/spec/models/concerns/cells/unique_spec.rb @@ -8,11 +8,11 @@ describe 'configuration' do it 'sets and retrieves cell configuration' do - test_klass.cells_config(owner_type: :user, source_table: :users) + test_klass.cells_config(type: :user, source_table: :users) test_klass.cells_unique_attribute(:email, type: :string) test_klass.cells_unique_attribute(:username, type: :string) - expect(test_klass.cells_owner_type).to eq(:user) + expect(test_klass.cells_type).to eq(:user) expect(test_klass.cells_source_table).to eq(:users) expect(test_klass.cells_unique_attributes).to eq(email: { type: :string }, username: { type: :string }) end diff --git a/spec/models/email_spec.rb b/spec/models/email_spec.rb index e4d820552a7df5..e0fb9df7e0f8ad 100644 --- a/spec/models/email_spec.rb +++ b/spec/models/email_spec.rb @@ -6,7 +6,7 @@ let_it_be(:user) { create(:user) } it_behaves_like 'cells unique model', - owner_type: Cells::Unique::OWNER_TYPE::USER, + type: Cells::Unique::OWNER_TYPE::UNSPECIFIED, source_table: Cells::Unique::SOURCE_TABLE::UNSPECIFIED, unique_attributes: [:email] diff --git a/spec/models/gpg_key_spec.rb b/spec/models/gpg_key_spec.rb index b71b3bf4b7eb7f..acad9633d120c7 100644 --- a/spec/models/gpg_key_spec.rb +++ b/spec/models/gpg_key_spec.rb @@ -6,7 +6,7 @@ subject { build(:gpg_key) } it_behaves_like 'cells unique model', - owner_type: Cells::Unique::OWNER_TYPE::UNSPECIFIED, + type: Cells::Unique::OWNER_TYPE::UNSPECIFIED, source_table: Cells::Unique::SOURCE_TABLE::UNSPECIFIED, unique_attributes: [:key, :fingerprint, :primary_keyid] diff --git a/spec/models/gpg_key_subkey_spec.rb b/spec/models/gpg_key_subkey_spec.rb index 295f0712cec532..e9d8a17cce7b6e 100644 --- a/spec/models/gpg_key_subkey_spec.rb +++ b/spec/models/gpg_key_subkey_spec.rb @@ -16,7 +16,7 @@ end it_behaves_like 'cells unique model', - owner_type: Cells::Unique::OWNER_TYPE::UNSPECIFIED, + type: Cells::Unique::OWNER_TYPE::UNSPECIFIED, source_table: Cells::Unique::SOURCE_TABLE::UNSPECIFIED, unique_attributes: [:fingerprint] end diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb index 66ca553c308736..3b6a7b142f2414 100644 --- a/spec/models/key_spec.rb +++ b/spec/models/key_spec.rb @@ -6,7 +6,7 @@ it_behaves_like 'having unique enum values' it_behaves_like 'cells unique model', - owner_type: Cells::Unique::OWNER_TYPE::UNSPECIFIED, + type: Cells::Unique::OWNER_TYPE::UNSPECIFIED, source_table: Cells::Unique::SOURCE_TABLE::ORGANIZATIONS, unique_attributes: [:key, :fingerprint_sha256] diff --git a/spec/models/organizations/organization_spec.rb b/spec/models/organizations/organization_spec.rb index d79b53274fae47..52486b630273c1 100644 --- a/spec/models/organizations/organization_spec.rb +++ b/spec/models/organizations/organization_spec.rb @@ -6,7 +6,7 @@ let_it_be_with_refind(:organization) { create(:organization) } it_behaves_like 'cells unique model', - owner_type: Cells::Unique::OWNER_TYPE::ORGANIZATION, + type: Cells::Unique::OWNER_TYPE::ORGANIZATION, source_table: Cells::Unique::SOURCE_TABLE::ORGANIZATIONS, unique_attributes: [:path] diff --git a/spec/models/route_spec.rb b/spec/models/route_spec.rb index 2207e72b2d8745..4db4bb72cba463 100644 --- a/spec/models/route_spec.rb +++ b/spec/models/route_spec.rb @@ -7,7 +7,7 @@ let(:route) { group.route } it_behaves_like 'cells unique model', - owner_type: Cells::Unique::OWNER_TYPE::UNSPECIFIED, + type: Cells::Unique::OWNER_TYPE::UNSPECIFIED, source_table: Cells::Unique::SOURCE_TABLE::ROUTES, unique_attributes: [:path] diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 08050de6faf322..aad954fb2fd622 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -50,7 +50,7 @@ it_behaves_like 'having unique enum values' it_behaves_like 'cells unique model', - owner_type: Cells::Unique::OWNER_TYPE::USER, + type: Cells::Unique::OWNER_TYPE::USER, source_table: Cells::Unique::SOURCE_TABLE::UNSPECIFIED, unique_attributes: [:username] diff --git a/spec/support/shared_examples/cells/unique_shared_examples.rb b/spec/support/shared_examples/cells/unique_shared_examples.rb index f5f152f52e705a..17a13d413936fe 100644 --- a/spec/support/shared_examples/cells/unique_shared_examples.rb +++ b/spec/support/shared_examples/cells/unique_shared_examples.rb @@ -1,8 +1,8 @@ # frozen_string_literal: true -RSpec.shared_examples 'cells unique model' do |owner_type:, source_table:, unique_attributes:| +RSpec.shared_examples 'cells unique model' do |type:, source_table:, unique_attributes:| it 'has the expected owner_type' do - expect(described_class.cells_owner_type).to eq(owner_type) + expect(described_class.cells_type).to eq(type) end it 'has the expected source_table' do -- GitLab