Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial design proposal of simultaneous flow editing by multiple clients #66

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

HiroyasuNishiyama
Copy link
Member

This PR adds initial design note on simultaneous flow editing by multiple clients for discussion of implementation direction.

Copy link
Member

@knolleary knolleary left a comment

Choose a reason for hiding this comment

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

Given this is a very early stage discussion that could cover many different things, it may have been easier to start with a discussion to help identify the scope and requirements which can then be turned into a more solid proposal of work.

As it stands, this PR covers two very different approaches - both of which are valid approaches. We have certainly had feedback/requests for both of these styles in the past.

It is also true to say that it does not have to be we only do one or the other.

The whole question of improving the simultaneous flow editing can cover a wide range of ideas - from small improvements to what we have, to the full 'live' editing experience you suggest in method 1.

The types of small improvements that could be made to the existing behaviour are around resolving conflicts and merging changes (when safe to do so) in the background without interrupting the user. That is something the 'live' editing experience would also have to do at a much finer level.

Even just having awareness that someone is 'on' a given tab would be a small improvement - even if their changes don't show up in realtime.

Being able to lock tabs to reduce the risk of merge conflicts would be an incremental improvement on top of that. And also fits with other potential requirements about finer-grained access control to flows.

The live editing is the extreme model, and in many aspects, the most intuitive to users - but has the Deploy button problem I describe in my other comments that I can't immediately see a solution to.

designs/simultaneous-edit/README.md Outdated Show resolved Hide resolved

#### 1-c: Implementation

- Extends the edit history information currently used for *undo* / *redo* of flow editing operations, and sends editing action information to other clients via the runtime.
Copy link
Member

Choose a reason for hiding this comment

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

I think I have a branch of code locally that started looking at how to generate JSON Patch fragments from the history component. That seemed to be the most sensible way to approach this - as it should be possible to apply patches in a well defined order (and also has some well-known algorithms for resolving out-of-order merges).

Copy link
Member Author

Choose a reason for hiding this comment

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

My assumption was to serialize the editing actions by communicating with runtime APIs.
Your suggestion using JSON patch may be useful if we deal with it as client-side merge process. However, I feel it will be difficult to guarantee that multiple clients will get the same editing results.

Copy link
Member

Choose a reason for hiding this comment

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

JSON Patch is one possible format - but the key is the history component would be the central place to know that a change has happened, and that change should be communicated to other editors. When the other editors receive the details of that event, they need to apply them to the local configuration - translating the event from whatever format it is in back into whatever modification to internal state in the editor is needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. My assumption is that the runtime manages the edit histories to keep the order consistent for all clients.

Copy link
Member

Choose a reason for hiding this comment

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

Shifting the edit history to the runtime is a major change to how the editor works. We'd need to see a lot more detail on how you think that would work. Resolving concurrent edit state between multiple parties is a very complex task - there are countless research papers on the topic. This is the part that I'm most interested in - we cannot underestimate the difficulty in getting it right.

For example, how do you resolve conflicting changes to the same resource. How do you manage ordering of changes. How do you manage undoing a change.

But this still remains an academic conversation untill we under stand how we handle the deploy button issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your comment. I have updated the design document.

Regarding the issue of the deploy button, I would like to suggest a method that all users to save the fix and then deploy it.
Also, for edit action conflicts, I would like to suggest serialized application of operations based on atomic application of each edit actions.


- We may need to synchronize client state at some point by sending refresh request to all client.

- Whether to use sync mode can be specified in `settings.js`.
Copy link
Member

Choose a reason for hiding this comment

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

Yes - this would have to be an opt-in feature.

I also wonder whether this should be provided by a plugin rather than part of the core. It would require some additional APIs to be exposed in core for sure, but it may be a cleaner approach - particular for those who embed Node-RED and don't want this type of functionality at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Various event extensions will be required in Node-RED side, but I think it can be implemented as a plugin.


#### 2-a: Requirements

Lock the flow edit target of client A in order to suppress or notify the edit by another client B.
Copy link
Member

Choose a reason for hiding this comment

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

Being able to lock tabs comes up in other contexts. For example, restricting who can edit certain flows based on user name/role. So I think there is value in exploring this approach as I think it will help other use cases.

There are plenty of difficulties around managing the locked state - and how you handle the case when someone wanders off for a long weekend, forgetting they have left a tab locked. And you do describe some possible ways to manage that.

Copy link
Member Author

Choose a reason for hiding this comment

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

As you wrote, the ability to limit flow editing may be useful in team development.
I think long-term locking issues can be resolved by arrangements within the team in most cases. Ancient revision control systems such as CVS used such means.
However, considering the occurrence of locking problem, it seems that forced unlock feature may be necessary.


#### 1-a: Requirements

- The flow edit result of a client is immediately reflected to the flow edited on workspaces of other clients.
Copy link
Member

Choose a reason for hiding this comment

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

It is easy to compare this to the user experience on Google Docs. However this is one very important difference.

In Google Docs, you are all editing the document on the server. No one has to hit save - the changes are 'live'.

In Node-RED, nothing is saved in the runtime until someone presses the Deploy button.

That difference is very important as it raises a number of questions over what happens when a user hits Deploy.

