You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by 1ambda <gi...@git.apache.org> on 2017/03/22 18:40:31 UTC
[GitHub] zeppelin pull request #2176: [ZEPPELIN-2300] SHOULD NOT update paragraph tex...
GitHub user 1ambda opened a pull request:
https://github.com/apache/zeppelin/pull/2176
[ZEPPELIN-2300] SHOULD NOT update paragraph text when local change exists
### What is this PR for?
**SHOULD NOT** update paragraph text when local change exists
### What type of PR is it?
[Bug Fix]
### Todos
NONE
### What is the Jira issue?
[ZEPPELIN-2300](https://issues.apache.org/jira/browse/ZEPPELIN-2300)
Related with
- https://github.com/apache/zeppelin/pull/2120 (different approach)
- https://issues.apache.org/jira/browse/ZEPPELIN-1492 (people are complaining on this issue. but different topic.)
### How should this be tested?
1. Open the same note in 2 browsers (use different sessions e.g normal chrome + secret chrome or firefox + safari)
2. Prepare a paragraph synchronized among sessions.
3. Modify text in one browser.
4. Run the paragraph in different session.
### Screenshots (if appropriate)
#### Before
#### After
### 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/1ambda/zeppelin ZEPPELIN-2300/should-not-overwrite-local-text
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/2176.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 #2176
----
commit a60be33b34bb9ab3d10ee7010d6726687526dec6
Author: 1ambda <1a...@gmail.com>
Date: 2017-03-22T18:32:36Z
fix: DON'T update paragraph.text when dirty exists
----
---
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 #2176: [ZEPPELIN-2300] SHOULD NOT update paragraph text when ...
Posted by 1ambda <gi...@git.apache.org>.
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2176
Let me implement locking and realtime notification soon. I will close this PR.
---
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 #2176: [ZEPPELIN-2300] SHOULD NOT update paragraph text when ...
Posted by FRosner <gi...@git.apache.org>.
Github user FRosner commented on the issue:
https://github.com/apache/zeppelin/pull/2176
I get what you are saying and I agree. My only point was that you need to be aware that this change only does the following thing:
**Before**: Who persists the local changes earlier, wins the conflict. (User A wins in the example above)
**Now**: Who persists the local changes later, wins the conflict. (User B wins in the example above)
And I was wondering if this is a useful change or not. If this change is considered useful, the PR looks good to me :) If not, we should not merge it.
Again, thanks for your effort and time and I didn't mean to say that we cannot go step by step. I just wanted to challenge this PR.
---
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 #2176: [ZEPPELIN-2300] SHOULD NOT update paragraph text when ...
Posted by FRosner <gi...@git.apache.org>.
Github user FRosner commented on the issue:
https://github.com/apache/zeppelin/pull/2176
Hi @1ambda.
What are the implications of this change?
In the scenario you describe, it makes sense. So if person A is editing a cell and person B is running it at the same time, it will not overwrite the added content from person A, correct?
But will person A notice that person B is working in the same paragraph at the same time? Will he notice that someone ran the paragraph? I think this is the basic requirement because otherwise you will start typing at the same time and then eventually someone gets overwritten.
IMHO the only right way to make this whole websocket collaboration work is to have it as real-time as possible, transmitting each character stroke and showing others peoples cursors (like you have in Google Docs, e.g.).
BTW the gifs are very very tiny in my browser and I can't really tell what's going on there \U0001f604 Are they big for you?
---
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 #2176: [ZEPPELIN-2300] SHOULD NOT update paragraph text when ...
Posted by 1ambda <gi...@git.apache.org>.
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2176
@FRosner If auto-save works properly,
- User B's local change will update server state
- And User A's paragraph text will be updated because he/she doesn't have local change
- If they have local change, their paragraph will ignore newly propagated text and will overwrite User b's text later.
---
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 #2176: [ZEPPELIN-2300] SHOULD NOT update paragraph text when ...
Posted by 1ambda <gi...@git.apache.org>.
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2176
@AhyoungRyu Could you help review?
---
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 #2176: [ZEPPELIN-2300] SHOULD NOT update paragraph text when ...
Posted by 1ambda <gi...@git.apache.org>.
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2176
@FRosner Thanks for detailed reply.
We can't get the ultimate feature at once.
- locking
- realtime notification
- ...
IMO, This is just the first step. Then other people think about synchronization, and so on as we did. They might think auto save is not ultimate solution. If we want to resolve the whole things at once, we can't merge any of #2120 #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 pull request #2176: [ZEPPELIN-2300] SHOULD NOT update paragraph tex...
Posted by 1ambda <gi...@git.apache.org>.
Github user 1ambda closed the pull request at:
https://github.com/apache/zeppelin/pull/2176
---
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 #2176: [ZEPPELIN-2300] SHOULD NOT update paragraph text when ...
Posted by FRosner <gi...@git.apache.org>.
Github user FRosner commented on the issue:
https://github.com/apache/zeppelin/pull/2176
I am not sure if this change helps. We still have a race condition and conflict resolution is still "forceful overwrite and don't notify". If I see it correctly:
*Before*: Who persists the local changes earlier, wins the conflict. (User A wins in the [example](https://github.com/apache/zeppelin/pull/2176#issuecomment-290395313) above)
*Now*: Who persists the local changes later, wins the conflict.
If this is the expected behaviour and what you wanted to achieve, then LGTM :)
PS: If live editing like in Google docs can't be easily done, we could at least lock the paragraph for all but one user. A lock is granted when a paragraphs is focused. The lock gets released either when the focus is lost or someone else wants to acquire the lock. Changes get persisted before the lock gets released (i.e. either when losing focus or someone else also wants to edit). This way you cannot overwrite any remote changes as the paragraph will always get updated when you click into it and we don't have a race condition because of the lock.
---
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 #2176: [ZEPPELIN-2300] SHOULD NOT update paragraph text when ...
Posted by FRosner <gi...@git.apache.org>.
Github user FRosner commented on the issue:
https://github.com/apache/zeppelin/pull/2176
Understood @1ambda . Thanks for updating the gifs, now it is clear to me except one thing. When the second notebook had `user1` written and the first notebook executed the paragraph with `admin` written, `user1` was not replaced by admin at first. But then it was. **How do I get the remote changes / discard my local changes because otherwise as soon as I have local changes I cannot get the remote changes, can I?**
I also agree with your google docs reference. However a good first step would be an indicator that someone has the same notebook open. Next level is an indicator on a paragraph level. This solves most of the conflicts as I can usually just ask my colleague what he was planning or we are on a webex. After that it would of course be great to see where the cursor is but that's just the frosting of the cake :)
Thanks for taking care!
---
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 #2176: [ZEPPELIN-2300] SHOULD NOT update paragraph text when ...
Posted by 1ambda <gi...@git.apache.org>.
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2176
@FRosner I updated GIF.
- The basic motivation is, prevent overwriting my local changes when other browsers execute paragraphs.
> From the user perspective, this is a critical problem (uncontrolled loss of changes to the code) ([ZEPPELIN-1492](https://issues.apache.org/jira/browse/ZEPPELIN-1492))
- **IMHO the only right way to make this whole websocket collaboration work..**
I agree, but it requires so much modification. And this PR doesn't prevent going that way.
---
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 #2176: [ZEPPELIN-2300] SHOULD NOT update paragraph text when ...
Posted by FRosner <gi...@git.apache.org>.
Github user FRosner commented on the issue:
https://github.com/apache/zeppelin/pull/2176
Sorry I was traveling @1ambda :(
I understand what you are saying but I still don't understand exactly how it will work after this PR. Consider the following situation:
- User A and B edit a paragraph at the same time
- User A executes the paragraph
- User B will not get his changes overwritten (due to this PR, correct?)
- ...
Then how does User B ever get back in sync with the server state? How will the conflict be resolved? Will User A then lose his modifications?
---
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 #2176: [ZEPPELIN-2300] SHOULD NOT update paragraph text when ...
Posted by 1ambda <gi...@git.apache.org>.
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2176
@FRosner
**How do I get the remote changes / discard my local changes**
- Zeppelin will synchronize text automatically. That's the reason why it doesn't have button for that.
- But if Zeppelin keep local changes, we need some buttons or method for synchronizing text. (e.g update text for every 10 seconds if we don't have local change).
It will be another feature we need to develop soon. IMO, it beyond the scope of this PR :)
---
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.
---