[ZEPPELIN-1999] get interpreter property with replaced context parameters by tinkoff-dwh · Pull Request #2085 · apache/zeppelin

tinkoff-dwh · 2017-03-01T08:21:45Z

What is this PR for?

Adds posibility to use context parameters (types: String.class, Double.class, Float.class, Short.class,
Byte.class, Character.class, Boolean.class, Integer.class, Long.class, ) into property value of interpreter.

What type of PR is it?

Feature

What is the Jira issue?

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

How should this be tested?

Add text with markers #{contextFieldNAme} (ex. #{noteId} or #{replName}) to interpreter property value (or add new property of interpreter).
Get this property (getProperty(key)), markers should be replaced by context values

Questions:

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

…ters

# Conflicts: # zeppelin-interpreter/src/test/java/org/apache/zeppelin/interpreter/InterpreterTest.java

felixcheung

is there a reason you choose the #{contextParameterName} format?

tinkoff-dwh · 2017-03-01T21:08:19Z

uniqueness of the marker to replace
similar format of the parameters in the paragraph https://github.com/apache/zeppelin/blob/master/docs/manual/dynamicform.md (${}, not the same, not to be confused)

andrykrp · 2017-03-02T10:08:58Z

Looks good

mgarmash · 2017-03-06T07:28:17Z

Nice feature! +1

tinkoff-dwh · 2017-03-09T12:10:20Z

@felixcheung
Are there problems for merge?

felixcheung · 2017-03-12T18:09:26Z

 .idea/
 *.iml
+*.iws
+*.ipr


what are these for?

it is excludes extensions of project (jetbrains IDEA)

If it's not related to this PR, can you please create new minor PR for handling it?

@jongyoul
done

felixcheung · 2017-03-12T18:10:18Z


-Properties are exported as environment variable when property name is consisted of upper characters, numbers and underscore ([A-Z_0-9]). Otherwise set properties as JVM property.
+Properties are exported as environment variable when property name is consisted of upper characters, numbers and underscore ([A-Z_0-9]). Otherwise set properties as JVM property. 
+You may use parameters from the context of interpreter by add #{contextParameterName} (except fields of paragraph) in value, parameter can be of the following types: string, number, boolean. The list of available context parameters can be viewed in the class [InterpreterContext.java](https://github.com/apache/zeppelin/blob/master/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/InterpreterContext.java) + parameter #{user}.


since this is talking about a new concept, let's put this into a new paragraph (ie. double new line in markdown,before this line)

i think it'd be better to get casual users to look at Java code - could you add a table here of the types/context parameters it could support?

.. and with some code/paragraph example in text that the user can run and test with?

felixcheung · 2017-03-12T18:13:07Z

I think this is a good feature to have. Let's hold on to get a bit more feedback on this - @Leemoonsoo what do you think about something like this?

dwhsys · 2017-03-15T09:04:35Z

I think that this PR is a great step forward to enterprise version of Zeppelin. Using this feature there will be an ability to implement native integration between different interpreters based on single schema per user / note in core analytical DBMS. Another feature is the possibility of data access control without using credentials.

@felixcheung, @Leemoonsoo, what do you think?

Leemoonsoo · 2017-03-18T15:34:53Z

Thanks @tinkoff-dwh for useful new feature.
LGTM!

AhyoungRyu · 2017-03-19T12:39:58Z


-Properties are exported as environment variable when property name is consisted of upper characters, numbers and underscore ([A-Z_0-9]). Otherwise set properties as JVM property.
-
+Properties are exported as environment variable when property name is consisted of upper characters, numbers and underscore ([A-Z_0-9]). Otherwise set properties as JVM property. 


One minor suggestion: )
as environment variable -> as environment variables

AhyoungRyu · 2017-03-19T12:46:54Z

-
+Properties are exported as environment variable when property name is consisted of upper characters, numbers and underscore ([A-Z_0-9]). Otherwise set properties as JVM property. 
+
+You may use parameters from the context of interpreter by add #{contextParameterName} (except fields of paragraph) in value, parameter can be of the following types: string, number, boolean.


How about
"You might want to use parameters from the context of interpreter by adding #{contextParameterName} (except fields of paragraph) in the value. In this case, the parameter can be one of the following types: string, number or boolean."

Probably it's only for me, but in my case, hard to understand what "except fields of paragraph" means. It'll be better we have more information what except fields is. What do you think?

list of available parameters described below

I assumed that the other readers may same with me. Couldn't get "except fields of paragraph" has same meaning with "list of available parameters described below".

AhyoungRyu · 2017-03-19T12:50:50Z

+
+If context parameter is null then replaced by empty string.
+
+<img src="../assets/themes/zeppelin/img/screenshots/interpreter_setting_with_context_parameters.png" width="500px">


It seems the image size is too small so hard to see now. Needs to be "800px" as this image does.

no, image width more than 500px

This is what I can see for this image. The font in this image is even smaller than the other text around it.

The font on the following image smaller too

I agree, width should increment to 800

AhyoungRyu · 2017-03-19T12:52:42Z

+
+###### Usage
+For example in database exist user `user1` with some password and exists user with same name into Zeppelin. Configure the jdbc interpreter (postgres), set `default.user = #{user}`.
+1. Sign in (as `user1`)


Can you add one new line before this line? Currently it's rendered like below.

AhyoungRyu · 2017-03-19T12:53:21Z

+###### Usage
+For example in database exist user `user1` with some password and exists user with same name into Zeppelin. Configure the jdbc interpreter (postgres), set `default.user = #{user}`.
+1. Sign in (as `user1`)
+2. Open interpreter


What do you mean exactly "Open interpreter" ? Can we use more elaborative word in here?

AhyoungRyu · 2017-03-19T13:19:06Z

+<img src="../assets/themes/zeppelin/img/screenshots/interpreter_setting_with_context_parameters.png" width="500px">
+
+###### Usage
+For example in database exist user `user1` with some password and exists user with same name into Zeppelin. Configure the jdbc interpreter (postgres), set `default.user = #{user}`.


For example in database exist user user1 with some password and exists user with same name into Zeppelin.

As the above sentence, we assume that the readers already know about how they configure user setting and enable Shiro. But as you know, this manual/interpreters.md mainly focuses on Zeppelin beginners(e.g. don't know about what interpreter is in Zeppelin/ how they can bind interpreter to the note ..). Not sure what can be the better description for this, i just thought this sentence need more information. What do you think? :)

maybe, but it is common doc to all interpreters and the best place which I found. might be better to remove this pragraph.

AhyoungRyu · 2017-03-20T02:25:13Z

@tinkoff-dwh Saw your latest update. As you did,

###### Usage		
 For example in database exist user `user1` with some password and exists user with same name into Zeppelin. Configure the jdbc interpreter (postgres), set `default.user = #{user}`.
		
   1. Sign in (as `user1`)		
   2. Open interpreter		
   3. Execute `select current_user`

The above usage example can be added JDBC interpreter docs. But it's okay we can update it in later time not in this PR. LGTM

felixcheung

any other comment?

felixcheung · 2017-03-22T08:25:09Z

+                p = p.replaceAll(String.format(markerTemplate, field.getName()),
+                    value != null ? value.toString() : StringUtils.EMPTY);
+              } catch (Exception e) {
+                logger.error("Cannot replace context parameter", e);


should it stop or continue with #{name} in it if it couldn't find a match?

if field with name name exists in context then replaced in any case

could you add a test for that new case then?

test testPropertyWithReplacedContextFields covers this case, replName in context is NULL and replaced with an empty string

felixcheung · 2017-03-23T17:08:55Z

Ok. Any other comment? Merging if no more discussion.

…ters ### What is this PR for? Adds posibility to use context parameters (types: String.class, Double.class, Float.class, Short.class, Byte.class, Character.class, Boolean.class, Integer.class, Long.class, ) into property value of interpreter. ### What type of PR is it? Feature ### What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-1999 ### How should this be tested? 1. Add text with markers #{contextFieldNAme} (ex. #{noteId} or #{replName}) to interpreter property value (or add new property of interpreter). 2. Get this property (getProperty(key)), markers should be replaced by context values ### Questions: * Does the licenses files need update? no * Is there breaking changes for older versions? no * Does this needs documentation? yes Author: Tinkoff DWH <tinkoff.dwh@gmail.com> Closes apache#2085 from tinkoff-dwh/ZEPPELIN-1999 and squashes the following commits: fa1500a [Tinkoff DWH] [ZEPPELIN-1999] fix logic of replace 93c759d [Tinkoff DWH] Merge remote-tracking branch 'origin/master' into ZEPPELIN-1999 be4fada [Tinkoff DWH] [ZEPPELIN-1999] revert gitignore c0110e9 [Tinkoff DWH] [ZEPPELIN-1999] documentation 61ac564 [Tinkoff DWH] [ZEPPELIN-1999] docs a10dc0e [Tinkoff DWH] [ZEPPELIN-1999] skip fields of paragraph ea9c6a3 [Tinkoff DWH] [ZEPPELIN-1999] docs 7c4489c [Tinkoff DWH] Merge remote-tracking branch 'origin/master' into ZEPPELIN-1999 527419a [Tinkoff DWH] Merge remote-tracking branch 'origin/master' into ZEPPELIN-1999 b5424b9 [Tinkoff DWH] [ZEPPELIN-1999] get interpreter property with replaced context parameters

tinkoff-dwh added 4 commits February 22, 2017 14:53

[ZEPPELIN-1999] get interpreter property with replaced context parame…

b5424b9

…ters

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

527419a

# Conflicts: # zeppelin-interpreter/src/test/java/org/apache/zeppelin/interpreter/InterpreterTest.java

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

7c4489c

[ZEPPELIN-1999] docs

ea9c6a3

felixcheung reviewed Mar 1, 2017

View reviewed changes

[ZEPPELIN-1999] skip fields of paragraph

a10dc0e

masyan mentioned this pull request Mar 5, 2017

[ZEPPELIN-2194] precode for PySparkInterpreter #2096

Closed

felixcheung requested changes Mar 12, 2017

View reviewed changes

[ZEPPELIN-1999] docs

61ac564

tinkoff-dwh mentioned this pull request Mar 17, 2017

[ZEPPELIN-2257] notification about incompleteness of data #2134

Closed

AhyoungRyu reviewed Mar 19, 2017

View reviewed changes

[ZEPPELIN-1999] documentation

c0110e9

tinkoff-dwh added 2 commits March 20, 2017 09:28

[ZEPPELIN-1999] revert gitignore

be4fada

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

93c759d

tinkoff-dwh closed this Mar 20, 2017

tinkoff-dwh reopened this Mar 20, 2017

tinkoff-dwh closed this Mar 20, 2017

tinkoff-dwh reopened this Mar 20, 2017

tinkoff-dwh closed this Mar 21, 2017

tinkoff-dwh reopened this Mar 21, 2017

felixcheung reviewed Mar 22, 2017

View reviewed changes

[ZEPPELIN-1999] fix logic of replace

fa1500a

asfgit closed this in 998c8f3 Mar 25, 2017

tinkoff-dwh deleted the ZEPPELIN-1999 branch March 27, 2017 05:49


		Properties are exported as environment variable when property name is consisted of upper characters, numbers and underscore ([A-Z_0-9]). Otherwise set properties as JVM property.

		Properties are exported as environment variable when property name is consisted of upper characters, numbers and underscore ([A-Z_0-9]). Otherwise set properties as JVM property.


		Properties are exported as environment variable when property name is consisted of upper characters, numbers and underscore ([A-Z_0-9]). Otherwise set properties as JVM property.

		You may use parameters from the context of interpreter by add #{contextParameterName} (except fields of paragraph) in value, parameter can be of the following types: string, number, boolean.


		If context parameter is null then replaced by empty string.

		<img src="../assets/themes/zeppelin/img/screenshots/interpreter_setting_with_context_parameters.png" width="500px">

Conversation

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

Uh oh!

What is this PR for?

What type of PR is it?

What is the Jira issue?

How should this be tested?

Questions:

Uh oh!

felixcheung left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

andrykrp commented Mar 2, 2017

Uh oh!

mgarmash commented Mar 6, 2017

Uh oh!

tinkoff-dwh commented Mar 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinkoff-dwh Mar 12, 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!

felixcheung commented Mar 12, 2017

Uh oh!

dwhsys commented Mar 15, 2017

Uh oh!

Leemoonsoo commented Mar 18, 2017

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 Mar 19, 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!

masyan Mar 19, 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!

AhyoungRyu commented Mar 20, 2017

Uh oh!

felixcheung left a comment

Choose a reason for hiding this comment

Uh oh!

tinkoff-dwh commented Mar 1, 2017 •

edited

Loading

tinkoff-dwh commented Mar 1, 2017 •

edited

Loading

tinkoff-dwh Mar 12, 2017 •

edited

Loading

tinkoff-dwh Mar 19, 2017 •

edited

Loading

masyan Mar 19, 2017 •

edited

Loading