More robust PuSH subscription refreshes #2799
Conversation
…ad of cron Fix an issue where / in domain would raise exception in TagManager#normalize_domain PuSH subscriptions refresh done in a round-robin way to avoid hammering a single server's hub in sequence. Correct handling of failures/retries through Sidekiq (see also #2613). Optimize Account#with_followers scope. Also, since subscriptions are now delegated to Sidekiq jobs, an uncaught exception will not stop the entire refreshing operation halfway through Fix #2702 - Correct user agent header on outgoing http requests
Procfile.dev
Outdated
| @@ -1,3 +1,4 @@ | |||
| web: PORT=3000 bundle exec puma -C config/puma.rb | |||
| sidekiq: bundle exec sidekiq -q default -q push -q pull -q mailers | |||
There was a problem hiding this comment.
will this need a corresponding announcement and docs update? Can we have an obvious admin error message if people aren't running the all of the queues?
There was a problem hiding this comment.
This'll need a docs update with recommendation to run foreman start instead of just rails s, though you may not always need sidekiq during dev so I think mostly it's fine.
There was a problem hiding this comment.
ah, sorry, this is my mistake. I misread this line and thought you were adding a queue here, rather then just adding the same four queues to the dev Procfile
There was a problem hiding this comment.
Is it possible to just do bundle exec sidekiq here and have it detect the queues from the newly added config file ... or do we need to specify the queues here?
If we CAN rely on just the config file, we can probably also remove the list of -q options from the main Procfile
There was a problem hiding this comment.
Yes that should be possible
app/helpers/http_helper.rb
Outdated
| private | ||
|
|
||
| def user_agent | ||
| "#{HTTP::Request::USER_AGENT} (Mastodon/#{Mastodon::Version}; +http://#{Rails.configuration.x.local_domain}/)" |
There was a problem hiding this comment.
probably better to have @@user_agent ||= "#{HTTP::Request::USER_AGENT}... etc. so we don't introduce any possible performance regressions. (generating a new string every time instead of once)
app/services/subscribe_service.rb
Outdated
|
|
||
| unless response.successful? | ||
| if response.code > 299 && response.code < 500 && response.code != 429 |
There was a problem hiding this comment.
Maybe wrap this up in a well named method and remove the comments? - response_failed?(response.code) or something...
app/services/subscribe_service.rb
Outdated
| account.secret = '' | ||
| Rails.logger.debug "PuSH subscription request for #{account.acct} failed: #{response.message}" | ||
| account.save! | ||
| elsif response.code > 199 && response.code < 300 |
There was a problem hiding this comment.
Same here - lose comments, use method with intention revealing name.
| include Sidekiq::Worker | ||
|
|
||
| def perform | ||
| Account.expiring(1.day.from_now).partitioned.pluck(:id) do |id| |
There was a problem hiding this comment.
Maybe...
- It looks like
expiringis only ever used for this purpose ... maybe add a method likeAccount.expiring_soonwhich just knows about{ 1.day.from_now } - Move this whole query to a method in this class like
expiring_accounts.pluck(:id)...
* Fix #2473 - Use sidekiq scheduler to refresh PuSH subscriptions instead of cron Fix an issue where / in domain would raise exception in TagManager#normalize_domain PuSH subscriptions refresh done in a round-robin way to avoid hammering a single server's hub in sequence. Correct handling of failures/retries through Sidekiq (see also #2613). Optimize Account#with_followers scope. Also, since subscriptions are now delegated to Sidekiq jobs, an uncaught exception will not stop the entire refreshing operation halfway through Fix #2702 - Correct user agent header on outgoing http requests * Add test for SubscribeService * Extract #expiring_accounts into method * Make mastodon:push:refresh no-op * Queues are now defined in sidekiq.yml * Queues are now in sidekiq.yml
server's hub in sequence. Correct handling of failures/retries through Sidekiq (see
also Some public posts from followed remote users missing #2613). Optimize Account#with_followers scope. Also, since subscriptions
are now delegated to Sidekiq jobs, an uncaught exception will not stop the entire
refreshing operation halfway through