Skip to content

Conversation

@bzz
Copy link
Member

@bzz bzz commented Dec 15, 2015

What is this PR for?

This PR has for goal to allow the user to search through the code in all the paragraphs and notebook names in all the notebooks

It add a simple 'search bar' to the nav-bar of Zeppelin WebApp, and an in-memory fulltext search index of paragraphs to the backend.

The search is pretty basic now, fine-tuning it for better search over all types of source code will be a subject of further work.

What type of PR is it?

Feature

Todos

  • - Fix typos 💃 b853aa6
  • - fix js issue in js console @felizbear 29da337
  • - update index on paragraph CRUD:
    • Read (initial work)
    • Create\Delete 825b266
    • Update e915a69
    • Delete paragraph
  • - add license to zeppelin-distribution/src/bin_license/LICENSE (backend for lucene, not sure about angular-resource as it is part of the AngularJS codebase, but will add just in case) c00b516
  • - add missing Apache headers ded9c3b @felizbear 29da337
  • - fix CI (was failing RAT on zengine Too many files with unapproved license: 2, now flacky integration test AKA ZEPPELIN-510)
  • - index notebook names e80c3e5, @felizbear 29da337
  • - make NotebookRepoSync.sync() package private again 5a18bc8
  • - NPE on persisting changes of existing notebook
  • - reduce log verbosity

Is there a relevant Jira issue?

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

How should this be tested?

Outline the steps to test the PR here.

Screenshots (if appropriate)

screen shot 2015-12-15 at 19 49 18

screen shot 2015-12-15 at 19 52 03

Questions:

  • Does the licenses files need update? Yes, added to license files
  • Is there breaking changes for older versions? No
  • Does this needs documentation? Yes

This work is a collaboration with @felizbear who contributed major parts of the frontend changes.

@vgmartinez
Copy link
Contributor

LGTM...
It is a good feature

@corneadoug
Copy link
Contributor

@bzz Could you rewrite the PR description following the PR template (https://github.com/apache/incubator-zeppelin/blob/master/CONTRIBUTING.md#creating-a-pull-request)?

@bzz
Copy link
Member Author

bzz commented Dec 15, 2015

@vgmartinez than you for positive feedback!

@corneadoug well, I expected a bit more of the code review, rather than "Wrong formatting of the PR description" ;)

I think I did a good job with JIRA issue. If you never the less feel that something is wrong - please contribute a new description you like better in the form of comment here and I will be willing to updated it.

@corneadoug
Copy link
Contributor

@bzz Of course, the code review will come :)

Well, JIRA issue is JIRA issue, PR Template is PR Template :)
You will find that there is a lot of nice categories and questions that will help review the PR.

One good example is:

Does the licenses files need update?

Which answer in this PR's case would be: Yes

But angular-resource was not added to the license file

Copy link
Contributor

Choose a reason for hiding this comment

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

@bzz I think there are some typos in this sentence! : )

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for proof-reading! Added to the scope below

@bzz
Copy link
Member Author

bzz commented Dec 22, 2015

@Leemoonsoo All reviews are addressed, I think it's ready to be merged.

Copy link
Member

Choose a reason for hiding this comment

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

We might not want to print contents of notebook in a log file. Either use LOG.debug or remove 'text' from the log.

@Leemoonsoo
Copy link
Member

@bzz Great work.
Seems like index is not updated after paragraph remove. Is it possible to be handled?

@Leemoonsoo
Copy link
Member

Following error is thrown when persisting changes of notebook. The notebook loaded on Zeppelin startup (not created after start up)

java.lang.NullPointerException
    at org.apache.zeppelin.notebook.Note.persist(Note.java:380)
    at org.apache.zeppelin.socket.NotebookServer.updateParagraph(NotebookServer.java:451)
    at org.apache.zeppelin.socket.NotebookServer.onMessage(NotebookServer.java:123)
    at org.apache.zeppelin.socket.NotebookSocket.onMessage(NotebookSocket.java:56)
    at org.eclipse.jetty.websocket.WebSocketConnectionRFC6455$WSFrameHandler.onFrame(WebSocketConnectionRFC6455.java:835)
    at org.eclipse.jetty.websocket.WebSocketParserRFC6455.parseNext(WebSocketParserRFC6455.java:349)
    at org.eclipse.jetty.websocket.WebSocketConnectionRFC6455.handle(WebSocketConnectionRFC6455.java:225)
    at org.eclipse.jetty.io.nio.SelectChannelEndPoint.handle(SelectChannelEndPoint.java:667)
    at org.eclipse.jetty.io.nio.SelectChannelEndPoint$1.run(SelectChannelEndPoint.java:52)
    at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:608)
    at org.eclipse.jetty.util.thread.QueuedThreadPool$3.run(QueuedThreadPool.java:543)
    at java.lang.Thread.run(Thread.java:745)

@bzz
Copy link
Member Author

bzz commented Dec 23, 2015

@Leemoonsoo thank you for thought testing and review! My bad, scope is updated now, will address all the feedback, and let you know.

@bzz
Copy link
Member Author

bzz commented Dec 23, 2015

@Leemoonsoo all the feedback has been addressed now

@Leemoonsoo
Copy link
Member

Tested and works really well. Thanks for really useful new feature.
LGTM!

@bzz
Copy link
Member Author

bzz commented Dec 23, 2015

Thank you! Merging if there are no other discussions

@asfgit asfgit closed this in 82de508 Dec 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants