Skip to content
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

Make reliable fetch work well with dynamic hostnames #1573

Closed
subelsky opened this issue Mar 18, 2014 · 9 comments
Closed

Make reliable fetch work well with dynamic hostnames #1573

subelsky opened this issue Mar 18, 2014 · 9 comments

Comments

@subelsky
Copy link
Contributor

Reliable fetch currently assumes the process is running on a platform with a static hostname. Ideally it would be more flexible to accommodate Heroku and other platforms.

This is a feature request for Sidekiq Pro 2.0 which would alleviate #1527. Could possibly address #1493.

@mperham
Copy link
Collaborator

mperham commented Mar 18, 2014

Can someone provide a sample (cleansed of private values) Heroku environment? Does Heroku provide any environment values that we can use in lieu of a hostname?

@subelsky
Copy link
Contributor Author

here's ours, scrubbed. Note we use a custom buildpack which might provide some avenues for setting a value like this

ACTOR_POOL_SIZE:                   10
BUILDPACK_URL:                     https://github.com/staqapp/heroku-buildpack-multi.git
DATABASE_URL:                      ############
GEM_PATH:                          vendor/bundle/ruby/1.9.1
LANG:                              en_US.UTF-8
LD_LIBRARY_PATH:                   /usr/local/lib:/usr/lib:/lib:/app/vendor/phantomjs/lib
PATH:                              /app/local/bin:bin:vendor/bundle/ruby/1.9.1/bin:/usr/local/bin:/usr/bin:/bin:/app/vendor/phantomjs/bin
REDISTOGO_URL:                     #####
SHARED_DATABASE_URL:               #####
SIDEKIQ_CONCURRENCY:               10
SSL_CERT_FILE:                     /usr/lib/ssl/certs/ca-certificates.crt
TZ:                                Etc/UTC

there may be something that could be added a .profile script as documented here

The Heroku Platform API is another source of information but at first glance those values don't look helpful.

The only thing that is constant is the name of the process - not ideal, but I guess instead of indexing with $DYNO you could index with the process name, but then you could only have one dyno per process. The procfile would be like:

worker_a: bundle exec sidekiq -i 'worker_a' #...
worker_b: bundle exec sidekiq -i 'worker_b' #...
....
worker_n: bundle exec sidekiq -i 'worker_n' #...

@mperham
Copy link
Collaborator

mperham commented Mar 19, 2014

How does a process get its own name?

@subelsky
Copy link
Contributor Author

I'm not sure. I wonder if @hone would be able to weigh in

@mperham mperham closed this as completed Mar 21, 2014
@mperham
Copy link
Collaborator

mperham commented Mar 21, 2014

Ok, the new logic for the private queue name is:

      if ENV['DYNO']
        # Running on Heroku, hostnames are not predictable or stable.
        "queue:#{q}_#{options[:index]}"
      else
        "queue:#{q}_#{Socket.gethostname}_#{options[:index]}"
      end

Sidekiq Pro 1.5.0 has been released with this fix.

@mperham
Copy link
Collaborator

mperham commented Mar 21, 2014

So on Heroku, the index must be globally unique not just on each host machine.

@mperham
Copy link
Collaborator

mperham commented Mar 21, 2014

If you use the -i ${DYNO:-1} trick, then the index should be globally unique enough. There is a possibility of duplicate job execution on startup or shutdown but that's already documented on the wiki best practices page.

@subelsky
Copy link
Contributor Author

@mperham thank you! deploying this now!

@subelsky
Copy link
Contributor Author

working great! this is a beautiful sight: WARN: ReliableFetch: recovering work on 6 messages

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants