ZEPPELIN-1515. Notebook: HDFS as a backend storage (Use hadoop client jar) by zjffdu · Pull Request #2455 · apache/zeppelin

zjffdu · 2017-06-30T08:00:09Z

What is this PR for?

This PR is trying to add hdfs as another implementation for NotebookRepo. There's another PR about using webhdfs to implement that. Actually hdfs client library is compatibility cross major versions. See http://hadoop.apache.org/docs/stable/hadoop-project-dist/hadoop-common/Compatibility.html#Wire_compatibility, if using webhdfs, the code become more complicated and may lose some features of hdfs.

This PR is also required for HA of zeppelin, so that multiple zeppelin instances can share notes via hdfs. I add hadoop-client in pom file. So zeppelin will package hadoop client jar into its binary distribution. This is because zeppelin may be installed in a gateway machine where no hadoop is installed (only hadoop configuration file is existed in this machine) And since the hadoop client will work with multiple versions of hadoop, so it is fine to package into binary distribution. Spark also package hadoop client jar in its binary distribution.

What type of PR is it?

[Feature]

Todos

- Task

What is the Jira issue?

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

How should this be tested?

Unit test is added. Also manually verify it in a single node cluster.

Screenshots (if appropriate)

Questions:

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

zjffdu · 2017-06-30T08:20:20Z

@Leemoonsoo @felixcheung @khalidhuseynov Could you help review it ? Thanks

jongyoul · 2017-06-30T08:51:56Z

Hi, Recently, I've found some version of hadoop related library make a crash of jetty which is used in Zeppelin Server. I've also changed yarn cluster manager to use another classloader. How do you think you can use another classloader while running hdfs notebook storage? It would make more changes in Zeppelin server side but it will be better for the future.

zjffdu · 2017-06-30T09:30:51Z

@jongyoul What kind of issue do you see in cluster manager PR ? Usually this is due to jar version conflict. You can exclude this transitive jar of hadoop client library.

felixcheung · 2017-07-01T04:31:51Z

+<property>
+ <name>zeppelin.notebook.storage</name>
+ <value>org.apache.zeppelin.notebook.repo.HdfsNotebookRepo</value>
+ <description>local notebook persistence layer implementation</description>


local notebook persistence layer -> hdfs notebook persistence layer ?

felixcheung · 2017-07-01T04:33:36Z


+## Notebook Storage in Hdfs repository <a name="Hdfs"></a>
+
+Notes may be stored in hdfs, so that multiple zeppelin instance can share the same notes. It supported all the versions of hadoop 2.x. If you use `HdfsNotebookRepo`, then `zeppelin.notebook.dir` is the path on hdfs. And you need to specify `HADOOP_CONF_DIR` in `zeppelin-env.sh` so that zeppelin can find the right hadoop configuration files.


capital Zeppelin
Notes may be stored in hdfs, so that multiple zeppelin instance can share the same notes.
if this is the intended goal, will there be any race condition on write when several instances are changing it the same time?

Right, but I think it is supposed to be handled in a base class.

felixcheung · 2017-07-01T04:35:50Z

+ <dependency>
+ <groupId>org.apache.hadoop</groupId>
+ <artifactId>hadoop-client</artifactId>
+ <version>2.6.0</version>


use a property, hadoop.version instead of hardcoding to 2.6

felixcheung · 2017-07-01T04:37:58Z

+ InputStream in = new ByteArrayInputStream(note.toJson().getBytes("UTF-8"));
+ IOUtils.copyBytes(in, fs.create(tmpNotePath), hadoopConf);
+ Path notePath = new Path(notebookDir.toString() + "/" + note.getId() + "/note.json");
+ fs.delete(notePath);


should it overwrite without warning like this?

I am afraid this is what we only can do for now. Each time when we save a note, it would overwrite the previous one. And actually you don't know the file is the previous note or other file.

khalidhuseynov · 2017-07-01T12:12:45Z


+## Notebook Storage in Hdfs repository <a name="Hdfs"></a>
+
+Notes may be stored in hdfs, so that multiple zeppelin instance can share the same notes. It supported all the versions of hadoop 2.x. If you use `HdfsNotebookRepo`, then `zeppelin.notebook.dir` is the path on hdfs. And you need to specify `HADOOP_CONF_DIR` in `zeppelin-env.sh` so that zeppelin can find the right hadoop configuration files.


minor: It supports?

khalidhuseynov · 2017-07-01T12:13:23Z

+<property>
+ <name>zeppelin.notebook.storage</name>
+ <value>org.apache.zeppelin.notebook.repo.HdfsNotebookRepo</value>
+ <description>notebook persistence layer implementation</description>


i guess should be changed to same description as above with hdfs

khalidhuseynov · 2017-07-01T12:39:20Z

+ if (fs.exists(tmpNotePath)) {
+ fs.delete(tmpNotePath, true);
+ }
+ InputStream in = new ByteArrayInputStream(note.toJson().getBytes("UTF-8"));


i think we would need zConf.ZEPPELIN_ENCODING instead of hard-coding UTF-8 in all these encoding cases

khalidhuseynov · 2017-07-01T13:00:34Z

+ public List<NoteInfo> list(AuthenticationInfo subject) throws IOException {
+ List<NoteInfo> noteInfos = new ArrayList<>();
+ for (FileStatus status : fs.globStatus(new Path(notebookDir, "*/note.json"))) {
+ NoteInfo noteInfo = new NoteInfo(status.getPath().getParent().getName(), "", null);


shouldn't we create Note object first and then new NoteInfo(note)? otherwise it would be missing name and config

This is what I am confused. I don't know the purpose of NoteInfo, previously I thought NoteInfo represent the basic metadata of Note, not include the note content. And from my testing, this PR works well. Besides, I create ZEPPELIN-2712 that we would read Note twice, the root cause is that we construct NoteInfo from Note.

good point, as i checked only Id field of NoteInfo is used to retrieve the whole note which would lead to duplicate get operation. from another point of view if there's no special folder structure as noteId/note.json it would make more sense to retrieve List<Note> in list() method. then the way used here looks good for now

jongyoul · 2017-07-06T01:23:42Z

@zjffdu Almost conflicts will be removed by extracting jars but how we guarantee those libraries are not related my features? I'm just curious.

zjffdu · 2017-07-06T01:32:07Z

@jongyoul Yeah, since your PR is also using the hadoop jar. I think we need to do 2 things:

Use the same hadoop jar version
Decide whether include it in distribution or not. For this, I personally prefer to include the hadoop jar in distribution. Several Reasons:
- It is much easy to implement, we don't need to find the jars at runtime by ourselves.
- Zeppelin might be installed in a gateway machine where hadoop is not installed, in this case, zeppelin would not work because if could not find the hadoop jars.
- Spark put hadoop jar in its distribution and approve it can work, so I think we can trust this approach.

Let me know your concern, thanks.

hayssams · 2017-07-31T15:03:51Z

@zjffdu I think that the zeppelin.sh file should be updated with the HADOOP_CONF_DIR in the classpath

zjffdu · 2017-08-01T00:03:27Z

Thanks for review. @hayssams I did that in zeppelin-daemon.sh.

hayssams · 2017-08-01T15:30:18Z

Yes but sometimes we need to launch zeppelin through zeppelin.sh equally in Mesos by Marathon

zjffdu · 2017-08-02T00:11:32Z

@hayssams Any reason why zeppelin-daemon.sh doesn't fit for you ? Because zeppelin does not guarantee zeppelin.sh can launch zeppelin server properly. zeppelin-daemon.sh not only add HADOOP_CONF_DIR but also other libraries, if you don't use zeppelin-daemon.sh, zeppelin-server may fail to launch.

hayssams · 2017-08-02T04:25:54Z

@zjffdu Yes when you want to start zeppelin not as a daemon.
Please also note the docker image rely on zeppelin.sh

zjffdu · 2017-08-04T08:58:07Z

Also add HADOOP_CONF_DIR in zeppelin.sh, but it looks like there's some code duplication between zeppelin.sh and zeppelin-daemon.sh, we need a followup PR to remove these duplication.

zjffdu · 2017-08-21T08:09:16Z

I also verified the kerberized hdfs manually in my local box. @jongyoul @Leemoonsoo @felixcheung @prabhjyotsingh @hayssams Please help review it, Thanks

prabhjyotsingh · 2017-08-21T09:38:51Z

+
+ public <T> T callHdfsOperation(final HdfsOperation<T> func) throws IOException {
+ if (isSecurityEnabled) {
+ UserGroupInformation.getLoginUser().reloginFromKeytab();


re login on every operation?

reloginFromKeytab() will check whether the keytab is expired, if not it won't relogin
https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java#L1288

prabhjyotsingh · 2017-08-21T09:39:11Z

Tested on local, works as expected, have tried on both environments, with and without Kerberos.
LGTM, except a minor comment.

zjffdu · 2017-08-22T11:16:44Z

Thanks for review @prabhjyotsingh @hayssams , will merge it if no more comments

felixcheung · 2017-08-22T07:45:20Z

+ public Void call() throws IOException {
+ Path noteFolder = new Path(notebookDir.toString() + "/" + noteId);
+ fs.delete(noteFolder, true);
+ return null;


there's no return value from this method as declared in the signature?

Right, there's nothing needs to be return for this method.

… jar) This PR is trying to add hdfs as another implementation for `NotebookRepo`. There's another PR about using webhdfs to implement that. Actually hdfs client library is compatibility cross major versions. See http://hadoop.apache.org/docs/stable/hadoop-project-dist/hadoop-common/Compatibility.html#Wire_compatibility, if using webhdfs, the code become more complicated and may lose some features of hdfs. This PR is also required for HA of zeppelin, so that multiple zeppelin instances can share notes via hdfs. I add hadoop-client in pom file. So zeppelin will package hadoop client jar into its binary distribution. This is because zeppelin may be installed in a gateway machine where no hadoop is installed (only hadoop configuration file is existed in this machine) And since the hadoop client will work with multiple versions of hadoop, so it is fine to package into binary distribution. Spark also package hadoop client jar in its binary distribution. [Feature] * [ ] - Task * https://issues.apache.org/jira/browse/ZEPPELIN-1515 Unit test is added. Also manually verify it in a single node cluster. * Does the licenses files need update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Author: Jeff Zhang <zjffdu@apache.org> Closesapache#2455 from zjffdu/ZEPPELIN-1515 and squashes the following commits: b3e83ab [Jeff Zhang] ZEPPELIN-1515. Notebook: HDFS as a backend storage (Read & Write Mode) (cherry picked from commit 30bfcae) Change-Id: Iecfbd1b5d2f1cb6b3c167906091e2c90524eb787

zjffdu mentioned this pull request Jun 30, 2017
Notebook: (Web) HDFS as a backend storage (Read & Write Mode) #2333
Closed

felixcheung reviewed Jul 1, 2017
View reviewed changes

khalidhuseynov reviewed Jul 1, 2017
View reviewed changes

zjffdu force-pushed the ZEPPELIN-1515 branch 2 times, most recently from e31a803 to 960d9f3Compare July 1, 2017 12:32

khalidhuseynov reviewed Jul 1, 2017
View reviewed changes

zjffdu force-pushed the ZEPPELIN-1515 branch from 960d9f3 to 8b5a67aCompare July 1, 2017 12:47

khalidhuseynov reviewed Jul 1, 2017
View reviewed changes

zjffdu changed the title ~~ZEPPELIN-1515. Notebook: HDFS as a backend storage (Read & Write Mode)~~ZEPPELIN-1515. Notebook: HDFS as a backend storage (Use hadoop client jar)Jul 2, 2017

zjffdu force-pushed the ZEPPELIN-1515 branch from 8b5a67a to 71b978dCompare July 14, 2017 13:12

zjffdu closed this Jul 17, 2017

zjffdu reopened this Jul 17, 2017

zjffdu force-pushed the ZEPPELIN-1515 branch from 9577226 to 8928b64Compare August 4, 2017 09:15

zjffdu mentioned this pull request Aug 19, 2017
[ZEPPELIN-1515] Use Hadoop-FileSystem api to implement NotebookRepo #2529
Closed
1 task

zjffdu force-pushed the ZEPPELIN-1515 branch 3 times, most recently from 54bb18e to 7059244Compare August 21, 2017 08:03

prabhjyotsingh reviewed Aug 21, 2017
View reviewed changes

zjffdu closed this Aug 21, 2017

zjffdu reopened this Aug 21, 2017

hayssams approved these changes Aug 21, 2017
View reviewed changes

prabhjyotsingh approved these changes Aug 22, 2017
View reviewed changes

ZEPPELIN-1515. Notebook: HDFS as a backend storage (Read & Write Mode)
b3e83ab

zjffdu force-pushed the ZEPPELIN-1515 branch from 7059244 to b3e83abCompare August 22, 2017 06:15

felixcheung approved these changes Aug 22, 2017
View reviewed changes

asfgit closed this in 30bfcaeAug 24, 2017


		## Notebook Storage in Hdfs repository <a name="Hdfs"></a>

		Notes may be stored in hdfs, so that multiple zeppelin instance can share the same notes. It supported all the versions of hadoop 2.x. If you use `HdfsNotebookRepo`, then `zeppelin.notebook.dir` is the path on hdfs. And you need to specify `HADOOP_CONF_DIR` in `zeppelin-env.sh` so that zeppelin can find the right hadoop configuration files.

Uh oh!

Conversation

zjffdu commented Jun 30, 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?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

Uh oh!

zjffdu commented Jun 30, 2017

Uh oh!

jongyoul commented Jun 30, 2017

Uh oh!

zjffdu commented Jun 30, 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!

Choose a reason for hiding this comment

Uh oh!

khalidhuseynovJul 1, 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!

khalidhuseynovJul 1, 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!

jongyoul commented Jul 6, 2017

Uh oh!

zjffdu commented Jul 6, 2017

Uh oh!

hayssams commented Jul 31, 2017

Uh oh!

zjffdu commented Aug 1, 2017

Uh oh!

hayssams commented Aug 1, 2017

Uh oh!

zjffdu commented Aug 2, 2017

Uh oh!

hayssams commented Aug 2, 2017

Uh oh!

zjffdu commented Aug 4, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zjffdu commented Aug 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prabhjyotsingh commented Aug 21, 2017

Uh oh!

zjffdu commented Aug 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

zjffdu commented Jun 30, 2017•
edited
Loading

khalidhuseynovJul 1, 2017•
edited
Loading

khalidhuseynovJul 1, 2017•
edited
Loading

zjffdu commented Aug 4, 2017•
edited
Loading