You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by FRosner <gi...@git.apache.org> on 2017/03/10 12:44:59 UTC
[GitHub] zeppelin pull request #2120: WIP ZEPPELIN-1492
GitHub user FRosner opened a pull request:
https://github.com/apache/zeppelin/pull/2120
WIP ZEPPELIN-1492
### What is this PR for?
A few sentences describing the overall goals of the pull request's commits.
First time? Check out the contributing guide - https://zeppelin.apache.org/contribution/contributions.html
### What type of PR is it?
[Bug Fix | Improvement | Feature | Documentation | Hot Fix | Refactoring]
### Todos
* [ ] - Task
### What is the Jira issue?
* Open an issue on Jira https://issues.apache.org/jira/browse/ZEPPELIN/
* Put link here, and add [ZEPPELIN-*Jira number*] in PR title, eg. [ZEPPELIN-533]
### How should this be tested?
Outline the steps to test the PR here.
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update?
* Is there breaking changes for older versions?
* Does this needs documentation?
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/FRosner/incubator-zeppelin ZEPPELIN-1492
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/2120.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 #2120
----
commit f9ca8c4fe77f0f43de62e10d42912d50a8542009
Author: Frank Rosner <fr...@fam-rosner.de>
Date: 2017-03-10T12:38:06Z
ZEPPELIN-1492 also update paragraph if the text has changed
----
---
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] zeppelin issue #2120: [ZEPPELIN-1492] fixing the issue where updating a para...
Posted by sergeymazin <gi...@git.apache.org>.
Github user sergeymazin commented on the issue:
https://github.com/apache/zeppelin/pull/2120
Issue we are facing is when somebody else is adding/renaming/deleting any other notebook, unsaved text in current notebook is lost.
Renaming any other notebook resets current notebook:
Deleting any other notebook resets current notebook:
Same comes with adding/importing new notebooks. This makes Zeppelin unusable when working in multitenant mode once somebody starts importing/renaming notebooks.
---
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] zeppelin issue #2120: [ZEPPELIN-1492] fixing the issue where updating a para...
Posted by 1ambda <gi...@git.apache.org>.
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2120
Thanks for fixing bug. I will review and give you feedback soon.
---
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] zeppelin issue #2120: [ZEPPELIN-1492] fixing the issue where updating a para...
Posted by 1ambda <gi...@git.apache.org>.
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2120
@FRosner I see and agree with that we need more realtime notification on paragraph editing.
LGTM
---
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] zeppelin issue #2120: [ZEPPELIN-1492] fixing the issue where updating a para...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the issue:
https://github.com/apache/zeppelin/pull/2120
@FRosner Jenkins checks `https://travis-ci.org/FRosner/zeppelin` instead of `https://travis-ci.org/FRosner/incubator-zeppelin` fyi.
---
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] zeppelin issue #2120: [ZEPPELIN-1492] fixing the issue where updating a para...
Posted by FRosner <gi...@git.apache.org>.
Github user FRosner commented on the issue:
https://github.com/apache/zeppelin/pull/2120
Looking at the discussion and the different issues I see what you mean @1ambda. We are actually having two issues:
1. Local changes are overwritten when the notebook gets updates from the server (e.g. someone renames a note)
2. Local changes are not propagated on a regular basis (and you risk bigger conflicts when editing simultaneously)
The first point is addressed by #2176 #2177 more than this PR, because even when saving on losing focus you might risk losing local changes if you didn't lose the focus and someone renames, correct?
The second point is addressed in a way here but as mentioned in one of the other discussions we should really move towards real time updates where every keystroke gets transmitted (or at least while typing maybe after a few seconds) and you can see the cursor of the other users in your paragraph (similar to Google Docs).
Does it make sense?
---
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] zeppelin issue #2120: WIP: ZEPPELIN-1492
Posted by FRosner <gi...@git.apache.org>.
Github user FRosner commented on the issue:
https://github.com/apache/zeppelin/pull/2120
Will do \U0001f44d
---
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] zeppelin pull request #2120: [ZEPPELIN-1492] fixing the issue where updating...
Posted by FRosner <gi...@git.apache.org>.
Github user FRosner closed the pull request at:
https://github.com/apache/zeppelin/pull/2120
---
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] zeppelin issue #2120: WIP: ZEPPELIN-1492
Posted by 1ambda <gi...@git.apache.org>.
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2120
Hi @FRosner. Thanks for contribution.
- Zeppelin build system relies on travis. So please setup your travis account and travis project for Zeppelin.
- https://zeppelin.apache.org/contribution/contributions.html#continuous-integration
---
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] zeppelin issue #2120: [ZEPPELIN-1492] fixing the issue where updating a para...
Posted by FRosner <gi...@git.apache.org>.
Github user FRosner commented on the issue:
https://github.com/apache/zeppelin/pull/2120
Do you want me to recreate the fork? I forked it long time ago :D
--
Sent from my phone. Please excuse my brevity.
On 03/13/2017, 16:05 Lee moon soo <no...@github.com> wrote:
@FRosner Jenkins checks https://travis-ci.org/FRosner/zeppelin instead of https://travis-ci.org/FRosner/incubator-zeppelin fyi.
\u2014
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
---
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] zeppelin issue #2120: [ZEPPELIN-1492] fixing the issue where updating a para...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the issue:
https://github.com/apache/zeppelin/pull/2120
@FRosner Yes, that would be appreciated.
---
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] zeppelin pull request #2120: [ZEPPELIN-1492] fixing the issue where updating...
Posted by FRosner <gi...@git.apache.org>.
GitHub user FRosner reopened a pull request:
https://github.com/apache/zeppelin/pull/2120
[ZEPPELIN-1492] fixing the issue where updating a paragraph was not propagated correctly
### What is this PR for?
This pull request fixes two issues regarding paragraphs not being updated and therefore overwritten unintentionally. The first issue yields to local changes being overwritten when remote changes are made. The second issue yields to changes being overwritten when, e.g., the notebook is renamed.
The first change happens in the `updateParagraph` broadcast event handler function. This function has the purpose to update the local state of the paragraph in the paragraph controller scope when there is an update from the web socket.
However, it did not update the state if the only thing that has changed was the text. Now it will, which fixes the original issue in the issue description. This was one of the issues identified by https://issues.apache.org/jira/browse/ZEPPELIN-1492?focusedCommentId=15744928&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15744928
The second issue is fixed by saving the paragraph when the editor loses focus.
### What type of PR is it?
Bug Fix
### What is the Jira issue?
- https://issues.apache.org/jira/browse/ZEPPELIN-1492
### How should this be tested?
The first issue can be reproduced with the following two browser-windows and a single notebook.
Browser A:
```
%pyspark
print "Original Zeppelin Notebook"
```
In Browser B, edit the notebook and the above command to:
```
%pyspark
print "Notebook is updated...."
```
If I run the notebook via browser A followed by Browser B, everything is updated nicely. Now also if I you add the following line to the Notebook though Browser B:
```
print "....once again"
```
and run the notebook through Browser B again, the content in Browser A will be updated.
The second issue can be reproduced by editing a cell without executing it and renaming the notebook right afterwards. The rename will reset your cell to the previous state. With the fix, your state is saved.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/FRosner/zeppelin ZEPPELIN-1492
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/2120.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 #2120
----
commit f9ca8c4fe77f0f43de62e10d42912d50a8542009
Author: Frank Rosner <fr...@fam-rosner.de>
Date: 2017-03-10T12:38:06Z
ZEPPELIN-1492 also update paragraph if the text has changed
commit 88125e3401c0b90d049f6ac224aec12bef29d85d
Author: Frank Rosner <fr...@fam-rosner.de>
Date: 2017-03-10T14:22:12Z
ZEPPELIN-1492 newPara is already a paragraph I guess :S
commit 62f032259505bacfb23c949f33829bdfa7caa945
Author: Frank Rosner <fr...@fam-rosner.de>
Date: 2017-03-10T15:33:08Z
empty commit to trigger build
commit 34c8174725b694bd06390e1a8e4b55af7fc2e762
Author: Frank Rosner <fr...@fam-rosner.de>
Date: 2017-03-13T10:32:08Z
ZEPPELIN-1492 save paragraph on editor blur
----
---
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] zeppelin issue #2120: [ZEPPELIN-1492] fixing the issue where updating a para...
Posted by FRosner <gi...@git.apache.org>.
Github user FRosner commented on the issue:
https://github.com/apache/zeppelin/pull/2120
Is there something I can do to accelerate the merge process here? Any open questions?
---
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] zeppelin issue #2120: [ZEPPELIN-1492] fixing the issue where updating a para...
Posted by 1ambda <gi...@git.apache.org>.
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2120
@FRosner
I'v fixed 2 issues without changing the default behavior of focus.
Please help review #2176, #2177.
---
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] zeppelin issue #2120: [ZEPPELIN-1492] fixing the issue where updating a para...
Posted by FRosner <gi...@git.apache.org>.
Github user FRosner commented on the issue:
https://github.com/apache/zeppelin/pull/2120
@1ambda it's building fine at travis: https://travis-ci.org/FRosner/incubator-zeppelin
Anything else I need to do? Why is Jenkins complaining?
---
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] zeppelin issue #2120: [ZEPPELIN-1492] fixing the issue where updating a para...
Posted by 1ambda <gi...@git.apache.org>.
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2120
@FRosner Could you attach a GIF for
```
Enter code in a cell
Remove the focus to rename the notebook
The code gets reset
```
---
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] zeppelin issue #2120: [ZEPPELIN-1492] fixing the issue where updating a para...
Posted by 1ambda <gi...@git.apache.org>.
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2120
@FRosner Sorry for late reply. \U0001f62d...
### I can't reproduce the original issue reported in 1492 in current master
Usually, paragraph will be updated If interpreter updates paragraph object properly for example `dataFinished`.
I am not sure there is an case when **other paragraph fields are exactly same and only `paragraph.text` is different**. Only in this case, paragraph will be not updated in current implementation.
```js
// https://github.com/apache/zeppelin/blob/f43d27f0bd9f841d71450ae303dc6f9f671bae0a/zeppelin-web/src/app/notebook/paragraph/paragraph.controller.js#L1123
newPara.dateFinished !== oldPara.dateFinished ||
```
But `newPara.text !== oldPara.text ||` is ok it's more accurate condition on if stmt.
### I can't reproduce focus issue too
in 1492, @FRosner said that
> 1. Enter code in a cell
> 2. Remove the focus to rename the notebook
> 3. The code gets reset
But I can't reproduce in current master.
### Save paragraphs automatically when focus is lost.
So this is more about **save paragraph when focus is lost** instead of resolving the problem submitted in 1492.
```js
$scope.editor.on('blur', function() {
handleFocus(false); handleFocus(false);
+ $scope.saveParagraph($scope.paragraph);
}); });
```
In current implementation Zeppelin will save when user refresh browsers. But this PR will cause more frequent `paragraph.text` overwriting.
I don't have strong opinion about the behavior, so I would like to other people's opinions.
@Leemoonsoo @AhyoungRyu What do you think of?
---
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] zeppelin issue #2120: [ZEPPELIN-1492] fixing the issue where updating a para...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the issue:
https://github.com/apache/zeppelin/pull/2120
@FRosner Thanks for the fix. LGTM and merge to master and branch-0.7
---
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] zeppelin issue #2120: [ZEPPELIN-1492] fixing the issue where updating a para...
Posted by sergeymazin <gi...@git.apache.org>.
Github user sergeymazin commented on the issue:
https://github.com/apache/zeppelin/pull/2120
@1ambda thanks for working on this.
I guess you figured everything out with @FRosner in #2176 and #2177 already. Please let me know if I can help somehow.
---
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] zeppelin pull request #2120: [ZEPPELIN-1492] fixing the issue where updating...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zeppelin/pull/2120
---
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] zeppelin issue #2120: [ZEPPELIN-1492] fixing the issue where updating a para...
Posted by 1ambda <gi...@git.apache.org>.
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2120
@sergeymazin Thanks for quick reply!
I see,
- that's because deleting, renaming or doing other notebook scale actions cause broad casting all notebooks.
- then, all notebooks will get websocket messages that contains persisted contents (including `paragraph.text`)
I will test and comment again in few hours.
---
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] zeppelin issue #2120: [ZEPPELIN-1492] fixing the issue where updating a para...
Posted by FRosner <gi...@git.apache.org>.
Github user FRosner commented on the issue:
https://github.com/apache/zeppelin/pull/2120
Here you go @1ambda
---
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.
---