Skip to content

JDBC bugfix: Don't reuse a defunct databaseMetaData#2660

Merged
michael-berlin merged 2 commits intovitessio:masterfrom
HubSpot:defunct_metadata
Mar 24, 2017
Merged

JDBC bugfix: Don't reuse a defunct databaseMetaData#2660
michael-berlin merged 2 commits intovitessio:masterfrom
HubSpot:defunct_metadata

Conversation

@bbeaudreault
Copy link
Copy Markdown
Contributor

@bbeaudreault bbeaudreault commented Mar 23, 2017

I'm not sure why databaseMetaData is a static variable. It looks to have been that way since the very beginning. But I recently ran into an issue where if you create multiple VitessConnections and close the first one, all of the other VitessConnections reference what is effectively a defunct databaseMetadata. That is, the databaseMetadata references a connection which is closed, and so nothing can be called on it.

This is very easily reproducible for anyone who uses hikari or other jdbc pooling libraries.

This fixes to ensure we don't re-use such databaseMetadata values. I'd also be happy to make this non-static, if preferred.

@harshit-gangal @ashudeep-sharma

@bbeaudreault bbeaudreault force-pushed the defunct_metadata branch 4 times, most recently from b0e52c7 to 18166e2 Compare March 23, 2017 02:05
@ashudeep-sharma-zz
Copy link
Copy Markdown
Contributor

The reason why we made the databaseMetadata as static is because we didn't want to fire the show queries for every connection.


Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessConnection.java, line 235 at r1 (raw file):

    }

    private boolean metadataNullOrDefunct() throws SQLException {

Please rename this function to metadataNullOrDefunctional().


Comments from Reviewable

@ashudeep-sharma-zz
Copy link
Copy Markdown
Contributor

Just a Minor Comment, Rest Looks Good to Me.

@bbeaudreault
Copy link
Copy Markdown
Contributor Author

bbeaudreault commented Mar 24, 2017

Thanks! Defunctional is not a word, so I renamed to Closed instead -- metadataNullOrClosed

@ashudeep-sharma-zz
Copy link
Copy Markdown
Contributor

Works for me.
:lgtm:


Reviewed 1 of 1 files at r1.
Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@ashudeep-sharma-zz
Copy link
Copy Markdown
Contributor

Thank you so much for fixing it.


Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@bbeaudreault
Copy link
Copy Markdown
Contributor Author

@sougou @alainjobart @michael-berlin This got a LGTM. Could one of you merge when you get a chance? Looking to submit a dependent PR today. Thanks!

@michael-berlin michael-berlin merged commit d6b9a38 into vitessio:master Mar 24, 2017
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.

4 participants