diff options
21 files changed, 47 insertions, 169 deletions
diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 222a6898726..3694d9e2abe 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -111,14 +111,6 @@ module ApplicationSettingsHelper ] end - def repository_storages_options_for_select(selected) - options = Gitlab.config.repositories.storages.map do |name, storage| - ["#{name} - #{storage['gitaly_address']}", name] - end - - options_for_select(options, selected) - end - def repository_storages_options_json options = Gitlab.config.repositories.storages.map do |name, storage| { diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index 3405ea389f8..31097b9151a 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -27,7 +27,6 @@ module MergeRequests success end end - log_info("Merge process finished on JID #{merge_jid} with state #{state}") rescue MergeError => e handle_merge_error(log_message: e.message, save_message_on_model: true) @@ -55,7 +54,7 @@ module MergeRequests error = if @merge_request.should_be_rebased? 'Only fast-forward merge is allowed for your project. Please update your source branch' - elsif !@merge_request.merged? && !@merge_request.mergeable? + elsif !@merge_request.mergeable? 'Merge request is not mergeable' end diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb index a3d7f623546..0364c0dd479 100644 --- a/app/services/merge_requests/post_merge_service.rb +++ b/app/services/merge_requests/post_merge_service.rb @@ -8,28 +8,17 @@ module MergeRequests # class PostMergeService < MergeRequests::BaseService def execute(merge_request) - # These operations need to happen transactionally - ActiveRecord::Base.transaction(requires_new: true) do - merge_request.mark_as_merged - - # These options do not call external services and should be - # relatively quick enough to put in a Transaction - create_event(merge_request) - todo_service.merge_merge_request(merge_request, current_user) - end - - # These operations are idempotent so can be safely run multiple times - notification_service.merge_mr(merge_request, current_user) - create_note(merge_request) + merge_request.mark_as_merged close_issues(merge_request) + todo_service.merge_merge_request(merge_request, current_user) + create_event(merge_request) + create_note(merge_request) + notification_service.merge_mr(merge_request, current_user) + execute_hooks(merge_request, 'merge') invalidate_cache_counts(merge_request, users: merge_request.assignees) merge_request.update_project_counter_caches delete_non_latest_diffs(merge_request) cleanup_environments(merge_request) - - # Anything after this point will be executed at-most-once. Less important activity only - # TODO: make all the work in here a separate sidekiq job so it can go in the transaction - execute_hooks(merge_request, 'merge') end private diff --git a/app/services/merge_requests/squash_service.rb b/app/services/merge_requests/squash_service.rb index 09d867f39ae..d25997c925e 100644 --- a/app/services/merge_requests/squash_service.rb +++ b/app/services/merge_requests/squash_service.rb @@ -4,14 +4,12 @@ module MergeRequests class SquashService < MergeRequests::BaseService include Git::Logger - def idempotent? - true - end - def execute # If performing a squash would result in no change, then # immediately return a success message without performing a squash - return success(squash_sha: merge_request.diff_head_sha) if squash_redundant? + if merge_request.commits_count < 2 && message.nil? + return success(squash_sha: merge_request.diff_head_sha) + end if merge_request.squash_in_progress? return error(s_('MergeRequests|Squash task canceled: another squash is already in progress.')) @@ -22,12 +20,6 @@ module MergeRequests private - def squash_redundant? - return true if merge_request.merged? - - merge_request.commits_count < 2 && message.nil? - end - def squash! squash_sha = repository.squash(current_user, merge_request, message || merge_request.default_squash_commit_message) diff --git a/changelogs/unreleased/213062-disallow-distinct-count-for-the-ordinary-batch-counter.yml b/changelogs/unreleased/213062-disallow-distinct-count-for-the-ordinary-batch-counter.yml new file mode 100644 index 00000000000..d219b6a52c2 --- /dev/null +++ b/changelogs/unreleased/213062-disallow-distinct-count-for-the-ordinary-batch-counter.yml @@ -0,0 +1,5 @@ +--- +title: Disallow distinct count for regular batch count +merge_request: 28518 +author: +type: performance diff --git a/changelogs/unreleased/32737-make-mergeservice-idempotent.yml b/changelogs/unreleased/32737-make-mergeservice-idempotent.yml deleted file mode 100644 index 5f434df2165..00000000000 --- a/changelogs/unreleased/32737-make-mergeservice-idempotent.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Make MergeService idempotent -merge_request: 24708 -author: -type: changed diff --git a/doc/administration/geo/replication/troubleshooting.md b/doc/administration/geo/replication/troubleshooting.md index 5ae199e374a..fae9705e935 100644 --- a/doc/administration/geo/replication/troubleshooting.md +++ b/doc/administration/geo/replication/troubleshooting.md @@ -486,9 +486,9 @@ to start again from scratch, there are a few steps that can help you: 1. Reset the Tracking Database ```shell - gitlab-rake geo:db:drop - gitlab-ctl reconfigure - gitlab-rake geo:db:setup + gitlab-rake geo:db:drop # on a secondary app node + gitlab-ctl reconfigure # on the tracking database node + gitlab-rake geo:db:setup # on a secondary app node ``` 1. Restart previously stopped services diff --git a/doc/user/clusters/applications.md b/doc/user/clusters/applications.md index 0d2a3ac3ed9..7cedd21edcb 100644 --- a/doc/user/clusters/applications.md +++ b/doc/user/clusters/applications.md @@ -52,7 +52,7 @@ Some applications are installable only for a project-level cluster. Support for installing these applications in a group-level cluster is planned for future releases. For updates, see [the issue tracking -progress](https://gitlab.com/gitlab-org/gitlab-foss/issues/51989). +progress](https://gitlab.com/gitlab-org/gitlab/-/issues/24411). CAUTION: **Caution:** If you have an existing Kubernetes cluster with Helm already installed, diff --git a/doc/user/project/web_ide/index.md b/doc/user/project/web_ide/index.md index 8680da3ad01..23b72e33aae 100644 --- a/doc/user/project/web_ide/index.md +++ b/doc/user/project/web_ide/index.md @@ -143,7 +143,7 @@ below. CAUTION: **Warning:** Interactive Web Terminals for the Web IDE is currently in **Beta**. -Shared Runners [do not yet support Interactive Web Terminals](https://gitlab.com/gitlab-org/gitlab-foss/issues/52611), +Shared Runners [do not yet support Interactive Web Terminals](https://gitlab.com/gitlab-org/gitlab/-/issues/24674), so you would need to use your own private Runner(s) to make use of this feature. [Interactive Web Terminals](../../../ci/interactive_web_terminal/index.md) diff --git a/lib/gitlab/database/batch_count.rb b/lib/gitlab/database/batch_count.rb index 1faaac95575..3eb0197d178 100644 --- a/lib/gitlab/database/batch_count.rb +++ b/lib/gitlab/database/batch_count.rb @@ -56,6 +56,7 @@ module Gitlab def count(batch_size: nil, mode: :itself, start: nil, finish: nil) raise 'BatchCount can not be run inside a transaction' if ActiveRecord::Base.connection.transaction_open? raise "The mode #{mode.inspect} is not supported" unless [:itself, :distinct].include?(mode) + raise 'Use distinct count for optimized distinct counting' if @relation.limit(1).distinct_value.present? && mode != :distinct # non-distinct have better performance batch_size ||= mode == :distinct ? DEFAULT_DISTINCT_BATCH_SIZE : DEFAULT_BATCH_SIZE diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 5fd9e119bc0..c94f6b6804d 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -3794,9 +3794,6 @@ msgstr "" msgid "Choose which shards you wish to synchronize to this secondary node" msgstr "" -msgid "Choose which shards you wish to synchronize to this secondary node." -msgstr "" - msgid "Choose which status most accurately reflects the current state of this issue:" msgstr "" @@ -9416,30 +9413,9 @@ msgstr "" msgid "Geo|All projects are being scheduled for re-verify" msgstr "" -msgid "Geo|Allow this secondary node to replicate content on Object Storage" -msgstr "" - msgid "Geo|Batch operations" msgstr "" -msgid "Geo|Choose which groups you wish to synchronize to this secondary node." -msgstr "" - -msgid "Geo|Container repositories sync capacity" -msgstr "" - -msgid "Geo|Control the maximum concurrency of LFS/attachment backfill for this secondary node" -msgstr "" - -msgid "Geo|Control the maximum concurrency of container repository operations for this Geo node" -msgstr "" - -msgid "Geo|Control the maximum concurrency of verification operations for this Geo node" -msgstr "" - -msgid "Geo|Control the minimum interval in days that a repository should be reverified for this primary node" -msgstr "" - msgid "Geo|Could not remove tracking entry for an existing project." msgstr "" @@ -9449,24 +9425,12 @@ msgstr "" msgid "Geo|Failed" msgstr "" -msgid "Geo|File sync capacity" -msgstr "" - msgid "Geo|Geo Status" msgstr "" -msgid "Geo|Groups to synchronize" -msgstr "" - -msgid "Geo|If enabled, and if object storage is enabled, GitLab will handle Object Storage replication using Geo" -msgstr "" - msgid "Geo|In sync" msgstr "" -msgid "Geo|Internal URL (optional)" -msgstr "" - msgid "Geo|Last repository check run" msgstr "" @@ -9512,9 +9476,6 @@ msgstr "" msgid "Geo|Projects in certain storage shards" msgstr "" -msgid "Geo|Re-verification interval" -msgstr "" - msgid "Geo|Redownload" msgstr "" @@ -9527,9 +9488,6 @@ msgstr "" msgid "Geo|Remove tracking database entry" msgstr "" -msgid "Geo|Repository sync capacity" -msgstr "" - msgid "Geo|Resync" msgstr "" @@ -9545,15 +9503,6 @@ msgstr "" msgid "Geo|Reverify all projects" msgstr "" -msgid "Geo|Select groups to replicate." -msgstr "" - -msgid "Geo|Selective synchronization" -msgstr "" - -msgid "Geo|Shards to synchronize" -msgstr "" - msgid "Geo|Status" msgstr "" @@ -9566,18 +9515,12 @@ msgstr "" msgid "Geo|Synchronization failed - %{error}" msgstr "" -msgid "Geo|The URL defined on the primary node that secondary nodes should use to contact it. Defaults to URL" -msgstr "" - msgid "Geo|The database is currently %{db_lag} behind the primary node." msgstr "" msgid "Geo|The node is currently %{minutes_behind} behind the primary node." msgstr "" -msgid "Geo|This is a primary node" -msgstr "" - msgid "Geo|Tracking database entry will be removed. Are you sure?" msgstr "" @@ -9587,15 +9530,9 @@ msgstr "" msgid "Geo|Tracking entry for upload (%{type}/%{id}) was successfully removed." msgstr "" -msgid "Geo|URL" -msgstr "" - msgid "Geo|Unknown state" msgstr "" -msgid "Geo|Verification capacity" -msgstr "" - msgid "Geo|Verification failed - %{error}" msgstr "" @@ -20171,9 +20108,6 @@ msgstr "" msgid "The unique identifier for the Geo node. Must match %{geoNodeName} if it is set in gitlab.rb, otherwise it must match %{externalUrl} with a trailing slash" msgstr "" -msgid "The unique identifier for the Geo node. Must match `geo_node_name` if it is set in gitlab.rb, otherwise it must match `external_url` with a trailing slash" -msgstr "" - msgid "The update action will time out after %{number_of_minutes} minutes. For big repositories, use a clone/push combination." msgstr "" diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 30228248baf..74ed4a0f991 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -725,7 +725,7 @@ describe Projects::IssuesController do stub_feature_flags(allow_possible_spam: false) end - it 'rejects an issue recognized as a spam' do + it 'rejects an issue recognized as spam' do expect { update_issue }.not_to change { issue.reload.title } end @@ -981,7 +981,7 @@ describe Projects::IssuesController do stub_feature_flags(allow_possible_spam: false) end - it 'rejects an issue recognized as a spam' do + it 'rejects an issue recognized as spam' do expect { post_spam_issue }.not_to change(Issue, :count) end diff --git a/spec/features/issues/spam_issues_spec.rb b/spec/features/issues/spam_issues_spec.rb index 18245f249fd..b59cd2d632a 100644 --- a/spec/features/issues/spam_issues_spec.rb +++ b/spec/features/issues/spam_issues_spec.rb @@ -23,7 +23,7 @@ describe 'New issue', :js do sign_in(user) end - context 'when identified as a spam' do + context 'when identified as spam' do before do WebMock.stub_request(:any, /.*akismet.com.*/).to_return(body: "true", status: 200) @@ -74,7 +74,7 @@ describe 'New issue', :js do end end - context 'when not identified as a spam' do + context 'when not identified as spam' do before do WebMock.stub_request(:any, /.*akismet.com.*/).to_return(body: 'false', status: 200) diff --git a/spec/features/snippets/spam_snippets_spec.rb b/spec/features/snippets/spam_snippets_spec.rb index efe1bdc963d..e9534dedcd3 100644 --- a/spec/features/snippets/spam_snippets_spec.rb +++ b/spec/features/snippets/spam_snippets_spec.rb @@ -52,7 +52,7 @@ shared_examples_for 'snippet editor' do end end - context 'when identified as a spam' do + context 'when identified as spam' do before do WebMock.stub_request(:any, /.*akismet.com.*/).to_return(body: "true", status: 200) end @@ -66,7 +66,7 @@ shared_examples_for 'snippet editor' do end end - context 'when not identified as a spam' do + context 'when not identified as spam' do before do WebMock.stub_request(:any, /.*akismet.com.*/).to_return(body: "false", status: 200) end diff --git a/spec/lib/gitlab/database/batch_count_spec.rb b/spec/lib/gitlab/database/batch_count_spec.rb index ec161cd6dcb..7842323d009 100644 --- a/spec/lib/gitlab/database/batch_count_spec.rb +++ b/spec/lib/gitlab/database/batch_count_spec.rb @@ -49,6 +49,12 @@ describe Gitlab::Database::BatchCount do [1, 2, 4, 5, 6].each { |i| expect(described_class.batch_count(model, batch_size: i)).to eq(5) } end + it 'will raise an error if distinct count is requested' do + expect do + described_class.batch_count(model.distinct(column)) + end.to raise_error 'Use distinct count for optimized distinct counting' + end + context 'in a transaction' do let(:in_transaction) { true } diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 09fff389cec..a316c8a4219 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -383,20 +383,21 @@ describe Issues::CreateService do context 'when recaptcha was verified' do let(:log_user) { user } let(:spam_logs) { create_list(:spam_log, 2, user: log_user, title: 'Awesome issue') } + let(:target_spam_log) { spam_logs.last } before do opts[:recaptcha_verified] = true - opts[:spam_log_id] = spam_logs.last.id + opts[:spam_log_id] = target_spam_log.id expect(Spam::AkismetService).not_to receive(:new) end - it 'does no mark an issue as a spam ' do + it 'does not mark an issue as spam' do expect(issue).not_to be_spam end - it 'an issue is valid ' do - expect(issue.valid?).to be_truthy + it 'issue is valid ' do + expect(issue).to be_valid end it 'does not assign a spam_log to an issue' do @@ -431,7 +432,7 @@ describe Issues::CreateService do end context 'when issuables_recaptcha_enabled feature flag is true' do - it 'marks an issue as a spam ' do + it 'marks an issue as spam' do expect(issue).to be_spam end @@ -444,7 +445,7 @@ describe Issues::CreateService do .to have_spam_log(title: issue.title, description: issue.description, user_id: user.id, noteable_type: 'Issue') end - it 'assigns a spam_log to an issue' do + it 'assigns a spam_log to the issue' do expect(issue.spam_log).to eq(SpamLog.last) end end @@ -454,7 +455,7 @@ describe Issues::CreateService do stub_feature_flags(allow_possible_spam: true) end - it 'does not mark an issue as a spam ' do + it 'does not mark an issue as spam' do expect(issue).not_to be_spam end @@ -480,7 +481,7 @@ describe Issues::CreateService do end end - it 'does not mark an issue as a spam ' do + it 'does not mark an issue as spam' do expect(issue).not_to be_spam end diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 2283f480895..fa7f745d8a0 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -47,23 +47,6 @@ describe MergeRequests::MergeService do expect(note.note).to include 'merged' end - it 'is idempotent' do - repository = project.repository - commit_count = repository.commit_count - merge_commit = merge_request.merge_commit.id - - # a first invocation of execute is performed on the before block - service.execute(merge_request) - - expect(merge_request.merge_error).to be_falsey - expect(merge_request).to be_valid - expect(merge_request).to be_merged - - expect(repository.commits_by(oids: [merge_commit]).size).to eq(1) - expect(repository.commit_count).to eq(commit_count) - expect(merge_request.in_progress_merge_commit_sha).to be_nil - end - context 'when squashing' do let(:merge_params) do { commit_message: 'Merge commit message', diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb index 06735d25ad5..fff6ddf3928 100644 --- a/spec/services/merge_requests/post_merge_service_spec.rb +++ b/spec/services/merge_requests/post_merge_service_spec.rb @@ -17,6 +17,7 @@ describe MergeRequests::PostMergeService do it 'refreshes the number of open merge requests for a valid MR', :use_clean_rails_memory_store_caching do # Cache the counter before the MR changed state. project.open_merge_requests_count + merge_request.update!(state: 'merged') service = described_class.new(project, user, {}) diff --git a/spec/services/merge_requests/squash_service_spec.rb b/spec/services/merge_requests/squash_service_spec.rb index 4636f26bb16..cb278eec692 100644 --- a/spec/services/merge_requests/squash_service_spec.rb +++ b/spec/services/merge_requests/squash_service_spec.rb @@ -137,24 +137,6 @@ describe MergeRequests::SquashService do include_examples 'the squash succeeds' end - context 'when the merge request has already been merged' do - let(:merge_request) { merge_request_with_one_commit } - - it 'checks the side-effects for multiple calls' do - merge_request.mark_as_merged - - expect(service).to be_idempotent - expect { IdempotentWorkerHelper::WORKER_EXEC_TIMES.times { service.execute } }.not_to raise_error - end - - it 'idempotently returns a success' do - merge_request.mark_as_merged - result = service.execute - - expect(result).to match(status: :success, squash_sha: merge_request.diff_head_sha) - end - end - context 'git errors' do let(:merge_request) { merge_request_with_only_new_files } let(:error) { 'A test error' } diff --git a/spec/services/snippets/create_service_spec.rb b/spec/services/snippets/create_service_spec.rb index 8c91763cc48..690aa2c066e 100644 --- a/spec/services/snippets/create_service_spec.rb +++ b/spec/services/snippets/create_service_spec.rb @@ -76,7 +76,7 @@ describe Snippets::CreateService do shared_examples 'spam check is performed' do shared_examples 'marked as spam' do - it 'marks a snippet as spam ' do + it 'marks a snippet as spam' do expect(snippet).to be_spam end diff --git a/spec/services/spam/spam_check_service_spec.rb b/spec/services/spam/spam_check_service_spec.rb index 3ebde2a92c6..3d0cb1447bd 100644 --- a/spec/services/spam/spam_check_service_spec.rb +++ b/spec/services/spam/spam_check_service_spec.rb @@ -91,9 +91,7 @@ describe Spam::SpamCheckService do end it 'updates spam log' do - subject - - expect(existing_spam_log.reload.recaptcha_verified).to be_truthy + expect { subject }.to change { existing_spam_log.reload.recaptcha_verified }.from(false).to(true) end end @@ -137,7 +135,7 @@ describe Spam::SpamCheckService do it 'marks as spam' do subject - expect(issue.reload.spam).to be_truthy + expect(issue).to be_spam end end @@ -147,7 +145,7 @@ describe Spam::SpamCheckService do it 'does not mark as spam' do subject - expect(issue.spam).to be_falsey + expect(issue).not_to be_spam end end end |