Skip to content

[SPARK-23186][SQL] Initialize DriverManager first before loading JDBC Drivers #20359

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.

Sign up for GitHub

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
dongjoon-hyun wants to merge 2 commits into from
Closed

[SPARK-23186][SQL] Initialize DriverManager first before loading JDBC Drivers #20359

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
234a637
[SPARK-23186][SQL] Initialize DriverManager first before loading Drivers
dongjoon-hyun Jan 23, 2018
52e6f19
Update comments.
dongjoon-hyun Feb 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/DriverRegistry.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ import org.apache.spark.util.Utils
*/
object DriverRegistry extends Logging {

// Initialize DriverManager first to prevent potential deadlocks between DriverManager and Driver
Copy link
Contributor

@cloud-fan cloud-fan Jan 25, 2018

Choose a reason for hiding this comment

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

We need to say more about why this can avoid deadlock.

Copy link
Contributor

@cloud-fan cloud-fan Jan 25, 2018

Choose a reason for hiding this comment

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

we can copy something from the storm PR: https://github.com/apache/storm/pull/2134/files

DriverManager.getDrivers
Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jan 23, 2018

Choose a reason for hiding this comment

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

Hi, @rxin .
Could you give me some comments on this?

Copy link
Member

@gatorsmile gatorsmile Jan 24, 2018

Choose a reason for hiding this comment

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

More context about this change? Based on your PR description, this is to resolve the deadlocks among executors? How does it work after applying this change?

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jan 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It's the same situation like STORM in the PR description and this occurs in Spark, too.
In the Spark executor, the stacks shows the deadlock between DriverManager and Driver.

  • Pheonix library call DriverManager.loadInitialDrivers()
  • Spark DriverRegistry call PhoenixDriver constructor before DriverManager created.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jan 24, 2018

Choose a reason for hiding this comment

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

Unfortunately, so far, I only have this log only. It's difficult to reproduce this deadlock.

Copy link
Member

@gatorsmile gatorsmile Jan 24, 2018

Choose a reason for hiding this comment

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

How did you test it?

Copy link
Member

@gatorsmile gatorsmile Jan 24, 2018

Choose a reason for hiding this comment

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

We need a test; otherwise, it is not the right thing to merge such a PR.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jan 24, 2018

Choose a reason for hiding this comment

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

Do you mean a unit test case?

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jan 24, 2018

Choose a reason for hiding this comment

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

I'm wondering if you can merge this PR if I can test this patch in somewhere of our customer cluster. :)

Copy link
Contributor

@cloud-fan cloud-fan Jan 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

if it's too hard to write a UT, I think a manual test is also fine.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jan 25, 2018

Choose a reason for hiding this comment

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

Thank you for review, @cloud-fan .
So far, this deadlock situation is reported intermittently without any logs.


private val wrapperMap: mutable.Map[String, DriverWrapper] = mutable.Map.empty

def register(className: String): Unit = {
Expand Down