[ZEPPELIN-1549] Change NotebookID variable name to NoteID #1518

hyonzin · 2016-10-14T07:00:49Z

What is this PR for?

This PR fixes wrong written NotebookID to NoteID.

What type of PR is it?

[Improvement]

What is the Jira issue?

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

Questions:

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

jongyoul · 2016-10-14T07:06:06Z

@hyonzin @cloverhearts Do you change these two files in this PR?

hyonzin · 2016-10-14T07:41:50Z

@jongyoul thank you for review. Yes, and I'm still working in process. I'll fix more files.

astroshim · 2016-10-15T03:20:54Z

It seems looks great to me.
What do you think @jongyoul ?

AhyoungRyu · 2016-10-15T03:48:48Z

Seems there are some test failures in SCALA_VER="2.10" SPARK_VER="1.5.2"

Results :

Tests in error: 
  InterpreterRestApiTest.init:53->AbstractTestRestApi.startUp:159 Â» NullPointer
  CredentialsRestApiTest.init:46->AbstractTestRestApi.startUp:159 Â» NullPointer
  ZeppelinRestApiTest.init:56->AbstractTestRestApi.startUp:159 Â» NullPointer
  ConfigurationsRestApiTest.init:39->AbstractTestRestApi.startUp:159 Â» NullPointer
  NotebookRestApiTest.init:55->AbstractTestRestApi.startUp:159 Â» NullPointer
  ZeppelinSparkClusterTest.init:49->AbstractTestRestApi.startUp:159 Â» NullPointer
  SecurityRestApiTest.init:44->AbstractTestRestApi.startUp:159 Â» NullPointer

Tests run: 7, Failures: 0, Errors: 7, Skipped: 0

[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO] 
[INFO] Zeppelin: Interpreter .............................. SUCCESS [ 14.139 s]
[INFO] Zeppelin: Zengine .................................. SUCCESS [  7.096 s]
[INFO] Zeppelin: Display system apis ...................... SUCCESS [  3.527 s]
[INFO] Zeppelin: Spark dependencies ....................... SUCCESS [ 42.764 s]
[INFO] Zeppelin: Spark .................................... SUCCESS [01:52 min]
[INFO] Zeppelin: Server ................................... FAILURE [ 44.231 s]
[INFO] Zeppelin: R Interpreter ............................ SKIPPED
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 03:44 min
[INFO] Finished at: 2016-10-14T15:35:13+00:00
[INFO] Final Memory: 47M/468M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:2.17:test (default-test) on project zeppelin-server: There are test failures.
[ERROR] 
[ERROR] Please refer to /home/travis/build/apache/zeppelin/zeppelin-server/target/surefire-reports for the individual test results.
[ERROR] -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
[ERROR] 
[ERROR] After correcting the problems, you can resume the build with the command
[ERROR]   mvn <goals> -rf :zeppelin-server

I think they are related with this change..

minahlee

Thanks for the contribution @hyonzin. I left some comments, please take a look.

minahlee · 2016-10-16T06:00:40Z

zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java

      throws IOException, IllegalArgumentException {
-    LOG.info("stop notebook jobs {} ", notebookId);
-    Note note = notebook.getNote(notebookId);
+    LOG.info("stop notebook jobs {} ", noteId);


Shall we change notebook -> note in log too?

minahlee · 2016-10-16T06:09:52Z

zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java

+  public Response getNoteParagraphJobStatus(@PathParam("noteId") String noteId, 
      @PathParam("paragraphId") String paragraphId)
      throws IOException, IllegalArgumentException {
    LOG.info("get notebook paragraph job status.");


Same here. Instead of notebook, note would be more correct.

minahlee · 2016-10-16T06:12:22Z

zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java

  private Notebook notebook;
  private NotebookServer notebookServer;
-  private SearchService notebookIndex;
+  private SearchService noteIndex;


Term noteIndex is little bit confusing for me. Since you are working on naming, how about changing it into noteSearchService?

minahlee · 2016-10-16T06:18:15Z

zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Notebook.java

+  public List<Map<String, Object>> getJobListByNoteId(String noteID) {
    final String CRON_TYPE_NOTEBOOK_KEYWORD = "cron";
    long lastRunningUnixTime = 0;
    boolean isNotebookRunning = false;


isNotebookRunning -> isNoteRunning

minahlee · 2016-10-16T06:21:55Z

zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Notebook.java

  }

-  public List<Map<String, Object>> getJobListBymNotebookId(String notebookID) {
+  public List<Map<String, Object>> getJobListByNoteId(String noteID) {


Could you make it to use Id instead of ID for consistency? We have camel convention.

hyonzin · 2016-10-18T10:35:47Z

@minahlee Thank you for your review! I modified more lines you commented, and some trivial placeholder because I think it's better to read. Is it okay?

hyonzin · 2016-10-18T17:03:21Z

Test in SCALA_VER="2.10" SPARK_VER="1.5.2" still fails. I checked its LOG,

13:50:41,164  INFO org.apache.zeppelin.rest.AbstractTestRestApi:299 - 200 - OK
13:50:41,164  INFO org.apache.zeppelin.rest.AbstractTestRestApi:140 - Test Zeppelin stared.
SPARK HOME detected null

+cp spark-1.5.2-bin-hadoop2.3.tgz ..
+cd ..
+tar zxf spark-1.5.2-bin-hadoop2.3.tgz

gzip: stdin: unexpected end of file
tar: Unexpected EOF in archive
tar: Unexpected EOF in archive
tar: Error is not recoverable: exiting now
+echo 'Unable to extract spark-1.5.2-bin-hadoop2.3.tgz'
Unable to extract spark-1.5.2-bin-hadoop2.3.tgz
+rm -rf spark-1.5.2-bin-hadoop2.3
+rm -f spark-1.5.2-bin-hadoop2.3.tgz
+set +xe

It seems to be disabled to extract .tgz file, so it can't find Spark Home.
And I wonder why it's associated with my modification yet.

Leemoonsoo · 2016-10-18T23:28:59Z

@hyonzin Sometimes, CI tests are flaky. Could you re-trigger CI once again and see if it goes to green?

hyonzin · 2016-10-19T18:41:03Z

I doubted CI's test in this PR, so I tried this test for absolutely same code in my own repository. And it shows all green.
travis-ci.org/hyonzin/zeppelin/builds/168996633
I guess all checks are failed in this PR because of cache - once downloaded incomplete tgz file and compiled object files in past.

minahlee · 2016-10-23T04:14:56Z

@hyonzin Could you rebase please? Let me know if you need any help.

hyonzin · 2016-10-23T18:03:03Z

@minahlee Thank you. I rebase it and solve conflicts now. Please confirm it!
https://travis-ci.org/hyonzin/zeppelin

minahlee · 2016-10-24T06:16:57Z

@hyonzin I found more places to be changed. There were too much of them to comment here so I made PR to your repository. Can you please check and merge it?

Change notebook to note and fix indentation

minahlee · 2016-10-24T10:58:25Z

@hyonzin There is another confilct :( Can you please rebase one more time? Thank you in advance for your patience!

hyonzin · 2016-10-24T11:04:24Z

@minahlee Thank you. I'm going to rebase it! I was checking the failure in test here.

…ELIN-1549

minahlee · 2016-10-24T11:11:04Z

@hyonzin I missed one file... https://github.com/apache/zeppelin/blob/master/zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTest.java#L157 in this line, notebook should be changed to note

hyonzin · 2016-10-24T11:15:34Z

@minahlee I have fixed it now. Really thank you for your great help :)

minahlee · 2016-10-24T11:23:35Z

zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepoSync.java

 import java.util.Date;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;


pushNoteIDs -> pushNoteIds

minahlee · 2016-10-24T11:23:42Z

zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepoSync.java

    if (getRepoCount() == 0) {
      LOG.info("No storage could be initialized, using default {} storage", defaultStorage);
      initializeDefaultStorage(conf);
    }


pushNoteIDs -> pushNoteIds

minahlee · 2016-10-24T13:06:38Z

CI passed in https://travis-ci.org/hyonzin/zeppelin/builds/170100291. Merge if there is no further discussion.

### What is this PR for? This PR fixes wrong written NotebookID to NoteID. ### What type of PR is it? [Improvement] ### What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-1549 ### Questions: * Does the licenses files need update? No. * Is there breaking changes for older versions? No. * Does this needs documentation? No Author: hyonzin <[email protected]> Author: 정현진 <[email protected]> Author: Mina Lee <[email protected]> Closes apache#1518 from hyonzin/ZEPPELIN-1549 and squashes the following commits: 2c5d461 [hyonzin] fix pullNoteID to pullNoteId f843abd [hyonzin] Fix missed line 22aecb3 [hyonzin] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1549 ac03666 [정현진] Merge pull request apache#1 from minahlee/ZEPPELIN-1549 8b3fffd [Mina Lee] Change notebook to note and fix indentation 000605f [hyonzin] Change clonedNotebookId to clonedNoteId 496695c [hyonzin] Change noteID to noteId 1e87463 [hyonzin] Remove tab indent 5647d37 [hyonzin] Rebase and solve conflicts 09bacd8 [hyonzin] Fix more lines unchanged 070bc2d [hyonzin] fix more in ZeppelinRestApiTest.java 24822a3 [hyonzin] Fix more code not changed (notebookIndex to noteSearchService) 4b4e1e8 [hyonzin] Fix detail (function's name) & Change some placeholder 429203d [hyonzin] Fix details & convention to camel 5fa270d [hyonzin] pull upstream master & fix some details 294bea5 [hyonzin] Fix some wrong written term: Notebook -> Note cc0d315 [hyonzin] Change NotebookID variable name to NoteID

### What is this PR for? This PR fixes wrong written NotebookID to NoteID. ### What type of PR is it? [Improvement] ### What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-1549 ### Questions: * Does the licenses files need update? No. * Is there breaking changes for older versions? No. * Does this needs documentation? No Author: hyonzin <[email protected]> Author: 정현진 <[email protected]> Author: Mina Lee <[email protected]> Closes apache#1518 from hyonzin/ZEPPELIN-1549 and squashes the following commits: 2c5d461 [hyonzin] fix pullNoteID to pullNoteId f843abd [hyonzin] Fix missed line 22aecb3 [hyonzin] Merge branch 'master' of https://github.com/apache/zeppelin into ZEPPELIN-1549 ac03666 [정현진] Merge pull request #1 from minahlee/ZEPPELIN-1549 8b3fffd [Mina Lee] Change notebook to note and fix indentation 000605f [hyonzin] Change clonedNotebookId to clonedNoteId 496695c [hyonzin] Change noteID to noteId 1e87463 [hyonzin] Remove tab indent 5647d37 [hyonzin] Rebase and solve conflicts 09bacd8 [hyonzin] Fix more lines unchanged 070bc2d [hyonzin] fix more in ZeppelinRestApiTest.java 24822a3 [hyonzin] Fix more code not changed (notebookIndex to noteSearchService) 4b4e1e8 [hyonzin] Fix detail (function's name) & Change some placeholder 429203d [hyonzin] Fix details & convention to camel 5fa270d [hyonzin] pull upstream master & fix some details 294bea5 [hyonzin] Fix some wrong written term: Notebook -> Note cc0d315 [hyonzin] Change NotebookID variable name to NoteID

Change NotebookID variable name to NoteID

cc0d315

hyonzin changed the title ~~[ZEPPELIN-1549] Change NotebookID variable name to NoteID~~ [ZEPPELIN-1549][WIP] Change NotebookID variable name to NoteID Oct 14, 2016

Fix some wrong written term: Notebook -> Note

294bea5

hyonzin changed the title ~~[ZEPPELIN-1549][WIP] Change NotebookID variable name to NoteID~~ [ZEPPELIN-1549] Change NotebookID variable name to NoteID Oct 14, 2016

hyonzin closed this Oct 15, 2016

hyonzin reopened this Oct 15, 2016

hyonzin changed the title ~~[ZEPPELIN-1549] Change NotebookID variable name to NoteID~~ [ZEPPELIN-1549][WIP] Change NotebookID variable name to NoteID Oct 16, 2016

minahlee reviewed Oct 16, 2016

View reviewed changes

hyonzin added 3 commits October 18, 2016 18:22

pull upstream master & fix some details

5fa270d

Fix details & convention to camel

429203d

Fix detail (function's name) & Change some placeholder

4b4e1e8

Fix more code not changed (notebookIndex to noteSearchService)

24822a3

hyonzin closed this Oct 18, 2016

hyonzin reopened this Oct 18, 2016

hyonzin closed this Oct 19, 2016

hyonzin reopened this Oct 19, 2016

hyonzin closed this Oct 19, 2016

hyonzin reopened this Oct 19, 2016

hyonzin added 2 commits October 19, 2016 16:13

fix more in ZeppelinRestApiTest.java

070bc2d

Fix more lines unchanged

09bacd8

hyonzin changed the title ~~[ZEPPELIN-1549][WIP] Change NotebookID variable name to NoteID~~ [ZEPPELIN-1549] Change NotebookID variable name to NoteID Oct 19, 2016

hyonzin added 4 commits October 24, 2016 01:23

Rebase and solve conflicts

5647d37

Remove tab indent

1e87463

Change noteID to noteId

496695c

Change clonedNotebookId to clonedNoteId

000605f

Change notebook to note and fix indentation

8b3fffd

Merge pull request #1 from minahlee/ZEPPELIN-1549

ac03666

Change notebook to note and fix indentation

Merge branch 'master' of https://github.com/apache/zeppelin into ZEPP…

22aecb3

…ELIN-1549

Fix missed line

f843abd

minahlee reviewed Oct 24, 2016

View reviewed changes

fix pullNoteID to pullNoteId

2c5d461

asfgit closed this in 4f6a0e3 Oct 25, 2016

tae-jun mentioned this pull request Nov 3, 2016

[ZEPPELIN-1586] Add security check in NotebookRestApi #1567

Closed

4 tasks

[ZEPPELIN-1549] Change NotebookID variable name to NoteID #1518

[ZEPPELIN-1549] Change NotebookID variable name to NoteID #1518

Uh oh!

Conversation

hyonzin commented Oct 14, 2016

What is this PR for?

What type of PR is it?

What is the Jira issue?

Questions:

Uh oh!

jongyoul commented Oct 14, 2016

Uh oh!

hyonzin commented Oct 14, 2016

Uh oh!

astroshim commented Oct 15, 2016

Uh oh!

AhyoungRyu commented Oct 15, 2016

Uh oh!

minahlee left a comment

Choose a reason for hiding this comment

Uh oh!

minahlee Oct 16, 2016

Choose a reason for hiding this comment

Uh oh!

minahlee Oct 16, 2016

Choose a reason for hiding this comment

Uh oh!

minahlee Oct 16, 2016

Choose a reason for hiding this comment

Uh oh!

minahlee Oct 16, 2016

Choose a reason for hiding this comment

Uh oh!

minahlee Oct 16, 2016

Choose a reason for hiding this comment

Uh oh!

hyonzin commented Oct 18, 2016

Uh oh!

hyonzin commented Oct 18, 2016

Uh oh!

Leemoonsoo commented Oct 18, 2016

Uh oh!

hyonzin commented Oct 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

minahlee commented Oct 23, 2016

Uh oh!

hyonzin commented Oct 23, 2016

Uh oh!

minahlee commented Oct 24, 2016

Uh oh!

minahlee commented Oct 24, 2016

Uh oh!

hyonzin commented Oct 24, 2016

Uh oh!

minahlee commented Oct 24, 2016

Uh oh!

hyonzin commented Oct 24, 2016

Uh oh!

minahlee Oct 24, 2016

Choose a reason for hiding this comment

Uh oh!

minahlee Oct 24, 2016

Choose a reason for hiding this comment

Uh oh!

minahlee commented Oct 24, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

hyonzin commented Oct 19, 2016 •

edited

Loading