Shell interpreter test and doc #1087

fvaleri · 2016-06-25T00:39:23Z

What is this PR for?

Small refactoring to Shell interpreter.
Add missing unit test and initial doc.

What type of PR is it?

Improvement, Documentation, Refactoring

Todos

- Task

What is the Jira issue?

Open an issue on Jira https://issues.apache.org/jira/browse/ZEPPELIN/
Put link here, and add [ZEPPELIN-Jira number] in PR title, eg. [ZEPPELIN-533]

How should this be tested?

Outline the steps to test the PR here.

Screenshots (if appropriate)

Questions:

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

jongyoul · 2016-06-29T23:50:19Z

Could you please rebase this from current master?

fvaleri · 2016-06-30T09:39:11Z

@jongyoul done and CI is ok.

bzz · 2016-06-30T09:42:48Z

shell/src/test/java/org/apache/zeppelin/shell/ShellInterpreterTest.java

+      result = shell.interpret("ls", context);
+    }
+    // System.out.println(result.message());
+    assertEquals(InterpreterResult.Code.SUCCESS, result.code());


It's better not to leave comments hanging and remove un-used ones.

bzz · 2016-06-30T09:46:22Z

Great clean up and improvement @fvaleri!

~~While you and @jongyoul are both here - how do you think guys, would it make sense to take care of Shell part of the ZEPPELIN-804 as prart of this PR as well?~~

Sorry, I have overlooked ZEPPELIN-923

bzz · 2016-06-30T09:58:40Z

shell/src/main/java/org/apache/zeppelin/shell/ShellInterpreter.java

+        .add(SHELL_COMMAND_TIMEOUT, DEFAULT_COMMAND_TIMEOUT,
+          "Shell command time out in millisecs. Default = " + DEFAULT_COMMAND_TIMEOUT).build()
+    );
+  }


Is there a reason to use static interpreter registration mechanism here again, aftert ZEPPELIN-923 AKA #956 ?

thanks @bzz for pointing out, I will take care of this ASAP

fvaleri · 2016-06-30T10:28:12Z

@bzz @jongyoul Now should be ok.

jongyoul · 2016-06-30T12:04:49Z

The failure is flaky and irrelevant but we need to ensure all other tests passed. Could you please re-trigger CI?

fvaleri · 2016-06-30T12:56:38Z

Another CI job failed even this time on zeppelin-web module.
One is still running but it is very slow (32 min now).

[ERROR] Failed to execute goal com.github.eirslett:frontend-maven-plugin:0.0.25:bower (bower install) on project zeppelin-web: Failed to run task: 'bower --allow-root install' failed. (error code 1) -> [Help 1]

Anyway, this one passed and it's the same code, except for a couple on unused imports that I removed.

jongyoul · 2016-06-30T13:00:54Z

@fvaleri Got it. LGTM.

@felixcheung

### What is this PR for? After #1087 merged, a new docs `shell.md` was added. But in the docs website, still Shell interpreter link points to `pleasecontribute.html`. So I changed this link, applied TOC and added more descriptions. ### What type of PR is it? Documentation ### Todos * [x] - Change `pleasecontribute.html` -> `shell.html` * [x] - Apply TOC(table of contents) * [x] - Add more description to `shell.md` ### Questions: * Does the licenses files need update? no * Is there breaking changes for older versions? no * Does this needs documentation? no Author: AhyoungRyu <[email protected]> Closes #1138 from AhyoungRyu/improve/shell-docs and squashes the following commits: 69d567d [AhyoungRyu] Address @felixcheung feedback fca76a6 [AhyoungRyu] Apply TOC to rest-credential.md c8e988b [AhyoungRyu] Change docs group manual -> interpreter a0bf1d5 [AhyoungRyu] Add shell.html to _navigation.html

@felixcheung

### What is this PR for? After apache#1087 merged, a new docs `shell.md` was added. But in the docs website, still Shell interpreter link points to `pleasecontribute.html`. So I changed this link, applied TOC and added more descriptions. ### What type of PR is it? Documentation ### Todos * [x] - Change `pleasecontribute.html` -> `shell.html` * [x] - Apply TOC(table of contents) * [x] - Add more description to `shell.md` ### Questions: * Does the licenses files need update? no * Is there breaking changes for older versions? no * Does this needs documentation? no Author: AhyoungRyu <[email protected]> Closes apache#1138 from AhyoungRyu/improve/shell-docs and squashes the following commits: 69d567d [AhyoungRyu] Address @felixcheung feedback fca76a6 [AhyoungRyu] Apply TOC to rest-credential.md c8e988b [AhyoungRyu] Change docs group manual -> interpreter a0bf1d5 [AhyoungRyu] Add shell.html to _navigation.html

### What is this PR for? Shell interpreter streaming output had been available by #683, but currently it's broken after #1087 merged. This patch is for putting it back. ### What type of PR is it? Bug Fix ### TODO - [x] Fix test ### What is the Jira issue? [ZEPPELIN-1880](https://issues.apache.org/jira/browse/ZEPPELIN-1880) ### How should this be tested? ``` %sh date && sleep 3 && date ``` the each timestamp must be printed as streaming output ### Screenshots (if appropriate) - before ![shellintpresultbefore](https://cloud.githubusercontent.com/assets/10060731/21585515/60c35a04-d105-11e6-8e68-853ee784e89d.gif) - after ![shellintpresult](https://cloud.githubusercontent.com/assets/10060731/21585516/62142ac8-d105-11e6-8628-1d6eec35daae.gif) ### Questions: * Does the licenses files need update? no * Is there breaking changes for older versions? no * Does this needs documentation? no Author: AhyoungRyu <[email protected]> Closes #1833 from AhyoungRyu/ZEPPELIN-1880 and squashes the following commits: 8fe33c4 [AhyoungRyu] Fix invalid test cases e2fd4bf [AhyoungRyu] Add test for shell inpt timeout property 34d3021 [AhyoungRyu] Fix shell intp streaming output result

### What is this PR for? Shell interpreter streaming output had been available by #683, but currently it's broken after #1087 merged. This patch is for putting it back. ### What type of PR is it? Bug Fix ### TODO - [x] Fix test ### What is the Jira issue? [ZEPPELIN-1880](https://issues.apache.org/jira/browse/ZEPPELIN-1880) ### How should this be tested? ``` %sh date && sleep 3 && date ``` the each timestamp must be printed as streaming output ### Screenshots (if appropriate) - before ![shellintpresultbefore](https://cloud.githubusercontent.com/assets/10060731/21585515/60c35a04-d105-11e6-8e68-853ee784e89d.gif) - after ![shellintpresult](https://cloud.githubusercontent.com/assets/10060731/21585516/62142ac8-d105-11e6-8628-1d6eec35daae.gif) ### Questions: * Does the licenses files need update? no * Is there breaking changes for older versions? no * Does this needs documentation? no Author: AhyoungRyu <[email protected]> Closes #1833 from AhyoungRyu/ZEPPELIN-1880 and squashes the following commits: 8fe33c4 [AhyoungRyu] Fix invalid test cases e2fd4bf [AhyoungRyu] Add test for shell inpt timeout property 34d3021 [AhyoungRyu] Fix shell intp streaming output result (cherry picked from commit 9b4a1bf) Signed-off-by: ahyoungryu <[email protected]>

fvaleri changed the title ~~Shell intepreter test and doc~~ Shell interpreter test and doc Jun 25, 2016

rebase

c069074

bzz reviewed Jun 30, 2016
View reviewed changes

Add new interpreter registration mechanism

7c42733

remove unused imports

03d073f

fvaleri added 3 commits June 30, 2016 14:21

re-trigger CI

9b9487b

re-trigger CI

e4b25ef

re-trigger CI

56bb663

asfgit closed this in e575e81 Jul 1, 2016

AhyoungRyu mentioned this pull request Jul 6, 2016

[DOC][MINOR] Add shell interpreter docs to _navigation.html #1138

Closed

3 tasks

AhyoungRyu mentioned this pull request Jan 2, 2017

[ZEPPELIN-1880] Fix shell interpreter output streaming result #1833

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Shell interpreter test and doc #1087

Shell interpreter test and doc #1087

Uh oh!

fvaleri commented Jun 25, 2016

Uh oh!

jongyoul commented Jun 29, 2016

Uh oh!

fvaleri commented Jun 30, 2016

Uh oh!

bzz Jun 30, 2016

Uh oh!

bzz commented Jun 30, 2016 •

edited

Loading

Uh oh!

bzz Jun 30, 2016 •

edited

Loading

Uh oh!

fvaleri Jun 30, 2016

Uh oh!

fvaleri commented Jun 30, 2016

Uh oh!

jongyoul commented Jun 30, 2016

Uh oh!

fvaleri commented Jun 30, 2016

Uh oh!

jongyoul commented Jun 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Shell interpreter test and doc #1087

Shell interpreter test and doc #1087

Uh oh!

Conversation

fvaleri commented Jun 25, 2016

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

Uh oh!

jongyoul commented Jun 29, 2016

Uh oh!

fvaleri commented Jun 30, 2016

Uh oh!

bzz Jun 30, 2016

Choose a reason for hiding this comment

Uh oh!

bzz commented Jun 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bzz Jun 30, 2016 • 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!

fvaleri Jun 30, 2016

Choose a reason for hiding this comment

Uh oh!

fvaleri commented Jun 30, 2016

Uh oh!

jongyoul commented Jun 30, 2016

Uh oh!

fvaleri commented Jun 30, 2016

Uh oh!

jongyoul commented Jun 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bzz commented Jun 30, 2016 •

edited

Loading

bzz Jun 30, 2016 •

edited

Loading