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

Support Ecto Repo #38

Closed
lpil opened this issue Dec 10, 2018 · 9 comments
Closed

Support Ecto Repo #38

lpil opened this issue Dec 10, 2018 · 9 comments
Labels
enhancement New feature or request

Comments

@lpil
Copy link
Collaborator

lpil commented Dec 10, 2018

Hello!

Ecto has a nice sandbox that allows us to run database hitting tests concurrently when using Ecto's connection pool.

It'd be nice to be able to use this with Rihanna. Currently the library is hard-coded to use a dedicated Postgrex connection, however if the user was able to pass a connection the user could pass an Ecto repo and thus make use of the sandbox.

This would also remove the need for the Rihanna.Job.Postgrex in many cases, halving the number of database connections that Rihanna uses. Configuration could be added to prevent this process from being started if desired.

In addition it would also enable the user to enqueue more jobs and perform business login with the same SQL transaction as the connection will be shared across both automatically with Ecto, enabling a little more safety.

Here's some prior art on adding Ecto integration to a SQL executing lib without coupling tightly lpil/yesql@e692828

I will implement this feature if given the thumbs up.

Cheers,
Louis

@lpil
Copy link
Collaborator Author

lpil commented Dec 10, 2018

It could be specified via mix config, as with the other configuration in Rihanna.

By default it continues to behave as it does today, however this config could be specified like so:

# A named postgrex connection is used
config :rihanna,
  enqueue_postgres_connection: {Postgrex, :my_postgrex_connection}
# An Ecto connection pool is used
config :rihanna,
  enqueue_postgres_connection: {Ecto, My.Repo}

In either of the above cases the Rihanna.Job.Postgrex process is not started.

@lpil lpil changed the title Support Ecto Postgres sandbox Support Ecto Repo Dec 10, 2018
@samsondav
Copy link
Owner

Interesting idea. From what I understand, this is only for the process that enqueues jobs, is that correct?

@lpil
Copy link
Collaborator Author

lpil commented Dec 10, 2018

What do you mean? :)

@samsondav
Copy link
Owner

I mean, it's for Rihanna.enqueue - not the dispatcher?

@samsondav samsondav reopened this Dec 10, 2018
@lpil
Copy link
Collaborator Author

lpil commented Dec 10, 2018

Yes, it's only for enqueuing.

Ecto doesn't offer a way to always request the same connection from the pool so the dispatcher so the advisory locks wouldn't work. I believe it is possible to check out a connection long term with Ecto, but this is effectively the same as having a dedicated Postgrex connection so I'm not sure I see an advantage.

@samsondav
Copy link
Owner

This seems like a good idea. Feel free to implement :)

@lpil
Copy link
Collaborator Author

lpil commented Dec 10, 2018

Do you like the config design above?

@samsondav
Copy link
Owner

Yeah it looks ok.

@lpil
Copy link
Collaborator Author

lpil commented Dec 10, 2018

It could be passed at runtime for sure.

I suggested that API because Rihanna doesn't currently accept configuration beyond the Postgrex config at runtime so I thought that would be more in keeping with the current API.

@lpil lpil added the enhancement New feature or request label Dec 10, 2018
@ghost ghost mentioned this issue Feb 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants