[ZEPPELIN-1988] add property "precode" to JDBCInterpreter by tinkoff-dwh · Pull Request #2078 · apache/zeppelin

tinkoff-dwh · 2017-02-28T05:06:57Z

What is this PR for?

Adds property "precode". Value of property contains SQL which executes while opening connection.

What type of PR is it?

Improvement

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-1988

How should this be tested?

Set property zeppelin.interpreter.precode =set search_path='test, public'
Execute %jdbc show search_path

Questions:

Does the licenses files need update? no
Is there breaking changes for older versions? no
Does this needs documentation? no

tinkoff-dwh · 2017-02-28T05:12:11Z

CI failed, errors in unrelated tests
https://travis-ci.org/tinkoff-dwh/zeppelin/builds/205735485

andrykrp · 2017-02-28T06:35:57Z

LGTM

sotnich · 2017-02-28T07:06:23Z

Looks Good To Me

felixcheung · 2017-02-28T08:45:12Z

    <td>jceks credential key</td>
  </tr>
+  <tr>
+    <td>zeppelin.interpreter.precode</td>


this is specifically for the jdbc interpreter? if so, suggest name zeppelin.jdbc.precode or zeppelin.jdbc.sql.precode

in planning for the future add this property to another interpreters (PySpark, SparkR)

I agree @felixcheung's opinion. If you want to extend this feature for the future, you can modify it when you do it.

felixcheung · 2017-02-28T08:46:06Z

