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

switch from redis-async to redis-rs #148

Closed
wants to merge 2 commits into from
Closed

switch from redis-async to redis-rs #148

wants to merge 2 commits into from

Conversation

arniu
Copy link
Contributor

@arniu arniu commented Jan 13, 2021

PR Type

Feature

PR Checklist

Check your PR fulfills the following:

  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • A changelog entry has been made for the appropriate packages.
  • Format code with the latest stable rustfmt

Overview

Roadmap:

  • Add redis_cmd!
  • Make Command private
  • Switch to redis-rs

Resolve #30
Resolve #46

@arniu arniu changed the title Add redis_cmd! macro to compose Redis commands switch from redis-async to redis-rs Jan 13, 2021
@robjtede robjtede added A-redis Project: acitx-redis B-semver-major breaking change requiring a major version bump labels Jan 13, 2021
@arniu arniu marked this pull request as draft January 13, 2021 10:25
@fakeshadow
Copy link
Contributor

I don't see benefit moving to redis-rs.
redis-async is pretty light and shares all deps with almost any actix-web app.

@arniu
Copy link
Contributor Author

arniu commented Jan 17, 2021

I don't see benefit moving to redis-rs.
redis-async is pretty light and shares all deps with almost any actix-web app.

Yes, redis-async is much lighter. But it does NOT support SSL or auth.

@fakeshadow
Copy link
Contributor

I don't think that's highly desired feature everyone want and people can implement their own solution for that.

That said I'm not against it for additional feature.

@fakeshadow
Copy link
Contributor

fakeshadow commented Mar 22, 2021

Update:

What this PR claim is not complete true. redis-async's client object could not supporting tls and auth(I did not do a full check on the crate so I could be wrong). But actix-redis does not actually use it's client object. actix-redis start a raw tcp connection with redis server and only use types redis-async offer to pack command and unpack response.

Therefore it would very possible to add tls and authentication support on top of the current code. I'm working on tls feature right now and it connect to my local tls enabled redis-server just fine

@fakeshadow
Copy link
Contributor

Update2:
Auth is already supported by sending command as Command(resp_array!["AUTH", username, password]). So it's not an issue from the beginning.
This command can be called directly from actix-redis though for a fool proof authentication

@robjtede robjtede closed this Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-redis Project: acitx-redis B-semver-major breaking change requiring a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

actix-redis connect to the redis address with password switch from redis-async to redis-rs
3 participants