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

Fix multiple bugs, cleaned up dependencies, and added a feature #4

Merged
merged 5 commits into from Sep 27, 2012
Merged

Fix multiple bugs, cleaned up dependencies, and added a feature #4

merged 5 commits into from Sep 27, 2012

Conversation

kemper-blinq
Copy link

We found that the gem you published would not release processed jobs
so they could be enqueued a second time. The problem was that the data
that was used to generate the md5 value in the server middleware was
not the same as the client middleware. We fixed that and added a test
with both of them interacting.

The way that hashing was done was insufficient as well; it would try
to hash based on just the arguments passed to sidekiq. Without
considering class name and queue name, you can never be sure that the
job is unique. We changed the hash logic to consider class, queue, and
args.

You had added logic that created very large expiration values for the
keys in redis for delayed/scheduled sidekiq jobs. Not sure what you
had in mind there, but made it so that the key expires
HASH_KEY_EXPIRATION seconds after it's scheduled to run. This makes it
comparable to what happens when you enqueue a job to be processed
immediately.

The final fix/cleanup was that the gem was written to require too
much. The gemfile duplicated dependencies of sidekiq that don't need
to be declared, and the middleware.rb file required sidekiq components
that would actually load the middleware files. This causes problems.
For instance, the middleware loads celluloid, and celluloid starts a
worker thread. That worker thread causes the exit code of specs to
always be successful making our ci server always report passing specs.
We cleaned up those dependency/load issues.

We also added the ability to use a value other than
HASH_KEY_EXPIRATION when configuring a job. This is passed in as a
sidekiq_option called 'unique_job_expiration'. We updated the readme
to reflect that option.

Unfortunately, one of the specs we added fails periodically; I'm not sure why, but it seems like the multi command causes some wierdness. I didn't really change that code, but I'm hoping it's not a sign of a larger problem.

Andrew Riley and Kris Kemper and others added 5 commits September 20, 2012 11:26
We also made the hashing consider class and queue, which
is needed to really determine whether a job is unique.
… job

Before, it was multiplying 24 * 60 * 60 to the number of seconds between
'Time.now' and when the job was scheduled to run, which resulted in a very
large expiration value.
Those gems are dependencies of sidekiq, and sidekiq is a dependency
this gem, so they'll be resolved correctly without being declared in
this projects Gemfile
That provides the ability to register middleware without causing
celluloid to run an actor thread.
mhenrixon added a commit that referenced this pull request Sep 27, 2012
Fix multiple bugs, cleaned up dependencies, and added a feature
@mhenrixon mhenrixon merged commit c1528c9 into mhenrixon:master Sep 27, 2012
@mhenrixon
Copy link
Owner

Odd but I don't see any real problems. Merging this in and releasing a new version while I try to debug this problem.

@mhenrixon
Copy link
Owner

Seems to be a timing issue of some sort. Putting the higher the sleep (n) the higher the success rate. Going ahead and releasing a new version.

@kemper-blinq
Copy link
Author

Yeah, I tried using sleeps all over the place and couldn't pin it down. I
like the test conceptually though, so I decided to keep it in, but comment
it so that people know it has some issues.

@mhenrixon
Copy link
Owner

Thanks a bunch for the awesome work on this. Really appreciated!

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

Successfully merging this pull request may close these issues.

2 participants