Skip to content

[SPARK-12048][SQL] Prevent to close JDBC resources twice #10101

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

Closed
wants to merge 1 commit into from

Conversation

rh99
Copy link

@rh99 rh99 commented Dec 2, 2015

No description provided.

@@ -509,6 +509,7 @@ private[sql] class JDBCRDD(
} catch {
case e: Exception => logWarning("Exception closing connection", e)
}
closed = true
Copy link
Member

Choose a reason for hiding this comment

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

I think this should go at the top, right after it is checked. My reasoning is that if close fails, it would let itself be called again, and that's not obviously helpful, I think.

@srowen
Copy link
Member

srowen commented Dec 4, 2015

@rh99 are you still working on this? seems like a good fix but I have one other comment here.

@rh99
Copy link
Author

rh99 commented Dec 4, 2015

All exceptions are caught anyway, so it will always reach the end (if there is no Error).
My reasoning was: If the code was changed, and it would really throw an exception somewhere in the middle, the location at the end is better, since the variable "closed" should only be true, if all JDBC resources have been closed. Otherwise the variable should have a name like "guard". Anyway either location is fine. Just tell me if I should move it.

@srowen
Copy link
Member

srowen commented Dec 4, 2015

Throwables aren't caught though. I suppose I don't feel strongly about it as you can argue a benefit either way. Any other ideas out there? otherwise will merge soonish.

@SparkQA
Copy link

SparkQA commented Dec 6, 2015

Test build #2174 has finished for PR 10101 at commit 4289a44.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

asfgit pushed a commit that referenced this pull request Dec 6, 2015
Author: gcc <spark-src@condor.rhaag.ip>

Closes #10101 from rh99/master.

(cherry picked from commit 04b6799)
Signed-off-by: Sean Owen <sowen@cloudera.com>
asfgit pushed a commit that referenced this pull request Dec 6, 2015
Author: gcc <spark-src@condor.rhaag.ip>

Closes #10101 from rh99/master.

(cherry picked from commit 04b6799)
Signed-off-by: Sean Owen <sowen@cloudera.com>
@srowen
Copy link
Member

srowen commented Dec 6, 2015

Merged to master/1.6/1.5

@asfgit asfgit closed this in 04b6799 Dec 6, 2015
@rh99
Copy link
Author

rh99 commented Dec 7, 2015

Thanks for including that fix so fast. I have just tested to fix in the master successfully.

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.

3 participants