New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More robust PuSH subscription refreshes #2799

Merged
merged 6 commits into from May 5, 2017

Conversation

Projects
None yet
3 participants
@Gargron
Member

Gargron commented May 4, 2017

  • 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
Fix #2473 - Use sidekiq scheduler to refresh PuSH subscriptions inste…
…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

@Gargron Gargron requested a review from mjankowski May 4, 2017

@@ -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

This comment has been minimized.

@nightpool

nightpool May 4, 2017

Collaborator

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?

This comment has been minimized.

@Gargron

Gargron May 4, 2017

Member

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.

This comment has been minimized.

@nightpool

nightpool May 4, 2017

Collaborator

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

This comment has been minimized.

@mjankowski

mjankowski May 5, 2017

Collaborator

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

This comment has been minimized.

@Gargron

Gargron May 5, 2017

Member

Yes that should be possible

private
def user_agent
"#{HTTP::Request::USER_AGENT} (Mastodon/#{Mastodon::Version}; +http://#{Rails.configuration.x.local_domain}/)"

This comment has been minimized.

@nightpool

nightpool May 4, 2017

Collaborator

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)

unless response.successful?
if response.code > 299 && response.code < 500 && response.code != 429

This comment has been minimized.

@mjankowski

mjankowski May 4, 2017

Collaborator

Maybe wrap this up in a well named method and remove the comments? - response_failed?(response.code) or something...

@mjankowski

The SubscribeService is basically untested before this PR. Can we add some spec coverage as part of making these changes?

account.secret = ''
Rails.logger.debug "PuSH subscription request for #{account.acct} failed: #{response.message}"
account.save!
elsif response.code > 199 && response.code < 300

This comment has been minimized.

@mjankowski

mjankowski May 4, 2017

Collaborator

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|

This comment has been minimized.

@mjankowski

mjankowski May 4, 2017

Collaborator

Maybe...

  • It looks like expiring is only ever used for this purpose ... maybe add a method like Account.expiring_soon which just knows about { 1.day.from_now }
  • Move this whole query to a method in this class like expiring_accounts.pluck(:id) ...

Gargron added some commits May 4, 2017

@mjankowski

There's probably some follow-up refactoring to do here around consolidating the pubsub error codes response handling (which is duped a bit now), but this is good enough to get in now I think.

I left one question about sidekiq queue specification.

Gargron added some commits May 5, 2017

@Gargron Gargron merged commit 8158477 into master May 5, 2017

0 of 3 checks passed

codeclimate Code Climate is analyzing this code.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@Gargron Gargron deleted the feature-robust-push-refresh branch May 5, 2017

Gargron added a commit that referenced this pull request May 5, 2017

More robust PuSH subscription refreshes (#2799)
* 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

ThibG added a commit to ThibG/mastodon that referenced this pull request May 6, 2017

More robust PuSH subscription refreshes (tootsuite#2799)
* Fix tootsuite#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 tootsuite#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 tootsuite#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

@yukota yukota referenced this pull request May 6, 2017

Closed

mastodon:push:clear doesn't work #2860

2 of 2 tasks complete

hyuki0000 added a commit to hyuki0000/mastodon that referenced this pull request May 7, 2017

Merge branch 'hyuki-1.3.3' into social.hyuki.net
* hyuki-1.3.3:
  Remove caption, add transparency to 'oops.png'
  Change 'oops.png' image.
  Bump version to 1.3.3
  Fix Scheduler::SubscriptionsScheduler (tootsuite#2834)
  Change favicon.ico.
  Fix tootsuite#2706 - Always respond with 200 to PuSH payloads (tootsuite#2733)
  Likely fix tootsuite#2458, fix tootsuite#2031 - handle out-of-order deletes for statuses (tootsuite#2734)
  More robust PuSH subscription refreshes (tootsuite#2799)
  Make db persistent.
  Change logo.
  Change link to src.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment