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

Handle crashes #206

Closed
wants to merge 8 commits into from

Conversation

@jipiboily
Copy link
Contributor

commented Mar 25, 2014

This is related to #189. The original implementation can't work in a transaction (see #189 for details).

This implementation in this new PR is actually working and also much simpler.

I only did a quick test with it and looks like it is working perfectly.

Includes:

  • update mechanism for new database column or functions (and update the functions if required)
  • handling of crashed workers by unlocking their jobs upon starting of a new worker
@jipiboily jipiboily referenced this pull request Mar 25, 2014
@ryandotsmith

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2014

Can you rebase this against master? It seems this branch has commits from #203 #204 and #205

Rakefile Outdated
@@ -7,7 +7,7 @@ require "./lib/queue_classic/tasks"
task :default => ['test']
Rake::TestTask.new do |t|
t.libs << 'test'
t.test_files = FileList['test/*_test.rb']
t.test_files = FileList['test/*_test.rb', 'test/lib/*_test.rb']

This comment has been minimized.

Copy link
@smathieu

smathieu Mar 25, 2014

Contributor
  t.test_files = FileList['test/**/*_test.rb']

Is better since you'll never have to play with this again.

if args[:connection]
@conn_adapter = ConnAdapter.new(args[:connection])
elsif QC.has_connection?
@conn_adapter = QC.default_conn_adapter

This comment has been minimized.

Copy link
@smathieu

smathieu Mar 25, 2014

Contributor

I think there's an implicit else here whre @conn_adapter would be nil. Is this a bug?

This comment has been minimized.

Copy link
@jipiboily

jipiboily Mar 25, 2014

Author Contributor

I did this on purpose, as there should be a connection in theory, but I can just make that a typical else.

readme.md Outdated
@@ -165,6 +169,25 @@ $ bundle exec rake qc:create
$ bundle exec rake qc:drop
```

## Upgrade

This comment has been minimized.

Copy link
@smathieu

smathieu Mar 25, 2014

Contributor

Should be "Upgrading form V2 to V3"

This comment has been minimized.

Copy link
@jipiboily

jipiboily Mar 25, 2014

Author Contributor

I meant it as a general way of doing upgrades, but being explicit can't hurt! Will change.

@@ -89,6 +96,12 @@ def lock_job
end
end

# This will unlock all jobs any postgres' PID that is not existing anymore
# to prevent any infinitely locked jobs
def unlock_jobs_of_dead_workers

This comment has been minimized.

Copy link
@smathieu

smathieu Mar 25, 2014

Contributor

No unit tests?

This comment has been minimized.

Copy link
@jipiboily

jipiboily Mar 25, 2014

Author Contributor

Much required, adding one.

This comment has been minimized.

Copy link
@jipiboily

jipiboily Mar 25, 2014

Author Contributor
@jipiboily

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2014

Rebase done. Adding a test.

@jipiboily jipiboily referenced this pull request Mar 25, 2014
@jipiboily

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2014

@ryandotsmith Had time to look at it and maybe play with it?

@ryandotsmith

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2014

@jipiboily I manually merged your fork and pushed the commits to master.

@jipiboily

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2014

❤️

@jipiboily jipiboily closed this Apr 4, 2014

@jipiboily jipiboily deleted the jipiboily:handle-crashes branch Apr 4, 2014

@brandur

This comment has been minimized.

Copy link

commented Apr 4, 2014

This is so boss! Thank-you for the very clean implementation.

@jipiboily

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2014

I think you can thank @smathieu for most of the reasoning behind it :)

That said, glad to have it working and getting nearer to 3.0 too!

@rappo

This comment has been minimized.

Copy link

commented Oct 15, 2014

I believe there's still a $200 bounty waiting to be claimed for this PR! https://www.bountysource.com/issues/1443334-handling-worker-failures/developers

@jipiboily

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2014

@rappo I know, but it's my employer and I did the work as part of my job, I should actually move the bounty to something else...thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.