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

New revisions should be marked as "draft", and should automatically prune drafts #2451

Closed
wlach opened this issue Nov 18, 2019 · 5 comments
Closed
Assignees

Comments

@wlach
Copy link
Contributor

wlach commented Nov 18, 2019

Currently we save new revisions of each iodide notebook every 30 seconds:

const SERVER_AUTOSAVE_TIMEOUT = 30000;

This leads to a lot of redundant revisions in notebooks with even a modest amount of history, which can make the history viewer hard to use. We should probably put a cap on the number of revisions we save to one per minute -- this would also let us save even more frequently.

I propose doing this by adding "draft" revisions. Draft revisions are subject to being deleted/replaced until they are at least a minute old.

Proposal:

  • Add a new boolean field "is_draft" to the notebook revision model. Need to change something around here (you'll also need to create django migrations here, try googling "django migrations"):
    class NotebookRevision(models.Model):
  • New revisions should have "is_draft=True" by default. Probably need to change something around here: https://github.com/iodide-project/iodide/blob/master/server/notebooks/api_views.py#L128
  • Every time a revision is submitted, schedule a periodic task that does the following:
    ** Get a list of all "draft" revisions
    ** Make sure that all draft revisions are spaced at least one minute apart. If there any draft revisions within the same minute interval, delete them.
    ** After this process is complete, mark any draft revisions more than a minute old as being non-draft.
  • Write some unit tests to make sure this is working as expected. You can see some existing ones under server/tests/: https://github.com/iodide-project/iodide/tree/master/server/tests

This is not a comprehensive set of things that need to be done but should be enough to get started. :)

@wlach
Copy link
Contributor Author

wlach commented Nov 18, 2019

@zzl0 is interested in taking this on

@zzl0
Copy link
Contributor

zzl0 commented Nov 18, 2019

@wlach thanks for creating this issue

@zzl0
Copy link
Contributor

zzl0 commented Nov 19, 2019

Hi William,

Good morning, after thinking more about this issue, I have some questions about current proposal:

  1. What do you mean by "redundant"? I assume there are some [small] difference between every two revisions.
  2. Should we slove this issue by grouping revisions on UI? I mean UI only show 1 revision every X minutes by default and users can also expand that one to see all small revisions within that X minutes. The benefits of doing this is that users don't see a lot of revision by default, and also they have the choice to see all the revisions.
  3. How to choose X? Why 1 minute is a good value? To me, 1 minute does not make much difference (compared to 30s).
  4. What do you mean by "schedule a periodic task"? use another thread/process to run that task?

@wlach
Copy link
Contributor Author

wlach commented Nov 19, 2019

Good questions!

1. What do you mean by "redundant"? I assume there are some [small] difference between every two revisions.

Maybe "redundant" is not the best term -- perhaps "excessive" would be better. You are correct that there should be a small difference between every two revisions, but the amount of changes a user can make within one minute is not too great. For the purpose of keeping a paper trail, I think keeping a persistent log of changes at one minute intervals should be sufficient.

We can always tweak the exact values depending on user feedback.

2. Should we slove this issue by grouping revisions on UI? I mean UI only show 1 revision every X minutes by default and users can also expand that one to see all small revisions within that X minutes. The benefits of doing this is that users don't see a lot of revision by default, and also they have the choice to see all the revisions.

I think this might be an interesting thing to do (it's what e.g. Google Docs does), though I think this will solve some of the same problems and is worth working on, on its own.

3. How to choose X? Why 1 minute is a good value? To me, 1 minute does not make much difference (compared to 30s).

1 minute should cut down the number of revisions down by half (or more, if we start saving more frequently which I think would be a logical followup)

4. What do you mean by "schedule a periodic task"? use another thread/process to run that task?

Ah yes, I should have described that in more detail. In iodide we use spinach (a python library like celery, if you've used that) to schedule tasks outside of user interaction. You can see an example of this here:

https://github.com/iodide-project/iodide/blob/master/server/files/tasks.py

@zzl0
Copy link
Contributor

zzl0 commented Nov 19, 2019

Thank you @wlach for the detailed explanation! Sounds good to me, I will continue working on this.

zzl0 added a commit to zzl0/iodide that referenced this issue Nov 19, 2019
zzl0 added a commit to zzl0/iodide that referenced this issue Nov 20, 2019
zzl0 added a commit to zzl0/iodide that referenced this issue Nov 20, 2019
zzl0 added a commit to zzl0/iodide that referenced this issue Nov 20, 2019
zzl0 added a commit to zzl0/iodide that referenced this issue Dec 1, 2019
zzl0 added a commit to zzl0/iodide that referenced this issue Dec 1, 2019
zzl0 added a commit to zzl0/iodide that referenced this issue Dec 3, 2019
wlach pushed a commit to zzl0/iodide that referenced this issue Dec 4, 2019
@wlach wlach closed this as completed in adf476b Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants