Skip to content
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

Groovy Interpreter for Apazhe Zeppelin [ZEPPELIN-2176] #2135

Closed
wants to merge 18 commits into from

Conversation

dlukyanov
Copy link
Contributor

What is this PR for?

Groovy Interpreter

What type of PR is it?

Feature

Todos

  • [ Tests ] - Task
  • [ Documentation ] - Task

What is the Jira issue?

[ZEPPELIN-2176]

How should this be tested?

Follow the groovy interpreter documentation samples

Questions:

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

@dlukyanov
Copy link
Contributor Author

@Leemoonsoo, I moved the repo under the user, used the latest master, now i have another error:

++ grep -e '"ref":' -e '"sha":'
++ tr '\n' ' '
+ COMMIT=
+ sleep 30
+ python ./travis_check.py dlukyanov
Traceback (most recent call last):
  File "./travis_check.py", line 36, in <module>
    commit = sys.argv[2]
IndexError: list index out of range
Build step 'Execute shell' marked build as failure
Putting comment on the pull request
Finished: FAILURE

@Leemoonsoo
Copy link
Member

Jenkins build script didn't handle well when source branch name of pullrequest is master.
I just updated the jenkins build script to handle the name master.
@dlukyanov Could you trigger jenkins again (close/open pr) and see if it checks correctly?

@dlukyanov
Copy link
Contributor Author

@Leemoonsoo, now I see status from travis )
And going to solve strange build errors...
Thanks!

@Leemoonsoo
Copy link
Member

The error

[ERROR] error An unexpected error occurred: "https://registry.yarnpkg.com/binary-extensions/-/binary-extensions-1.8.0.tgz: Request failed \"502 Bad Gateway\"".
[ERROR] Failed to execute goal com.github.eirslett:frontend-maven-plugin:1.3:yarn (yarn install) on project zeppelin-web: Failed to run task: 'yarn install --no-lockfile' failed. (error code 1) -> [Help 1]
[ERROR] 

looks like temporary network problem. You can always restart the failed Job and see if it passing.

… HTTP.groovy to resources to simplify build, add default z-properties
… HTTP.groovy to resources to simplify build, add default z-properties
@dlukyanov
Copy link
Contributor Author

@Leemoonsoo , is it possible to restart jenkins checks after travis succeed?
I've never got travis success without restarting travis jobs manually.
https://travis-ci.org/dlukyanov/zeppelin/builds

@dlukyanov dlukyanov changed the title [WIP] Groovy Interpreter for Apazhe Zeppelin [ZEPPELIN-2176] Groovy Interpreter for Apazhe Zeppelin [ZEPPELIN-2176] Mar 21, 2017
@dlukyanov
Copy link
Contributor Author

Hello All, Asking for help with Jenkins:
Travis succeed: https://travis-ci.org/dlukyanov/zeppelin
But jenkins failed: https://builds.apache.org/job/zeppelin-pull-request/604/console

Build https://travis-ci.org/dlukyanov/zeppelin/builds/213281815
[1] OK              https://travis-ci.org/dlukyanov/zeppelin/jobs/213281816
[2] Not completed   https://travis-ci.org/dlukyanov/zeppelin/jobs/213281817
[3] OK              https://travis-ci.org/dlukyanov/zeppelin/jobs/213281818
[4] OK              https://travis-ci.org/dlukyanov/zeppelin/jobs/213281819
[5] OK              https://travis-ci.org/dlukyanov/zeppelin/jobs/213281820
[6] OK              https://travis-ci.org/dlukyanov/zeppelin/jobs/213281821
[7] OK              https://travis-ci.org/dlukyanov/zeppelin/jobs/213281822
[8] OK              https://travis-ci.org/dlukyanov/zeppelin/jobs/213281823
1 job(s) failed, 0 job(s) running/pending
+ RET_CODE=1
+ '[' 1 -eq 2 ']'
+ '[' 1 -eq 2 ']'
+ exit 1

@Leemoonsoo
Copy link
Member

Looks like second test matrix succeeded after the manual restart.
In this case, you also need to trigger Jenkins again to check latest build status.
Simply close and reopen this PR will do that.

@Leemoonsoo
Copy link
Member

I tested last commit and it works well.

However the variable can't be read from the other paragraph.
image

This behavior is different from other interpreters, like spark (scala), python, r.
Is it intended?

Also, can we place document under /docs/interpreter/ instead of /groovy/, so website can show groovy documentation?

@dlukyanov
Copy link
Contributor Author

@Leemoonsoo Interesting) I did not knew that... i'll take a look how it's implemented...
With doc - i'll move it

- implement shared script variables
- move docs
- implement run methods
@dlukyanov
Copy link
Contributor Author

dlukyanov commented Mar 25, 2017

@Leemoonsoo, Just committed changes.
Doc moved.
About shared vars - it will be a little different from scala/spark.
If you use groovy script variable (undeclared or annotated) it will behave like you mentioned

a="Hello world"
//or
@groovy.transform.Field String aa = "Hello world"
//or
g.put('aaa',"Hello world")

The usual declared variables - has local visibility.

groovyscriptvars

@Leemoonsoo
Copy link
Member

@dlukyanov Thanks for the update and explanation on behavior.

Inside of docs directory, groovy.md will need some header to be compiled with Jekyll. For example, spark.md

---
layout: page
title: "Apache Spark Interpreter for Apache Zeppelin"
description: "Apache Spark is a fast and general-purpose cluster computing system. It provides high-level APIs in Java, Scala, Python and R, and an optimized engine that supports general execution engine."
group: interpreter
---
<!--
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->
{% include JB/setup %}

Menu in docs also need link to groovy. https://github.com/apache/zeppelin/blob/master/docs/_includes/themes/zeppelin/_navigation.html#L56

.travis.yml is updated since this branch is created. And we need add !groovy in this line. Could you handle it?

dm and others added 3 commits March 27, 2017 22:08
- Inside of docs directory, groovy.md will need some header to be compiled with Jekyll
- Menu in docs also need link to groovy
- .travis.yml we need add !groovy
@dlukyanov
Copy link
Contributor Author

@Leemoonsoo , done. waiting for travis.

@Leemoonsoo
Copy link
Member

Thanks @dlukyanov for the contribution.

LGTM!

@Leemoonsoo
Copy link
Member

Merge to master if no further comments.

Copy link
Contributor

@AhyoungRyu AhyoungRyu left a comment

Choose a reason for hiding this comment

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

@dlukyanov Thanks for great contribution and sorry for my late blocking. I left some minor comment about docs. BTW why did you added README.txt & README.md both?
And generally, Zeppelin follows Google Java code style#2-block-indentation. Can this be addressed as well before merge? Probably this looks not important but other ppl who want to contribute to Zeppelin can refer your PR(e.g. code style/ license handling/ docs etc etc..) in the future :)

@@ -78,6 +78,7 @@
<li><a href="{{BASE_PATH}}/interpreter/scio.html">Scio</a></li>
<li><a href="{{BASE_PATH}}/interpreter/shell.html">Shell</a></li>
<li><a href="{{BASE_PATH}}/interpreter/spark.html">Spark</a></li>
<li><a href="{{BASE_PATH}}/interpreter/groovy.html">Groovy</a></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put Groovy below Geode? It should be in alphabetical order.


* `groovy.xml.MarkupBuilder g.html()`

Starts or continues rendering of `%angular` to output and returns [groovy.xml.MarkupBuilder](https://www.google.com/search?q=groovy.xml.MarkupBuilder)
Copy link
Contributor

Choose a reason for hiding this comment

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

How about this link http://groovy-lang.org/processing-xml.html#_markupbuilder instead of google search result ?

- Zeppelin follows Google Java code
- interpreter alphabetical order in _navigation.html
- direct link to MarkupBuilder in groovy help
@dlukyanov
Copy link
Contributor Author

dlukyanov commented Mar 29, 2017

@AhyoungRyu, changes committed,waiting for travis...

From travis-ci.org:
We stopped accepting new builds because of an issue with our logs DB.
Sorry for the inconvenience. You can follow the updates here. Thank you for your understanding.

@@ -55,159 +55,172 @@

/**
* Groovy interpreter for Zeppelin.
* @author dlukyanov@ukr.net / dmitry lukyanov
Copy link
Contributor

Choose a reason for hiding this comment

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

Here also

Copy link
Contributor Author

Choose a reason for hiding this comment

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

code autoformat.. but it's not allowed to keep the created by ? any footprint? )))

Copy link
Contributor

Choose a reason for hiding this comment

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

@dlukyanov I see. Yeah so sadly Zeppelin doesn't encourage the footprint 😭 (see here: d64920d)

@@ -41,308 +43,328 @@

/**
* Groovy interpreter for Zeppelin.
* @author dlukyanov@ukr.net / dmitry lukyanov
Copy link
Contributor

Choose a reason for hiding this comment

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

@dlukyanov Zeppelin generally don't write author name in the source code. Even if it does, all ppl still can track and see what you contribute to Zeppelin :)


bindings.clear();
InterpreterResult result = new InterpreterResult(Code.SUCCESS, out.toString());
//log.info("RESULT: "+result);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove this commented log.info if it's not necessary?

@AhyoungRyu
Copy link
Contributor

Thanks for addressing my comments! I left some minor comments again about "author name" in the source code and removing unnecessary log.info. Except these two things, LGTM 👍

- remove @author
- remove commented code
@dlukyanov
Copy link
Contributor Author

@Leemoonsoo , @AhyoungRyu , any other comments?

@AhyoungRyu
Copy link
Contributor

Looks good to me :)

@AhyoungRyu
Copy link
Contributor

Merge to master if no further comments.

@asfgit asfgit closed this in 53a28a3 Apr 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants