Skip to content

ZEPPELIN-487 Change supporting multiple statements to multiple connections - #517

Closed
jongyoul wants to merge 3 commits into
apache:masterfrom
jongyoul:ZEPPELIN-487
Closed

ZEPPELIN-487 Change supporting multiple statements to multiple connections#517
jongyoul wants to merge 3 commits into
apache:masterfrom
jongyoul:ZEPPELIN-487

Conversation

@jongyoul

Copy link
Copy Markdown
Member

Changed multiple statements to multiple connection. Some JDBC don't support parallel executions with one connection.

- keyConnectionMap -> propertyKeyConnectionMap
- Multiple Statements -> multiple connections
@prasadwagle

Copy link
Copy Markdown

I tested the pull request and it allowed Vertica queries to execute in parallel with one modification. Thanks!

I had to comment:
// if (statement.isClosed()) {
// connection = getConnection(propertyKey);
// statement = connection.createStatement();
// }
since I ran into:
java.lang.AbstractMethodError: com.vertica.jdbc.VerticaStatementImpl.isClosed()Z
at org.apache.zeppelin.hive.HiveInterpreter.getStatement(HiveInterpreter.java:208)

Can you help me understand the need for the statement.isClosed() check in line 208 since the statement is created in line 207?

@jongyoul

Copy link
Copy Markdown
MemberAuthor

It's soft of defensive code for avoiding error. Do you think it's redundant? I'll fix this code more general :-). Thanks for the review quickly.

- Fix the error when statement doesn't support isClosed method
@jongyouljongyoul closed this Dec 7, 2015
@jongyouljongyoul reopened this Dec 7, 2015
@jongyoul

Copy link
Copy Markdown
MemberAuthor

@praagarw Could you please review this PR again?

@prasadwagle

Copy link
Copy Markdown

Hi Jongyoul, since createStatement is implemented in the JDBC drivers, I think it is unlikely it will return a closed statement. However, I am all for defensive coding and your fix in ZEPPELIN-487 looks good to me.

@jongyoul

Copy link
Copy Markdown
MemberAuthor

@praagarw Sure, but I couldn't guarantee all of jdbc connections create statement when it's alive. Thus double check might be helpful in that case. And thanks for kind review. I'm merging in three day.

@jongyoul

Copy link
Copy Markdown
MemberAuthor

Merging without additional discussion

@jongyouljongyoul mentioned this pull request Dec 15, 2015
Sign up for freeto 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.

2 participants

@jongyoul@prasadwagle