diff --git a/doc/development/sidekiq/index.md b/doc/development/sidekiq/index.md index 62e9e9f6b04af3b24ea0f0806e16377879990c0f..bbbdd2b175e8fbe895eb0203f0b4f732b1995916 100644 --- a/doc/development/sidekiq/index.md +++ b/doc/development/sidekiq/index.md @@ -29,6 +29,19 @@ All workers should include `ApplicationWorker` instead of `Sidekiq::Worker`, which adds some convenience methods and automatically sets the queue based on the [routing rules](../../administration/sidekiq/processing_specific_job_classes.md#routing-rules). +## Sharding + +All calls to Sidekiq APIs must account for sharding. To achieve this, +utilize the Sidekiq API within the `Sidekiq::Client.via` block to guarantee the correct `Sidekiq.redis` pool is utilized. +Obtain the suitable Redis pool by invoking the `Gitlab::SidekiqSharding::Router.get_shard_instance` method. + +```ruby +pool_name, pool = Gitlab::SidekiqSharding::Router.get_shard_instance(worker_class.sidekiq_options['store']) +Sidekiq::Client.via(pool) do + ... +end +``` + ## Retries Sidekiq defaults to using [25 retries](https://github.com/mperham/sidekiq/wiki/Error-Handling#automatic-job-retry), diff --git a/rubocop/cop/sidekiq_api_usage.rb b/rubocop/cop/sidekiq_api_usage.rb index 35e5ec474cdc6ed0c0c6c02313564d090976c773..62816a781fd7669e7442c2ceedbdbac5783bc6c6 100644 --- a/rubocop/cop/sidekiq_api_usage.rb +++ b/rubocop/cop/sidekiq_api_usage.rb @@ -3,8 +3,11 @@ module RuboCop module Cop class SidekiqApiUsage < RuboCop::Cop::Base - MSG = 'Refrain from directly using Sidekiq APIs.' \ - 'Only permitted in migrations, administrations and Sidekiq middlewares.' + MSG = 'Refrain from directly using Sidekiq APIs. ' \ + 'Only permitted in migrations, administrations and Sidekiq middlewares. ' \ + 'When disabling the cop, ensure that Sidekiq APIs are wrapped with ' \ + 'Sidekiq::Client.via(..) { ... } block to remain shard aware. ' \ + 'See doc/development/sidekiq/index.md#sharding for more information.' ALLOWED_WORKER_METHODS = [ :skipping_transaction_check, @@ -12,6 +15,8 @@ class SidekiqApiUsage < RuboCop::Cop::Base :raise_exception_for_being_inside_a_transaction? ].freeze + ALLOWED_CLIENT_METHODS = [:via].freeze + def_node_matcher :using_sidekiq_api?, <<~PATTERN (send (const (const nil? :Sidekiq) $_ ) $... ) PATTERN @@ -23,6 +28,9 @@ def on_send(node) # allow methods defined in config/initializers/forbid_sidekiq_in_transactions.rb next if klass == :Worker && ALLOWED_WORKER_METHODS.include?(methods_called[0]) + # allow Sidekiq::Client.via calls + next if klass == :Client && ALLOWED_CLIENT_METHODS.include?(methods_called[0]) + add_offense(node, message: MSG) end end diff --git a/spec/rubocop/cop/sidekiq_api_usage_spec.rb b/spec/rubocop/cop/sidekiq_api_usage_spec.rb index 79a0774e6253abeadcc5ec93a4c06448d64d59dd..851703ab3e1ba57b31030be8fe3e7675e52df99b 100644 --- a/spec/rubocop/cop/sidekiq_api_usage_spec.rb +++ b/spec/rubocop/cop/sidekiq_api_usage_spec.rb @@ -27,6 +27,19 @@ PATTERN end + it 'registers no offences for calling .via' do + expect_no_offenses(<<~PATTERN) + Sidekiq::Client.via { "testing" } + PATTERN + end + + it 'registers offence for calling other Sidekiq::Client methods' do + expect_offense(<<~PATTERN) + Sidekiq::Client.push('test') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{msg} + PATTERN + end + it 'registers offence for calling other Sidekiq::Worker methods' do expect_offense(<<~PATTERN) Sidekiq::Worker.drain_all