They will end up deploying their work and whatever the other user was in the middle of doing - with no guarantee the other user was ready to deploy their work. They could completely break things. This is one of the reasons I haven't tackled this yet - despite a few early experiments. And it is why the current concurrent editing model is based around when a user Deploys their work.

Copy link
Member Author

Choose a reason for hiding this comment

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

As I wrote above, within a small team, I think it's appropriate to deal with this by an arrangement between members.
Our users have similar issues with team development on current Node-RED and they are covering the issue by operations.
However, there may be some support features that need to be improved, such as a feature for helping participating users agree on deploy.

Copy link
Member

Choose a reason for hiding this comment

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

My worry is the feature makes the collaboration issues harder - because there is nothing built-in to help co-ordinate deploys. User's could get into a real mess if this feature allows User A to deploy User B's half-finished work. Even if it relies on co-ordination outside of Node-RED, accidents still happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand such concerns. I think that one guard is that this feature is opt-in feature. So, we can expect team members understand the behavior using it.
As for the support function for this, for example, notification to the user who is editing and authorization from them may be useful.

Copy link
Member

Choose a reason for hiding this comment

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

I think the design needs to carefully describe what the user workflows are so we can see what type of interaction is possible and what issues come up.

@SandeepAgarwal13
Copy link

Hi ,
This is a feature which interests me as a user. Hence sharing my thoughts.
From an end user view - who knows very limited internals/backend of NR - locking option is something i would vote for (Option 2)

Already apologizing in case i have caught wrong end of some NR internals.

  1. If locking feature enabled (via. settings.js) Every client needs to lock a flow else its read only. Only a user with write permissions (based on today) can lock. (The design talks about client - not clear if it means user login is mandatory - thereby being able to capture user info for locked state - i am assuming so. )

  2. Only the user who locked a flow or admin user can unlock it back.
    I am aware that today user role permissions are limited to read/write ALL - so till the absence of more granularity- any user with write permission can unlock any flow. (Working in a team - people do know when someone goes on vacation). Once unlocked - its unlocked. And any user (including the one who locked it originally) needs to relock.

  3. Its mentioned in above scenario when user wanders off.... defining time and then unlocking........ and then again comparing and giving the lock back. when user comes back. Maybe not a day 1 feature . Once unlocked - user needs to relock since the unlocking was intentional by his peer team member.

(It does mean - if the original user did some changes on his client which are not yet "deployed" - they would be lost . But being a team collaboration - i would assume only when the user goes on long breaks - other team members would want to unlock post discussing with him). Hence i mean not a day 1 feature.

  1. I am not clear on "The problem with method 2 is that conflicts may occur when locks are not acquired and a flow is edited by multiple clients. So, there still exist the same issues with current Node-RED implementation when conflicts occur.".

If working in lock feature enabled mode - i would assume a lock is mandatory i.e. for a user to be able to edit.
Does it refer to error scenario when trying to acquire a lock ?The only reason i would presume not to get a lock would be when either that flow is locked by others or for some reason NR went down and user still has editor open on his browser.
As i see if not locked - then no edit.

  1. On notifying other users when a change happens - This may not be a day 1 improvement. But down the line. For Day 1 the current mechanism of Merge / Review continues when some flows change.
    Down the line -
    The enhancements can be - indicating to other users if a flow has been updated - some symbol on flow & allowing an option to refresh that flow/all flows.
    Or refresh happening automatically when the user opens a flow in his editor instance which was updated by someone else & deployed already.
    Or refresh happening only on locking (it means user sees a "stale" version of the flow till he locks or refreshes - not ideal way as user keeps seeing incorrect information on screen)

My above write up is based on the way we work across other systems - which allows us to track which user did what - there by maintaining some level of concurrency without overwriting & backtracking any potential issues.

In case above is completely flawed - please feel free to correct me.
Thanks.

@HiroyasuNishiyama
Copy link
Member Author

Hi, Thank you for your comments.

  1. If locking feature enabled (via. settings.js) Every client needs to lock a flow else its read only. Only a user with write permissions (based on today) can lock. (The design talks about client - not clear if it means user login is mandatory - thereby being able to capture user info for locked state - i am assuming so. )

The original design assumed that the flow was always editable. However, I think it is useful to be able to make the flow read only as you suggested, so I reflected it in the design.

  1. Only the user who locked a flow or admin user can unlock it back.

The problem here is that Node-RED has no way of distinguishing users. But I have added your suggestion to discussion part.

@knolleary
Copy link
Member

The problem here is that Node-RED has no way of distinguishing users.

It does if you have adminAuth enabled. I have assumed this would all be based on a system with adminAuth enabled so that you know who is editing the flows.

Are you thinking differently?

@HiroyasuNishiyama
Copy link
Member Author

Are you thinking differently?

Here, I was assuming the default setting (adminAuth disabled) because many of our users are on the situation.
However, I think it will be possible to make that a prerequisite.

@SandeepAgarwal13
Copy link

The problem here is that Node-RED has no way of distinguishing users. But I have added your suggestion to discussion part.

Here i did mean "adminAuth" enabled. As without that tracking who did what is impossible (well almost).

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.

3 participants