Delegate redis connection build to redix#61
Closed
thiamsantos wants to merge 6 commits intophoenixframework:masterfrom
Closed
Delegate redis connection build to redix#61thiamsantos wants to merge 6 commits intophoenixframework:masterfrom
thiamsantos wants to merge 6 commits intophoenixframework:masterfrom
Conversation
Collaborator
|
Thanks for the PR, I've incorporated many of the changes here in #76! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Recently I was setting up
phoenix_pubsub_redisin 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 usingRedix.start_linknormally, but if I used the same opts withphoenix_pubsub_redisit would fail. In the end the problem was that the optionusernameis 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
redixsupports.:sentinelis set #60Proposed Solution
I would like to propose delegating the handle of redis connection opts directly to redis,
phoenix_pubsub_redisWe could do that by introducing a new option called
redis_optsand 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 functionRedix.URI.opts_from_uri/1is private. I plan to open a discussion onredixon the best way to pass thisurltoredix, so maybe we can remove this responsibility completely from the codebase.Does the approach makes sense?