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 for #168. Handle the NOSCRIPT by sending the script again #178

Merged
merged 2 commits into from Apr 11, 2016

Conversation

otzy007
Copy link
Contributor

@otzy007 otzy007 commented Apr 4, 2016

Run EVAL again if NOSCRIPT error happens. This will fix #168
Based on the documentation from http://redis.io/commands/EVAL#bandwidth-and-evalsha:
The client library implementation can always optimistically send EVALSHA under the hood even when the client actually calls EVAL, in the hope the script was already seen by the server. If the NOSCRIPT error is returned EVAL will be used instead.

@mhenrixon
Copy link
Owner

Thank you!

Could you fix the errors? Not sure if this is your fault but I am reluctant to merge anything that doesn't pass the tests and style violation. Would love to get this released! 😄

@otzy007 otzy007 closed this Apr 6, 2016
@otzy007 otzy007 reopened this Apr 6, 2016
…n order to avoid the rubocop too many lines error
@otzy007 otzy007 closed this Apr 6, 2016
@otzy007 otzy007 reopened this Apr 6, 2016
@otzy007 otzy007 closed this Apr 7, 2016
@otzy007 otzy007 reopened this Apr 7, 2016
@otzy007
Copy link
Contributor Author

otzy007 commented Apr 7, 2016

I've fixed the rubocop's method is too long offense.
There's one more from the master branch

spec/support/sidekiq_meta.rb:20:9: C: Avoid modifier if after another conditional.
    end if version && operator
        ^^

What do we do with it?

@mhenrixon
Copy link
Owner

Lets just ignore that one for now. Personally really like those types of if statements so just ignore that error until I consider how to handle it properly.

@otzy007
Copy link
Contributor Author

otzy007 commented Apr 7, 2016

hmm. Ok.

this

  unless Sidekiq::VERSION.send(operator, version)
      skip('Skipped due to version check (requirement was that sidekiq version is ' \
           "#{operator} #{version}; was #{Sidekiq::VERSION})")
    end if version && operator

could be changed to just this?

   if version && operator && Sidekiq::VERSION.send(operator, version).nil?
      skip('Skipped due to version check (requirement was that sidekiq version is ' \
           "#{operator} #{version}; was #{Sidekiq::VERSION})")
    end

Tests are passing with it

@mhenrixon
Copy link
Owner

Sounds good to me!

@otzy007
Copy link
Contributor Author

otzy007 commented Apr 7, 2016

Ok. I'll open later another PR with the unless change
This one is ready to be merged 😀

@mhenrixon mhenrixon merged commit 97a3ee6 into mhenrixon:master Apr 11, 2016
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.

NOSCRIPT No matching script. Please use EVAL.
2 participants