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

Delegate redis connection build to redix #61

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

thiamsantos
Copy link
Contributor

@thiamsantos thiamsantos commented Sep 26, 2022

Motivation

Recently I was setting up phoenix_pubsub_redis in our application, and I incurred in a few problems connecting to ElastiCache redis. The most confusing thing was that I was able to connect to redis using Redix.start_link normally, but if I used the same opts with phoenix_pubsub_redis it would fail. In the end the problem was that the option username is been ignored.

Looking in the repository we have a few other open and closed PRs with similar issues.
Updating the code just to add behaviour on the parsing of redis urls or allowing new options that redix supports.

Proposed Solution

I would like to propose delegating the handle of redis connection opts directly to redis,

  • so we don't have to maintain a version of the url parsing inside of phoenix_pubsub_redis
  • remember to always have a updated version of the opts supported by redix inside the codebase

We could do that by introducing a new option called redis_opts and then pass that option directly to redix, and soft-deprecate the old option (host, password etc).
In order to keep backward compatibility we could continue supporting the existing options, but encourage the use of the new redis_opts.

In addition to that we can start using the url parsing directly from redix. Currently the function Redix.URI.opts_from_uri/1 is private. I plan to open a discussion on redix on the best way to pass this url to redix, so maybe we can remove this responsibility completely from the codebase.

Does the approach makes sense?

@thiamsantos thiamsantos changed the title Delegate redis connection build to redix WIP Delegate redis connection build to redix Sep 26, 2022
@thiamsantos thiamsantos changed the title WIP Delegate redis connection build to redix Delegate redis connection build to redix Sep 26, 2022
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.

None yet

1 participant