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

Add method to close and unregister a database connection [DRAFT 1] #529

Merged
merged 1 commit into from
Mar 25, 2019
Merged

Add method to close and unregister a database connection [DRAFT 1] #529

merged 1 commit into from
Mar 25, 2019

Conversation

Maxr1998
Copy link
Contributor

This is my first draft that would close #523, the only issue I currently see is when you'd add the same TransactionManager for multiple databases and then removed it (it remove the first occurrence from the List, but not necessarily the right one); or register another TransactionManager for the same database, basically creating a "dead" manager inside the list, this issue should be easily fixable though.

Looking forward to your opinions on my PR.

@Tapac
Copy link
Contributor

Tapac commented Mar 24, 2019

Is it really necessary to add deque instead of a simple @Synchronized and setting NotInitializedManager to _manager when the last manager was unregistered?

@Maxr1998
Copy link
Contributor Author

Good question. I thought it'd make sense to revert to the previous TransactionManager explicitly, but we could also ofc reset it to NotInitializedManager. What do you think?

@Maxr1998 Maxr1998 changed the title Add method to close and unregister a database connection Add method to close and unregister a database connection [DRAFT 1] Mar 24, 2019
@Tapac
Copy link
Contributor

Tapac commented Mar 25, 2019

I missed the point that you were trying to restore the previous manager and now I think that it was a bad advise to set NotInitializedManager. Maybe it's better to use any (first?) manager from the registeredDatabases map?

@Maxr1998
Copy link
Contributor Author

Getting a random element from the registeredDatabases isn't a really pretty solution in my opinion, additionally unintuitive and not well-defined.. We could also add another private volatile var that saves the first TransactionManager, and is set to NotInitializedManager if that one is (finally) removed, however, there might be other TransactionManagers left in the map. So, that solution might also be counter-intuitive for users of the library.

I fear there's not really a good way around using the Deque to have the TransactionManagers in a certain order..

@Tapac
Copy link
Contributor

Tapac commented Mar 25, 2019

I guess that a deque is not such a bad decision. So I will approve that PR and reject another one.
Thank you.

@Tapac Tapac merged commit bebc719 into JetBrains:master Mar 25, 2019
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.

How can I disconnect a connected database?
2 participants