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

Added support for pools using DSN #1808

Merged
merged 5 commits into from
Oct 21, 2020
Merged

Added support for pools using DSN #1808

merged 5 commits into from
Oct 21, 2020

Conversation

Nyholm
Copy link
Contributor

@Nyholm Nyholm commented Oct 11, 2020

I was about to update https://github.com/Happyr/elastica-dsn which basically adds DSN support to 6.x. But then I saw that 7.x will have support for DNS but not for multiple hosts.

Im sorry that I introduce a new dependency like this for such a small feature. I do need this feature in my app and I really wanted to try out nyholm/dsn:2.0 before I tag a stable release.

This will support any crazy DSN you may think of. See the full specification here: https://github.com/nyholm/dsn

Let me know what you think.


The PR is in draft as it should not be merged before nyholm/dsn:2.0 is released.

@ruflin
Copy link
Owner

ruflin commented Oct 14, 2020

Thanks for the addition. I'm always a bit on the fence about adding more external dependency at the same time, get rid of code to maintain is great. My initial reaction was that this might remove quite a bit of code on our end up it seems it is not the case. What else does the DSN lib to add as features to Elastica besides multi hosts or are we just missing multi hosts?

Do you know when 2.0 is planned to be shipped?

@Nyholm
Copy link
Contributor Author

Nyholm commented Oct 16, 2020

What else does the DSN lib to add as features to Elastica besides multi hosts or are we just missing multi hosts?

Not much. The usual. =)

  • Safer parsing with some error messages
  • The ability to do DSN functions like roundRobin(http://host1 http://host2)

My initial reaction was that this might remove quite a bit of code on our end up it seems it is not the case.

True. I wanted to keep BC. Ideally I would remove the connections key and force people to use a DSN function. That would also make it possible to validate the options to make sure the user dont misspell stuff.

Do you know when 2.0 is planned to be shipped?

I plan to release it as soon we are happy here.

Copy link
Owner

@ruflin ruflin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, I missed the most important bit that you are the owner of the lib 🤦‍♂️ That simplifies things of course as you are in control of it and the release.

I'm good with moving forward here. This PR would also need a changelog entry.

@Nyholm Nyholm marked this pull request as ready for review October 16, 2020 13:40
@Nyholm
Copy link
Contributor Author

Nyholm commented Oct 16, 2020

I've released 2.0, PR is ready for review.

@ruflin
Copy link
Owner

ruflin commented Oct 21, 2020

@Nyholm Just wanted to merge this but unfortunately there is now a conflict with master :-( Could you rebased / merge in master? Sorry for the delay on my end which probably caused this.

@Nyholm
Copy link
Contributor Author

Nyholm commented Oct 21, 2020

No worries.

PR is rebased. Tests should pass.

@ruflin ruflin merged commit 48cceb0 into ruflin:master Oct 21, 2020
@ruflin
Copy link
Owner

ruflin commented Oct 21, 2020

@Nyholm Thanks for the contribution!

@Nyholm
Copy link
Contributor Author

Nyholm commented Oct 21, 2020

Wohoo. Thank you for merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants