You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by prabhjyotsingh <gi...@git.apache.org> on 2015/11/26 11:14:51 UTC
[GitHub] incubator-zeppelin pull request: Option to restart interpreter fro...
GitHub user prabhjyotsingh opened a pull request:
https://github.com/apache/incubator-zeppelin/pull/480
Option to restart interpreter from paragraph.
In case of operation timeout, or any event in which user might think a restart to interpreter might fix the error in paragraph. A shortcut to restart that specific interpreter from the console, without going to the setting menu.
<img width="1440" alt="screen shot 2015-11-26 at 3 29 06 pm" src="https://cloud.githubusercontent.com/assets/674497/11420341/770bb530-9453-11e5-9dfa-14ec941a693e.png">
<img width="1440" alt="screen shot 2015-11-26 at 3 29 27 pm" src="https://cloud.githubusercontent.com/assets/674497/11420342/7730aa5c-9453-11e5-8bf2-b7b6b9b5d90b.png">
<img width="1439" alt="screen shot 2015-11-26 at 3 32 02 pm" src="https://cloud.githubusercontent.com/assets/674497/11420343/77519550-9453-11e5-8f39-4a35f0bab2f6.png">
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/prabhjyotsingh/incubator-zeppelin restartInterperter
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-zeppelin/pull/480.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 #480
----
commit 1301f6c2a52c6f67e5661b921ca65175fda117d6
Author: Prabhjyot Singh <pr...@gmail.com>
Date: 2015-11-26T10:05:15Z
Option to restart interpreter from paragraph.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Option to restart interpreter fro...
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/480#issuecomment-184974923
Could you please rebase again.
@Leemoonsoo @corneadoug comments?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Option to restart interpreter fro...
Posted by corneadoug <gi...@git.apache.org>.
Github user corneadoug commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/480#issuecomment-194783809
Sorry,
I had a pile of notifications.
It depends on how the error is detected, the best could be to fire the modal on error (if multiple errors, they check if the modal is fired or not), which would suppress the button and be only the modal shown before.
The only problem overall would be the timing of those errors (in case you already closed the modal or restarted the interpreter)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Option to restart interpreter fro...
Posted by r-kamath <gi...@git.apache.org>.
Github user r-kamath commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/480#issuecomment-165386267
@prabhjyotsingh rebase
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Option to restart interpreter fro...
Posted by prabhjyotsingh <gi...@git.apache.org>.
Github user prabhjyotsingh commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/480#issuecomment-161549542
Have modified logic to show restart button only there is "timed out" or "thrift" related exception.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Option to restart interpreter fro...
Posted by prabhjyotsingh <gi...@git.apache.org>.
Github user prabhjyotsingh commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/480#issuecomment-187107089
@corneadoug Yes, I did have a thought about of having `$scope.paragraph.interpreterRestarting` at Notebook level, but then it more suited here.
For `checkErrorForRestart()`, I thought about it, but then its `(paragraph.status == 'ERROR') && checkErrorForRestart()`. So, only when there is an error this will get called, so it think it should be ok.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Option to restart interpreter fro...
Posted by prabhjyotsingh <gi...@git.apache.org>.
Github user prabhjyotsingh commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/480#issuecomment-160531815
Yes, that does make sense.
How about if we try to look for string/error related to transport or texception (thrift) and then show it.
And giving restart at a notebook level makes an impression (to me) that there is problem with the whole notebook and after restart I need to re-run the entire notebook.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Option to restart interpreter fro...
Posted by prabhjyotsingh <gi...@git.apache.org>.
Github user prabhjyotsingh commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/480#issuecomment-190151951
@felixcheung Sure, that's definitely a better idea, implemented that.
Also, moved restartInterpreterSetting function to notebook.controller.js
<img width="1440" alt="screen shot 2016-02-29 at 4 17 48 pm" src="https://cloud.githubusercontent.com/assets/674497/13392400/3b377ccc-df00-11e5-8237-edec8d6ef4a6.png">
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Option to restart interpreter fro...
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/480#issuecomment-191637577
agreed, and "except" would be rather common.
perhaps @corneadoug would have an suggestion on where to put a button or a dialog or something?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Option to restart interpreter fro...
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/480#issuecomment-187440655
I guess from the usability point of view, would it be confusing - because restarting interpreter at a particular paragraph, but all local variables would be cleared right? The user would need restart running from the top? And not just re-run this paragraph?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Option to restart interpreter fro...
Posted by prabhjyotsingh <gi...@git.apache.org>.
Github user prabhjyotsingh commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/480#issuecomment-187570184
@felixcheung I know it can be confusing (or ambiguous if run all was used), and yes I agree if restart, it may clear local variables depending upon interpreter (in case of spark yes).
So, will it make sense if we alter this message https://github.com/apache/incubator-zeppelin/pull/480/files#diff-52fa6628f0b6793d22ffe1f3e4763635R2110
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Option to restart interpreter fro...
Posted by prabhjyotsingh <gi...@git.apache.org>.
Github user prabhjyotsingh commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/480#issuecomment-169570766
Rebased with master.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Option to restart interpreter fro...
Posted by prabhjyotsingh <gi...@git.apache.org>.
Github user prabhjyotsingh commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/480#issuecomment-185082408
Rebased with master.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Option to restart interpreter fro...
Posted by corneadoug <gi...@git.apache.org>.
Github user corneadoug commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/480#issuecomment-187250818
We can keep it like that for now
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Option to restart interpreter fro...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/480#issuecomment-160251125
I think providing a button to restart interpreter in the notebook page is good idea.
But 'Restart Interpreter' button seem to displayed when paragraph status is ERROR. However, paragraph can be ERROR status with such things like syntax error, table not found etc. So ERROR status does not mean Interpreter is required.
And can think little bit more about position of the restart interpreter button and message? Is there any alternative place? because of interpreter is notebook specific. I think showing button in notebook is more intuitive than showing button in paragraph.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Option to restart interpreter fro...
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/480#issuecomment-189786658
@prabhjyotsingh probably - we could say 'restart this interpreter X - note that all interpreter states and local variables will be reset' or similar. Would be good to name the interpreter too, to be clear.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Option to restart interpreter fro...
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/480#issuecomment-191077565
@Leemoonsoo @corneadoug any more thought on this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Option to restart interpreter fro...
Posted by corneadoug <gi...@git.apache.org>.
Github user corneadoug commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/480#issuecomment-185301442
There is a few values or function, that seems out of place to me, for example:
`$scope.paragraph.interpreterRestarting` should probably not be at paragraph level, but at Notebook level since interpreters are linked to Notebook. (and multiple paragraphs might share the same interpreter)
But since I don't have good solutions to re-organize the code better (function and communication between controllers - wise), I think its fine.
However, for the `$scope.checkErrorForRestart()`, it is usually bad for performances to link a function to a ng-show/hide or ng-if. Sometimes, it seems inevitable, however in this case we could probably assign a value to this error and change if when we process the query result.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Option to restart interpreter fro...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/480#issuecomment-191457366
I still think there should be better place to put "interpreter restart" button, than the inside of paragraph.
Let's say, user run multiple paragraphs. They all have connection failure. and they all show "interpreter restart" button under. User click one of them, and rerun paragraph, but the other buttons in the other paragraphs will still remain there. Isn't it make some confusions?
Another thing is, how it detects the error.
Current detection routine search for one of "timed out", "thrift", "except", "connection refused" string from error message. But they're quite common words to be shown in any other error messages and we can not really sure it's interpreter connectivity error or not.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---