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

Allow calling code to manage connection disconnects #256

Merged
merged 1 commit into from
Nov 21, 2023

Conversation

b-ryan
Copy link
Contributor

@b-ryan b-ryan commented Nov 21, 2023

Resolves #255

The current implementation in this branch is not exactly what was discussed in #255. I got more familiar with the code and wanted to put something out there and discuss further.

The change in this branch appears to be the one that requires the fewest changes. The Database record now does not call disconnect* if :managed-connection? is set on the :db config.


Alternative that was discussed in #255 was to create a new implementation of Store. This would require:

  1. Adding a new method for proto/make-store that I guess would check for the :store to be :managed-database, example config:
{:store :managed-database                                   
 :properties {:env ["database.table"]               
              :map {:database {:user "bob"}}}       
 :db {:connection ...}}                         

(I haven't looked through all of the code to see what else might need to change to handle a new :store key.)
2. Most of the functionality inside the Database record would need to factored out into standalone functions that both Database and the new ManagedDatabase could call.


A second alternative that I thought of would be to introduce either a protocol or a multimethod that is used by the disconnect* function (and perhaps for consistency, connect* as well).

I'm currently leaning towards doing this, since it will mean the disconnect* function will be safe to use in the future, without the calling code needing to worry about whether the disconnect should really happen.

@yogthos
Copy link
Owner

yogthos commented Nov 21, 2023

@ieugen I recall we chatted about adding this a little while back, so just looping you in on this

@yogthos yogthos merged commit 9f9e241 into yogthos:master Nov 21, 2023
@yogthos
Copy link
Owner

yogthos commented Nov 21, 2023

hmm does look like we're seeing some test failures with the new changes

@b-ryan
Copy link
Contributor Author

b-ryan commented Nov 21, 2023

Hey sorry - I should have marked this as WIP. Didn't do much testing, as I mostly wanted to confirm the direction. I can take a look at failures now and maybe introduce additional tests.

@b-ryan b-ryan deleted the b-ryan/managed-connections branch November 21, 2023 15:04
@yogthos
Copy link
Owner

yogthos commented Nov 21, 2023

Ah no worries, just ping me when you have a chance to take a look at that.

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.

Allow managing the connection state from calling code
2 participants