+          statement.close();
+        }
+      } catch (SQLException e) {
+        logger.error("Cannot create precode statement", e);


this is going to log but ignore the error - is this what we want here?

Yes, I want the main code will run anyway. Although it may be worth it to generate an error

agree, for the user it is more logical to see the error, fix it

felixcheung · 2017-02-28T08:47:18Z


  public static final String ZEPPELIN_INTERPRETER_HOST = "zeppelin.interpreter.host";

+  public static final String ZEPPELIN_PRECODE_PROPERTY_KEY = "zeppelin.interpreter.precode";


I'm not sure this should go here if this is only for one interpreter?

in planning for the future add this property to another interpreters (PySpark, SparkR)

DrIgor · 2017-02-28T09:32:08Z

+    String precode = getProperty(ZEPPELIN_PRECODE_PROPERTY_KEY);
+    if (StringUtils.isNotEmpty(precode)) {
+      logger.info("Run SQL precode '{}'", precode);
+      try {


Connection and Statement are AutoCloseable. Isn't it better to use try-with-resources?

jongyoul · 2017-02-28T10:14:43Z

 import org.apache.hadoop.security.alias.CredentialProvider;
 import org.apache.hadoop.security.alias.CredentialProviderFactory;
 import org.apache.thrift.transport.TTransportException;
-import org.apache.zeppelin.interpreter.*;


I like this kind of change.

tinkoff-dwh · 2017-03-01T06:49:41Z

@jongyoul @DrIgor @felixcheung Thx for review.
CI green, https://travis-ci.org/tinkoff-dwh/zeppelin/builds/206490151

felixcheung · 2017-03-01T06:51:08Z


+  private void executePrecode(Connection connection) throws SQLException {
+    String precode = getProperty(ZEPPELIN_JDBC_PRECODE_KEY);
+    if (StringUtils.isNotEmpty(precode)) {


if (StringUtils.isNotBlank(precode)) { precode = StringUtils.trim(precode); ....

tinkoff-dwh · 2017-03-01T11:05:52Z

@felixcheung
CI green, https://travis-ci.org/tinkoff-dwh/zeppelin/builds/206525642

masyan · 2017-03-03T08:20:56Z

+1

felixcheung · 2017-03-05T01:31:04Z

do you know why isn't the test status reflected here?

felixcheung · 2017-03-05T01:32:00Z

I thought you might want to trim the input precode string too, as pointed out #2078 (comment).

since it's a nit, LGTM,
merging if no more comment

felixcheung · 2017-03-05T20:41:22Z

@Leemoonsoo let me know if you want to hold this when we are discussing in #2096.

Leemoonsoo · 2017-03-06T01:29:37Z

@felixcheung I think we can merge this one first. A way we're discuss in #2096 may replace the this PR we don't have the implementation yet.

felixcheung · 2017-03-07T07:16:01Z

merged to master.

felixcheung · 2017-03-07T07:17:26Z

@tinkoff-dwh what's your User ID in Apache JIRA? We could assign this issue to you

tinkoff-dwh · 2017-03-07T07:34:41Z

@felixcheung tinkoff-dwh

### What is this PR for? Adds property "precode". Value of property contains SQL which executes while opening connection. ### What type of PR is it? Improvement ### What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-1988 ### How should this be tested? 1) Set property zeppelin.interpreter.precode =` set search_path='test, public' ` 2) Execute `%jdbc show search_path` ### Questions: * Does the licenses files need update? no * Is there breaking changes for older versions? no * Does this needs documentation? no Author: Tinkoff DWH <tinkoff.dwh@gmail.com> Closes apache#2078 from tinkoff-dwh/ZEPPELIN-1988 and squashes the following commits: cd46cce [Tinkoff DWH] [ZEPPELIN-1988] trim precode 42ffcb7 [Tinkoff DWH] [ZEPPELIN-1988] fix condition 7636b3f [Tinkoff DWH] Merge remote-tracking branch 'origin/master' into ZEPPELIN-1988 66d6ae4 [Tinkoff DWH] [ZEPPELIN-1988] fixes of review items 9d37bc4 [Tinkoff DWH] [ZEPPELIN-1988] fix ba3477a [Tinkoff DWH] [ZEPPELIN-1988] add property "precode" to JDBCInterpreter

[ZEPPELIN-1988] add property "precode" to JDBCInterpreter

ba3477a

felixcheung requested changes Feb 28, 2017

View reviewed changes

DrIgor reviewed Feb 28, 2017

View reviewed changes

[ZEPPELIN-1988] fix

9d37bc4

jongyoul reviewed Feb 28, 2017

View reviewed changes

tinkoff-dwh added 2 commits February 28, 2017 15:42

[ZEPPELIN-1988] fixes of review items

66d6ae4

Merge remote-tracking branch 'origin/master' into ZEPPELIN-1988

7636b3f

felixcheung requested changes Mar 1, 2017

View reviewed changes

[ZEPPELIN-1988] fix condition

42ffcb7

[ZEPPELIN-1988] trim precode

cd46cce

Leemoonsoo mentioned this pull request Mar 5, 2017

[ZEPPELIN-2194] precode for PySparkInterpreter #2096

Closed

asfgit closed this in ebd5e1e Mar 7, 2017

tinkoff-dwh deleted the ZEPPELIN-1988 branch March 7, 2017 07:45

felixcheung mentioned this pull request Mar 12, 2017

[ZEPPELIN-2245] separate precode into JDBCInterpreter #2121

Closed


		public static final String ZEPPELIN_INTERPRETER_HOST = "zeppelin.interpreter.host";

		public static final String ZEPPELIN_PRECODE_PROPERTY_KEY = "zeppelin.interpreter.precode";

Conversation

tinkoff-dwh commented Feb 28, 2017

What is this PR for?

What type of PR is it?

What is the Jira issue?

How should this be tested?

Questions:

Uh oh!

tinkoff-dwh commented Feb 28, 2017

Uh oh!

andrykrp commented Feb 28, 2017

Uh oh!

sotnich commented Feb 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jongyoul Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinkoff-dwh commented Mar 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinkoff-dwh commented Mar 1, 2017

Uh oh!

masyan commented Mar 3, 2017

Uh oh!

felixcheung commented Mar 5, 2017

Uh oh!

felixcheung commented Mar 5, 2017

Uh oh!

felixcheung commented Mar 5, 2017

Uh oh!

Leemoonsoo commented Mar 6, 2017

Uh oh!

felixcheung commented Mar 7, 2017

Uh oh!

felixcheung commented Mar 7, 2017

Uh oh!

tinkoff-dwh commented Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jongyoul Feb 28, 2017 •

edited

Loading

tinkoff-dwh commented Mar 7, 2017 •

edited

Loading