ZEPPELIN-198 HDFS File Interpreter #752

runyontr · 2016-02-27T02:56:00Z

What is this PR for?

This pull request is a follow of #276 started by @raj-bains. The additional commits address comments from the pull request regarding string creation and error propagation for bad object requests.

What type of PR is it?

Todos

Is there a relevant Jira issue?

ZEPPELIN-198

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

…c ls, cd and pwd functionality against WebHDFS. It addresses ZEPPELIN-198

Merge with master so that the documentation can be checked-in using single commit

…ing creation to StringBuilder.

felixcheung · 2016-02-27T05:49:36Z

file/pom.xml

+  <artifactId>zeppelin-file</artifactId>
+  <packaging>jar</packaging>
+  <version>0.6.0-incubating-SNAPSHOT</version>
+  <name>Zeppelin: File Manager</name>


should probably say HDFS File Interpreter

Currently the only file system implemented in this project is HDFS. The breakout of having a generic FileInterpreter that that other file systems could easily extend would enable faster turn around on future file system interpreters such as S3, Swift, or Ceph. (the two funcions required are listAll and isDirectory)

I see two paths:

Change the current name to reflect its current use, and have the next implementation that uses this class rename later to be more generic, in which case I agree HDFS File Interpreter should be the name here.

Rename to something that would suggest to new developers that this is where "File System Interpreters" should be created. I propose Zeppelin File System Interpreters

Zeppelin File System Interpreters sounds ok to me.

Documentation uses HDFS File Interpreter while package name does not.
@runyontr could you update documentation?

You'll also need to add an link https://github.com/apache/incubator-zeppelin/blob/master/docs/_includes/themes/zeppelin/_navigation.html#L41

… match functionality

felixcheung · 2016-03-02T02:05:19Z

zeppelin-server/derby.log

@@ -0,0 +1,13 @@
+----------------------------------------------------------------


hmm .. is this added by accident or missing .gitignore?

It looks like both were an issue. I accidentally added it, and it wasn't in the gitignore. When I deleted the file and removed it from git, after re-running mvn clean package, git wanted to add the file again. I've added a line to the .gitingore file to prevent this from happening again.

Leemoonsoo · 2016-03-05T15:22:49Z

docs/interpreter/hdfs.md

+---
+{% include JB/setup %}
+.add(HDFS_USER, "hdfs", "The WebHDFS user")
+            .add(HDFS_MAXLENGTH, "1000", "Maximum number of lines of results fetched").build());


Is it intended to be here?

No, its absolutely not supposed to be there.

…en pom file and interpreter documentation.

AhyoungRyu · 2016-03-07T06:34:05Z

docs/interpreter/hdfs.md

+<br/>
+This interpreter connects to HDFS using the HTTP WebHDFS interface.
+It supports the basic shell file commands applied to HDFS, it currently only supports browsing
+* You can use <i>ls [PATH]</i> and <i>ls -l [PATH]</i> to list a directory. If the path is missing, then the current directory is listed.  <i>ls </i> supports a <i>-h</h> flag for human readable file sizes.


I think <i>-h</h> should be changed to <i>-h</i> since all sentences after this are shown italic font.

…atting issues for hdfs.md

runyontr · 2016-03-08T01:38:30Z

Thank you all for the comments. I've noticed that now there is a note for this pull request that this branch has conflicts that must be resolved. Would you like me to re-merge in the current master branch from apache/incubator-zeppelin, or will the approver of the pull request deal with the merge conflicts that have come up from pulling new branches into master?

Leemoonsoo · 2016-03-08T02:02:28Z

It's more recommended to merge current master to solve conflict. That'll run additional CI test before merge it into master.

# Conflicts: # conf/zeppelin-site.xml.template

Leemoonsoo · 2016-03-10T03:22:19Z

file/src/main/java/org/apache/zeppelin/file/HDFSFileInterpreter.java

