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.

Sign up for GitHub

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

Jump to bottom

Check both 'nil' and 'false' in redis.call return value #64

Merged
merged 1 commit into from
Mar 3, 2017
Merged

Check both 'nil' and 'false' in redis.call return value #64

merged 1 commit into from
Mar 3, 2017

Conversation

diesse
Copy link

@diesse diesse commented Jun 21, 2016

In redis 3.2, lua script evaluates redis 'nil' value as 'false' instead of 'nil'
This causes lua errors as described in #63, since testing for 'nil' fails and empty parameters can be passed to functions

@neilmb
Copy link

neilmb commented Jun 21, 2016

This looks good to me, but we don't have an automated way of testing on Redis 3.2. Did you run make test locally with the new Redis?

@b4hand Any thoughts on getting another Redis version into the Travis build matrix?

@diesse
Copy link
Author

diesse commented Jun 21, 2016

Yes, all 240 tests completed successfully ( on Redis 3.2.1 )

@dlecocq
Copy link

dlecocq commented Jun 21, 2016

We should totally build matrix the Redis version.

@pintsized
Copy link

Can this be merged or is it waiting on further testing?

@pintsized pintsized mentioned this pull request Sep 16, 2016
Closed
@andrewblim andrewblim mentioned this pull request Oct 21, 2016
Merged
@andrewblim
Copy link

I'll echo @pintsized's questions, I'd be in favor of merging this unless others have objections

@bkirz bkirz mentioned this pull request Mar 3, 2017
Merged
@bkirz
Copy link
Contributor

bkirz commented Mar 3, 2017

Thanks for this fix, and apologies for letting it languish for so long! I've merged #68, which adds redis 3.2.7 to our build matrix. I've rebased this branch on top of that change and have confirmed that this fixes the 3.2.7 tests. Here's the corresponding travis build: https://travis-ci.org/seomoz/qless-core/builds/207219314

Thanks again!

@bkirz bkirz merged commit eed379c into seomoz:master Mar 3, 2017
@bkirz bkirz mentioned this pull request Mar 3, 2017
Merged
@diesse diesse deleted the redis32-nil-fix branch August 1, 2017 14:53
jshorty added a commit to custora/qless that referenced this pull request Aug 4, 2017
@jshorty
Use upstream qless-core again
4ab0454 
First step into reconciling with mainline qless. See #3 for why we created our own repo for qless-core. The changes we were adding there ahead of Moz are now merged into the mainline (seomoz/qless-core#64). I tested qless on staging yesterday and ran jobs with seomoz/qless-core without issues.
@jshorty jshorty mentioned this pull request Aug 4, 2017
Merged
oggy pushed a commit to custora/qless that referenced this pull request Aug 4, 2017
@jshorty @oggy
Use upstream qless-core again
3539031 
First step into reconciling with mainline qless. See #3 for why we created our own repo for qless-core. The changes we were adding there ahead of Moz are now merged into the mainline (seomoz/qless-core#64). I tested qless on staging yesterday and ran jobs with seomoz/qless-core without issues.
@bslobodin bslobodin mentioned this pull request Sep 6, 2017
Merged
tom93 added a commit to tom93/nztrain that referenced this pull request Feb 3, 2019
@tom93
Workaround for missing test/unit/assertions when running specs
44caf13 
Now most of the tests pass.

Submissions fail with the message "Lua redis() command arguments must be
strings or integers" because of incompatible qless and redis versions
(seomoz/qless-core#64).
tom93 added a commit to tom93/nztrain that referenced this pull request Feb 3, 2019
@tom93
Update qless from 0.10.3 to 0.12.0
a191d94 
The old version of qless is incompatible with newer versions of Redis,
see seomoz/qless-core#64.

Update using 'bundle update --conservative qless' to avoid updating
other gems if possible.

Submissions caused the following error:

Redis::CommandError - ERR Error running script (call to ...): @user_script:925: @user_script: 925: Lua redis() command arguments must be strings or integers:
  app/workers/application_worker.rb:6:in `put'
  app/workers/judge_submission_worker.rb:24:in `judge'
  app/models/submission.rb:121:in `judge'
  app/controllers/problems_controller.rb:90:in `block in submit'
  app/controllers/problems_controller.rb:88:in `submit'
  lib/rack/response_timer.rb:12:in `_call'
  lib/rack/response_timer.rb:7:in `call'

Now all all the tests pass.
@evanbattaglia evanbattaglia mentioned this pull request Feb 25, 2019
Open
tom93 added a commit to tom93/nztrain that referenced this pull request Mar 27, 2019
@tom93
Workaround for missing test/unit/assertions when running specs
240cc1c 
Now most of the tests pass.

Submissions fail with the message "Lua redis() command arguments must be
strings or integers" because of incompatible qless and redis versions
(seomoz/qless-core#64).
tom93 added a commit to tom93/nztrain that referenced this pull request Mar 27, 2019
@tom93
Update qless from 0.10.3 to 0.12.0
9c235bc 
The old version of qless is incompatible with newer versions of Redis,
see seomoz/qless-core#64.

Update using 'bundle update --conservative qless' to avoid updating
other gems if possible.

Submissions caused the following error:

Redis::CommandError - ERR Error running script (call to ...): @user_script:925: @user_script: 925: Lua redis() command arguments must be strings or integers:
  app/workers/application_worker.rb:6:in `put'
  app/workers/judge_submission_worker.rb:24:in `judge'
  app/models/submission.rb:121:in `judge'
  app/controllers/problems_controller.rb:90:in `block in submit'
  app/controllers/problems_controller.rb:88:in `submit'
  lib/rack/response_timer.rb:12:in `_call'
  lib/rack/response_timer.rb:7:in `call'

Now all all the tests pass.
Techno-coder pushed a commit to Techno-coder/nztrain that referenced this pull request Feb 15, 2020
@tom93 @Techno-coder
Workaround for missing test/unit/assertions when running specs
3012a65 
Now most of the tests pass.

Submissions fail with the message "Lua redis() command arguments must be
strings or integers" because of incompatible qless and redis versions
(seomoz/qless-core#64).
Techno-coder pushed a commit to Techno-coder/nztrain that referenced this pull request Feb 15, 2020
@tom93 @Techno-coder
Workaround for missing test/unit/assertions when running specs
9c56217 
Now most of the tests pass.

Submissions fail with the message "Lua redis() command arguments must be
strings or integers" because of incompatible qless and redis versions
(seomoz/qless-core#64).
Techno-coder pushed a commit to Techno-coder/nztrain that referenced this pull request Feb 15, 2020
@tom93 @Techno-coder
Update qless from 0.10.3 to 0.12.0
97eccc3 
The old version of qless is incompatible with newer versions of Redis,
see seomoz/qless-core#64.

Update using 'bundle update --conservative qless' to avoid updating
other gems if possible.

Submissions caused the following error:

Redis::CommandError - ERR Error running script (call to ...): @user_script:925: @user_script: 925: Lua redis() command arguments must be strings or integers:
  app/workers/application_worker.rb:6:in `put'
  app/workers/judge_submission_worker.rb:24:in `judge'
  app/models/submission.rb:121:in `judge'
  app/controllers/problems_controller.rb:90:in `block in submit'
  app/controllers/problems_controller.rb:88:in `submit'
  lib/rack/response_timer.rb:12:in `_call'
  lib/rack/response_timer.rb:7:in `call'

Now all all the tests pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@diesse @neilmb @dlecocq @pintsized @andrewblim @bkirz