Uploaded image for project: 'Zeppelin'
  1. Zeppelin
  2. ZEPPELIN-1770

Restart only the client user's interpreter when restarting interpreter setting

    Details

    • Type: New Feature
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 0.7.0
    • Fix Version/s: 0.7.0, 0.8.0
    • Component/s: None
    • Labels:
      None

      Description

      ZEPPELIN-1306 implement the feature to restart the interpreter associated with the notes, this ticket is to implement to restart the interpreter associated with the user if user click to restart in the interpreter setting page.

      Actually I think in future multiple user would be the major use case for zeppelin, so we should take user as the first dimension/parameter for any operation as opposed before we take noteId as the first dimension/parameter.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user zjffdu opened a pull request:

          https://github.com/apache/zeppelin/pull/1846

          ZEPPELIN-1770. Restart only the client user's interpreter when restarting interpreter setting

              1. What is this PR for?
                This PR would only restart the trigger user's interpreter rather than all the interpreter. So that restarting won't affect other users.
              1. What type of PR is it?
                [Improvement]
              1. Todos
          • [ ] - Task
              1. What is the Jira issue?
              1. How should this be tested?
                Tested manually.
              1. Screenshots (if appropriate)
              1. Questions:
          • Does the licenses files need update? No
          • Is there breaking changes for older versions? No
          • Does this needs documentation? No

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/zjffdu/zeppelin ZEPPELIN-1770

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/zeppelin/pull/1846.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #1846


          commit 903322f97f4b719abfd13bfb9e6baeda1da52493
          Author: Jeff Zhang <zjffdu@apache.org>
          Date: 2016-12-14T07:04:20Z

          ZEPPELIN-1770. Restart only the client user's interpreter when restarting interpreter setting


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user zjffdu opened a pull request: https://github.com/apache/zeppelin/pull/1846 ZEPPELIN-1770 . Restart only the client user's interpreter when restarting interpreter setting What is this PR for? This PR would only restart the trigger user's interpreter rather than all the interpreter. So that restarting won't affect other users. What type of PR is it? [Improvement] Todos [ ] - Task What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-1770 How should this be tested? Tested manually. Screenshots (if appropriate) Questions: Does the licenses files need update? No Is there breaking changes for older versions? No Does this needs documentation? No You can merge this pull request into a Git repository by running: $ git pull https://github.com/zjffdu/zeppelin ZEPPELIN-1770 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zeppelin/pull/1846.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1846 commit 903322f97f4b719abfd13bfb9e6baeda1da52493 Author: Jeff Zhang <zjffdu@apache.org> Date: 2016-12-14T07:04:20Z ZEPPELIN-1770 . Restart only the client user's interpreter when restarting interpreter setting
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user felixcheung commented on the issue:

          https://github.com/apache/zeppelin/pull/1846

          oh gosh
          is it possible to have test for this?

          Show
          githubbot ASF GitHub Bot added a comment - Github user felixcheung commented on the issue: https://github.com/apache/zeppelin/pull/1846 oh gosh is it possible to have test for this?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jongyoul commented on the issue:

          https://github.com/apache/zeppelin/pull/1846

          Good catch. LGTM @felixcheung We had too big InterpreterFactory, InterpreterSetting and InterpreterGroup. I will refactor all of this class by next major release - 0.8.0 - and will make them testable. I think we have to do it now.

          Show
          githubbot ASF GitHub Bot added a comment - Github user jongyoul commented on the issue: https://github.com/apache/zeppelin/pull/1846 Good catch. LGTM @felixcheung We had too big InterpreterFactory, InterpreterSetting and InterpreterGroup. I will refactor all of this class by next major release - 0.8.0 - and will make them testable. I think we have to do it now.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user felixcheung commented on the issue:

          https://github.com/apache/zeppelin/pull/1846

          Sounds good!!

          Show
          githubbot ASF GitHub Bot added a comment - Github user felixcheung commented on the issue: https://github.com/apache/zeppelin/pull/1846 Sounds good!!
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zjffdu commented on the issue:

          https://github.com/apache/zeppelin/pull/1846

          @jongyoul agreen on refactoring Interpreter component, I also mention that in other PRs. I'd love to help on that

          Show
          githubbot ASF GitHub Bot added a comment - Github user zjffdu commented on the issue: https://github.com/apache/zeppelin/pull/1846 @jongyoul agreen on refactoring Interpreter component, I also mention that in other PRs. I'd love to help on that
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Leemoonsoo commented on the issue:

          https://github.com/apache/zeppelin/pull/1846

          @zjffdu Thanks for the patch!

          I also think this PR deserves a unittest to make sure correct restart behavior, like @felixcheung mentioned, if it is not too much difficult.

          Show
          githubbot ASF GitHub Bot added a comment - Github user Leemoonsoo commented on the issue: https://github.com/apache/zeppelin/pull/1846 @zjffdu Thanks for the patch! I also think this PR deserves a unittest to make sure correct restart behavior, like @felixcheung mentioned, if it is not too much difficult.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zjffdu commented on the issue:

          https://github.com/apache/zeppelin/pull/1846

          @Leemoonsoo Could you point me the right place to put this unit test? I think this may need to enable shiro, but I didn't find any test with shiro enabled.

          Show
          githubbot ASF GitHub Bot added a comment - Github user zjffdu commented on the issue: https://github.com/apache/zeppelin/pull/1846 @Leemoonsoo Could you point me the right place to put this unit test? I think this may need to enable shiro, but I didn't find any test with shiro enabled.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Leemoonsoo commented on the issue:

          https://github.com/apache/zeppelin/pull/1846

          [AuthenticationIT.java](https://github.com/apache/zeppelin/blob/master/zeppelin-server/src/test/java/org/apache/zeppelin/integration/AuthenticationIT.java) is test that uses zeppelin with shiro enabled. However it's selenium test and bit difficult to make.

          I think it's possible to make unittest by calling methods in InterpreterFactory class, directly.
          Like [this test](https://github.com/apache/zeppelin/blob/master/zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/InterpreterFactoryTest.java#L148).

          Let me know if you need any help.

          Show
          githubbot ASF GitHub Bot added a comment - Github user Leemoonsoo commented on the issue: https://github.com/apache/zeppelin/pull/1846 [AuthenticationIT.java] ( https://github.com/apache/zeppelin/blob/master/zeppelin-server/src/test/java/org/apache/zeppelin/integration/AuthenticationIT.java ) is test that uses zeppelin with shiro enabled. However it's selenium test and bit difficult to make. I think it's possible to make unittest by calling methods in InterpreterFactory class, directly. Like [this test] ( https://github.com/apache/zeppelin/blob/master/zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/InterpreterFactoryTest.java#L148 ). Let me know if you need any help.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zjffdu commented on the issue:

          https://github.com/apache/zeppelin/pull/1846

          Thanks @Leemoonsoo Unit test is added. Please help review \cc @felixcheung @jongyoul

          Show
          githubbot ASF GitHub Bot added a comment - Github user zjffdu commented on the issue: https://github.com/apache/zeppelin/pull/1846 Thanks @Leemoonsoo Unit test is added. Please help review \cc @felixcheung @jongyoul
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zjffdu closed the pull request at:

          https://github.com/apache/zeppelin/pull/1846

          Show
          githubbot ASF GitHub Bot added a comment - Github user zjffdu closed the pull request at: https://github.com/apache/zeppelin/pull/1846
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user zjffdu reopened a pull request:

          https://github.com/apache/zeppelin/pull/1846

          ZEPPELIN-1770. Restart only the client user's interpreter when restarting interpreter setting

              1. What is this PR for?
                This PR would only restart the trigger user's interpreter rather than all the interpreter. So that restarting won't affect other users.
              1. What type of PR is it?
                [Improvement]
              1. Todos
          • [ ] - Task
              1. What is the Jira issue?
              1. How should this be tested?
                Tested manually.
              1. Screenshots (if appropriate)
              1. Questions:
          • Does the licenses files need update? No
          • Is there breaking changes for older versions? No
          • Does this needs documentation? No

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/zjffdu/zeppelin ZEPPELIN-1770

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/zeppelin/pull/1846.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #1846


          commit 8cb28a31b96c71d749f40e850e886958954136ff
          Author: Jeff Zhang <zjffdu@apache.org>
          Date: 2016-12-14T07:04:20Z

          ZEPPELIN-1770. Restart only the client user's interpreter when restarting interpreter setting

          commit 5ee076dd93d74d50f34aa3ef27b49450b714c5f2
          Author: Jeff Zhang <zjffdu@apache.org>
          Date: 2017-01-10T06:07:33Z

          fix scoped mode and add unit test


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user zjffdu reopened a pull request: https://github.com/apache/zeppelin/pull/1846 ZEPPELIN-1770 . Restart only the client user's interpreter when restarting interpreter setting What is this PR for? This PR would only restart the trigger user's interpreter rather than all the interpreter. So that restarting won't affect other users. What type of PR is it? [Improvement] Todos [ ] - Task What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-1770 How should this be tested? Tested manually. Screenshots (if appropriate) Questions: Does the licenses files need update? No Is there breaking changes for older versions? No Does this needs documentation? No You can merge this pull request into a Git repository by running: $ git pull https://github.com/zjffdu/zeppelin ZEPPELIN-1770 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zeppelin/pull/1846.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1846 commit 8cb28a31b96c71d749f40e850e886958954136ff Author: Jeff Zhang <zjffdu@apache.org> Date: 2016-12-14T07:04:20Z ZEPPELIN-1770 . Restart only the client user's interpreter when restarting interpreter setting commit 5ee076dd93d74d50f34aa3ef27b49450b714c5f2 Author: Jeff Zhang <zjffdu@apache.org> Date: 2017-01-10T06:07:33Z fix scoped mode and add unit test
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zjffdu commented on the issue:

          https://github.com/apache/zeppelin/pull/1846

          CI failure is not relevant.

          ```
          Failed tests:
          NotebookTest.testAbortParagraphStatusOnInterpreterRestart:760 expected:<ABORT> but was:<RUNNING>
          AngularElem

          • should provide onclick method *** FAILED ***
            The code passed to eventually never returned normally. Attempted 1 times over 2.966494567 seconds. Last failure message: 0 was not equal to 1. (AbstractAngularElemTest.scala:64)
            AngularElem
            ```
          Show
          githubbot ASF GitHub Bot added a comment - Github user zjffdu commented on the issue: https://github.com/apache/zeppelin/pull/1846 CI failure is not relevant. ``` Failed tests: NotebookTest.testAbortParagraphStatusOnInterpreterRestart:760 expected:<ABORT> but was:<RUNNING> AngularElem should provide onclick method *** FAILED *** The code passed to eventually never returned normally. Attempted 1 times over 2.966494567 seconds. Last failure message: 0 was not equal to 1. (AbstractAngularElemTest.scala:64) AngularElem ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jongyoul commented on the issue:

          https://github.com/apache/zeppelin/pull/1846

          Tests LGTM. Thanks!

          Show
          githubbot ASF GitHub Bot added a comment - Github user jongyoul commented on the issue: https://github.com/apache/zeppelin/pull/1846 Tests LGTM. Thanks!
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zjffdu commented on the issue:

          https://github.com/apache/zeppelin/pull/1846

          ping @jongyoul @Leemoonsoo @felixcheung

          Show
          githubbot ASF GitHub Bot added a comment - Github user zjffdu commented on the issue: https://github.com/apache/zeppelin/pull/1846 ping @jongyoul @Leemoonsoo @felixcheung
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jongyoul commented on the issue:

          https://github.com/apache/zeppelin/pull/1846

          LGTM, merging if there's no more discussion

          Show
          githubbot ASF GitHub Bot added a comment - Github user jongyoul commented on the issue: https://github.com/apache/zeppelin/pull/1846 LGTM, merging if there's no more discussion
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/zeppelin/pull/1846

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/zeppelin/pull/1846
          Hide
          agoyal Aman Goyal added a comment -

          I tried to test this functionality by downloading distro from the website and when I restart interpreter for one notebook, it restarts interpreter for all the notebooks.. Can somebody please check if it is working?

          Show
          agoyal Aman Goyal added a comment - I tried to test this functionality by downloading distro from the website and when I restart interpreter for one notebook, it restarts interpreter for all the notebooks.. Can somebody please check if it is working?
          Hide
          zjffdu Jeff Zhang added a comment -

          Which mode do you ? It only works in per user mode where each user own one interpreter.

          Show
          zjffdu Jeff Zhang added a comment - Which mode do you ? It only works in per user mode where each user own one interpreter.
          Hide
          agoyal Aman Goyal added a comment -

          I tried in both isolated and scoped mode.. I am trying to share spark interpreter between user1 and user2... But when I restart spark interpreter on user1 by going to interpreter binding; it restarts spark interpreter for user2 too... Is it the indent functionality?

          Show
          agoyal Aman Goyal added a comment - I tried in both isolated and scoped mode.. I am trying to share spark interpreter between user1 and user2... But when I restart spark interpreter on user1 by going to interpreter binding; it restarts spark interpreter for user2 too... Is it the indent functionality?
          Hide
          agoyal Aman Goyal added a comment -

          Also I see CI is failing.. Does it have any effect on merge for this story

          Show
          agoyal Aman Goyal added a comment - Also I see CI is failing.. Does it have any effect on merge for this story

            People

            • Assignee:
              zjffdu Jeff Zhang
              Reporter:
              zjffdu Jeff Zhang
            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development