+  static {
+    Interpreter.register(
+        "hdfs",
+        "hdfs",


The second "hdfs" is interpreter group name.
If 'file' interpreter submodule planning to have more implementation other than HDFSFileInterpreter, i suggest change group name as "file".

Leemoonsoo · 2016-03-10T03:30:53Z

I have tested and working well. Thanks for the contribution!
I left few more comments. And you also probably want to add org.apache.zeppelin.file.HDFSFileInterpreter to ZeppelinConfiguration.java and conf/zeppelin-site.xml.template.

Leemoonsoo · 2016-03-11T17:53:36Z

zeppelin-distribution/src/bin_license/LICENSE


    (CDDL 1.0) javax.activation (javax.activation:activation:jar:1.1.1 - http://java.sun.com/javase/technologies/desktop/javabeans/jaf/index.jsp)
    (CDDL 1.1) Jersey (com.sun.jersey:jersey:jar:1.9 - https://jersey.java.net/)
+    (CDDL 1.1) (GPL2) jersey-core (org.glassfish.jersey.core:jersey-core:2.22.2 - https://jersey.java.net/)


I think we can remove (GPL2) here, if we're providing jersey-core under CDDL 1.1 license.
Also all transitive dependency (e.g. org.glassfish.hk2:hk2-api, etc) need to be addressed, if license of jersey-core doesn't cover them.

I'll make that change later and push the amended version back up.

WRT the transitive dependencies: I'm not very familiar with licensing and had a question: If there's a main package reference (e.g. org.glassfish.jersey in our case) that is provided under a particular license (e.g. CDDL 1.1) it's not true that the particular license covers all the dependencies of that package?

I'll run through the list individually and add any items that are needed.

Addressing all the transitive dependency is hard work. but, transitive dependencies are no different from first order dependencies. Please check here.

Leemoonsoo · 2016-03-15T18:48:36Z

Looks good to me.

Leemoonsoo · 2016-03-16T18:18:48Z

Merge if there're no more discussions

### What is this PR for? This pull request is a follow of apache#276 started by raj-bains. The additional commits address comments from the pull request regarding string creation and error propagation for bad object requests. ### What type of PR is it? [Bug Fix | Improvement | Feature | Documentation | Hot Fix | Refactoring] Feature/Subtask ### Todos ### Is there a relevant Jira issue? [ZEPPELIN-198](https://issues.apache.org/jira/browse/ZEPPELIN-198) ### How should this be tested? Outline the steps to test the PR here. ### Screenshots (if appropriate) ![selection_001](https://cloud.githubusercontent.com/assets/9200575/13370345/87a922ac-dcd2-11e5-9217-66ec21c016de.png) ![selection_002](https://cloud.githubusercontent.com/assets/9200575/13370347/89a5603e-dcd2-11e5-9555-9c38167e8667.png) ![selection_003](https://cloud.githubusercontent.com/assets/9200575/13370359/b9bbadc8-dcd2-11e5-97bd-47d216f0c7da.png) ![selection_004](https://cloud.githubusercontent.com/assets/9200575/13370364/16ff9274-dcd3-11e5-9b4c-f324e7035c20.png) ### Questions: * Does the licenses files need update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Author: Tom Runyon <runyontr@gmail.com> Author: Raj Bains <rajbains@Rajs-MacBook-Pro.local> Closes apache#752 from runyontr/master and squashes the following commits: f7bfef8 [Tom Runyon] ZEPPELIN-198 added transitive dependency lincense information af16ce0 [Tom Runyon] ZEPPELIN-198 removed GPL2 reference from license file 5e9e131 [Tom Runyon] ZEPPELIN-198 Updated ZeppelinConfiguration to hold HDFS interpreter 9832622 [Tom Runyon] ZEPPELIN-198 Changed group for hdfs interpreter c34a913 [Tom Runyon] ZEPPELIN-198 Updated licenses file to include org.glassfish.jersey.core 8d0ee3d [Tom Runyon] Merge https://github.com/apache/incubator-zeppelin 9f66514 [Tom Runyon] ZEPPELIN-198 Removed extra copy of configuration table and fixed formatting issues for hdfs.md 5938b0e [Tom Runyon] ZEPPELIN-198 Updated documentation to be consistent with naming between pom file and interpreter documentation. 67bbc5b [Tom Runyon] ZEPPELIN-198 Added navigation to hdfs interpreter 1c7a5c2 [Tom Runyon] ZEPPELIN-198 removed errant text in documentation. 56a5174 [Tom Runyon] ZEPPELIN-198 fixed logging to match standards 933c890 [Tom Runyon] MAINT Updated .gitignore file to remove zeppelin-server/derby.log 29540df [Tom Runyon] ZEPPELIN-198 removed zeppelin-server/derby.log 71d53d3 [Tom Runyon] ZEPPELIN-198 Changed pom name to Zeppelin File System Interpreters to match functionality aec0512 [Tom Runyon] ZEPPELIN-198 Fixed compile error for error logging. d24f4c0 [Tom Runyon] ZEPPELIN-198 Added error logging when returning error in interpet 227b815 [Tom Runyon] ZEPPELIN-198 Updated interpreter documentation. b505391 [Tom Runyon] ZEPPELIN-198 Added completion functionality to HDFSInterpreter 32ed7cb [Tom Runyon] ZEPPELIN-198 Added -h flag for human readable byte sizes. Updated string creation to StringBuilder. 797fd29 [Tom Runyon] Added org.glassfish.jersey.core to pom.xml file for hdfs intepretor 27e0438 [Tom Runyon] Modified string creation to use StringBuilder 79f0d90 [Tom Runyon] Merge branch 'master' of https://github.com/raj-bains/incubator-zeppelin 70507a8 [Raj Bains] Add Documentation and a missing dependency for HDFS File Browser 1239fe6 [Raj Bains] Merge remote-tracking branch 'upstream/master' 7d61e5f [Raj Bains] This is the first reviewed version of File Interpreter that adds basic ls, cd and pwd functionality against WebHDFS. It addresses ZEPPELIN-198 865e6ab [Raj Bains] Add File Interpreter, HDFS Interpreter and Tests

Closes #276 (fixed by #752) Closes #381 (fixed by #378) Closes #933 (fixed by #1043)

Raj Bains and others added 8 commits September 2, 2015 23:50

Add File Interpreter, HDFS Interpreter and Tests

865e6ab

This is the first reviewed version of File Interpreter that adds basi…

7d61e5f

…c ls, cd and pwd functionality against WebHDFS. It addresses ZEPPELIN-198

Merge remote-tracking branch 'upstream/master'

1239fe6

Merge with master so that the documentation can be checked-in using single commit

Add Documentation and a missing dependency for HDFS File Browser

70507a8

Merge branch 'master' of https://github.com/raj-bains/incubator-zeppelin

79f0d90

Modified string creation to use StringBuilder

27e0438

Added org.glassfish.jersey.core to pom.xml file for hdfs intepretor

797fd29

ZEPPELIN-198 Added -h flag for human readable byte sizes. Updated str…

32ed7cb

…ing creation to StringBuilder.

felixcheung reviewed Feb 27, 2016
View reviewed changes

runyontr added 5 commits February 27, 2016 11:03

ZEPPELIN-198 Added completion functionality to HDFSInterpreter

b505391

ZEPPELIN-198 Updated interpreter documentation.

227b815

ZEPPELIN-198 Added error logging when returning error in interpet

d24f4c0

ZEPPELIN-198 Fixed compile error for error logging.

aec0512

ZEPPELIN-198 Changed pom name to Zeppelin File System Interpreters to…

71d53d3

… match functionality

felixcheung reviewed Mar 2, 2016
View reviewed changes

runyontr added 3 commits March 2, 2016 06:08

ZEPPELIN-198 removed zeppelin-server/derby.log

29540df

MAINT Updated .gitignore file to remove zeppelin-server/derby.log

933c890

ZEPPELIN-198 fixed logging to match standards

56a5174

Leemoonsoo reviewed Mar 5, 2016
View reviewed changes

runyontr added 3 commits March 5, 2016 18:13

ZEPPELIN-198 removed errant text in documentation.

1c7a5c2

ZEPPELIN-198 Added navigation to hdfs interpreter

67bbc5b

ZEPPELIN-198 Updated documentation to be consistent with naming betwe…

5938b0e

…en pom file and interpreter documentation.

AhyoungRyu reviewed Mar 7, 2016
View reviewed changes

ZEPPELIN-198 Removed extra copy of configuration table and fixed form…

9f66514

…atting issues for hdfs.md

Merge https://github.com/apache/incubator-zeppelin

8d0ee3d

# Conflicts: # conf/zeppelin-site.xml.template

Leemoonsoo reviewed Mar 10, 2016
View reviewed changes

runyontr added 3 commits March 10, 2016 21:06

ZEPPELIN-198 Updated licenses file to include org.glassfish.jersey.core

c34a913

ZEPPELIN-198 Changed group for hdfs interpreter

9832622

ZEPPELIN-198 Updated ZeppelinConfiguration to hold HDFS interpreter

5e9e131

Leemoonsoo reviewed Mar 11, 2016
View reviewed changes

runyontr added 2 commits March 13, 2016 21:59

ZEPPELIN-198 removed GPL2 reference from license file

af16ce0

ZEPPELIN-198 added transitive dependency lincense information

f7bfef8

asfgit closed this in b45663d Mar 17, 2016

asfgit pushed a commit that referenced this pull request Jun 26, 2016

Close PRs on Github without commiting

0209bc3

Closes #276 (fixed by #752) Closes #381 (fixed by #378) Closes #933 (fixed by #1043)

		@@ -0,0 +1,13 @@
		----------------------------------------------------------------

ZEPPELIN-198 HDFS File Interpreter #752

ZEPPELIN-198 HDFS File Interpreter #752

Uh oh!

Conversation

runyontr commented Feb 27, 2016

What is this PR for?

What type of PR is it?

Todos

Is there a relevant Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

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!

runyontr commented Mar 8, 2016

Uh oh!

Leemoonsoo commented Mar 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Leemoonsoo commented Mar 10, 2016

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!

Leemoonsoo commented Mar 15, 2016

Uh oh!

Leemoonsoo commented Mar 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants