Client-side Purging by SCdF · Pull Request #5075 · medic/cht-core

SCdF · 2018-12-06T16:48:47Z

For your consideration.

I spent a couple of hours today knocking out the basics of purge on the phone:

Runs every N days on refresh (in my hard-coded configuration I have it at 0 so it runs on every refresh)
Runs over all reports for all contacts and executes the configured function
Converts the result (guarding against bad ids) into minimally deleted files and writes them into PouchDB
Doesn't sync those deletes upwards
~~Initiates a compaction if anything was purged~~ Does nothing because we auto-compact

I have tested it and it seems to nominally work.

Still to do:

Write tests!
Actually pull config from config
Document that config's existence, write a TL / CHT guide on how to use the feature
Performance test to make sure running like this on startup doesn't destroy a typically full phone in an obviously bad way. If it does, consider pausing between sets for n seconds to reduce constant cpu usage
Consider exposing this on the about page so users can manually run purge.
Consider putting this behind a "persisted queue", so if a user gets halfway through and refreshes we pick up where we left off.
Consider view warming. Doesn't really seem worth it on PouchDB, idk. Which would you pick etc.

More notes:

If at some point we want purge rules to only apply to certain people we could allow for the function to return a map of id to role, e.g. ['id1', 'id2'] -> {id1: true, id2: ['role1', 'role2']}
If we decide that we don't want to ship without it running on the server, while it was good practice for thinking about the problem and not much time investment I'd probably have to throw the vast majority of it away.

Interested in your thoughts.

#5048

SCdF · 2018-12-06T17:02:15Z

Oh yeah, please don't merge 🙃

garethbowen · 2018-12-09T23:18:44Z

Looking good, nice work!

A few thoughts...

I think it's critical we never purge anything that's not yet replicated. I'm not sure what the best way to achieve this is. You could check the changes feed for the last updated date for this doc and compare that to the last sync time. Alternatively you could listen for dbsync to complete and then prompt Purger to check.
Seeing as you're doing this on refresh consider doing it in the bootstrap layer so the UI is blocked (nice spinner, "purging docs" message, etc). This would mean the whole process can run faster because angular wouldn't be constantly updating the UI as docs are purged.
We set auto compaction on in PouchDB but it would be worth considering an explicit compaction after deletion. This is a slow process but IIRC until compaction you don't see improvements in performance or disk space and I don't completely trust auto compaction to keep working.
If the doc is updated on the server it'll conflict on replication. I'm guessing that'll be silently ignored which is probably the right thing to do in this case, but we could consider bringing the doc back?

SCdF · 2018-12-10T10:09:05Z

@garethbowen,

This is pretty hard, and is made redundant by running it on the server, which is another great reason to migrate to that at some point.
This is bad because it blocks, but good because it makes the flow cleaner and more obvious (things don't randomly delete below you), and also removes any stability concerns we have about our angular code handling deletes. This is great because it also half-solves replicating super large users: we'll still download all their data but we will never boot angular or even pouchdb views (I can rewrite to not use views) against the large data set.
OK can look into that
The result this is non-deterministic, but in the style of eventual consistency even if it appears again the next purge will remove it, so I figured this was OK.

So:

I think there are more pros than cons to moving this code to run between PouchDB bootstrap and angular bootstrap, so I'll move it there
I'll change it to not use views at all, and just use all docs, which sounds bad but really 99% of a user's data is going to be a contact or a report anyway, so there is little wastage
I will look into fixing 1.

alxndrsn · 2018-12-07T17:04:08Z


+    if (!Session.isOnlineOnly()) {
+      Purger();
+    }


If this delays starting of the app UI, can we have a startup message for this stage?

It didn't (it spawns of a promise that we ignore), but in other discussions I think we've decided to move it to be an active part of pre-angular startup.

So yeah, it's going to be reworked into be an event emitter so we can follow progress etc

alxndrsn · 2018-12-10T19:25:20Z

+          keys.includes('_rev') &&
+          keys.includes('_deleted')) {
+        return false;
+      }


Would it be simpler just to have a _medic_purged: true flag instead of checking for these specific things? Is this doing something clever by deliberately copying pouch/couch soft-delete functionality?

Yeah I feel more comfortable with this, because it increases the specificity of what we're trying to avoid replicating. It means we can also (for now) do things like block writes of that in validate_doc_update for the server if we're so inclined

alxndrsn · 2018-12-10T19:25:59Z

+      return $q.all([
+        Search('reports', {}, options),
+        Search('contacts', {}, options)
+      ])


Assuming Search() is slow and that almost all docs are either contacts or reports, would it be quicker to do allDocs() here and then filter manually? Or just hand the filtering off to the custom function?

It's basically pointless to use search here AFAICT yeah, it's just copying what nools bootstrap does.

Since I'm moving the code to be before angular even starts I am definitely just going to do an all docs in the future

SCdF · 2018-12-12T12:28:11Z

@garethbowen / @alxndrsn I moved it to startup. I still need to hook up a progress bar etc so it looks nice.

I am also not protecting against documents being deleted that are not yet replicated.

It's still not clear to me a safe way of doing this, especially since we're now doing this before angular boots.

We could reverse engineer the _local document name, read that and only purge if the seq is the same as the db's current max seq. However, that's not an exposed API so we'd have to... copy it I guess? It doesn't seem safe.

I think if we really don't want to ship without this feature we are going to have to spend the effort to run the code on the server.

garethbowen · 2018-12-12T21:43:40Z

Looking good! Let's chat sometime about the server/client tradeoffs so we can move forward here.

alxndrsn · 2018-12-13T17:38:15Z

It's still not clear to me a safe way of doing this, especially since we're now doing this before angular boots.

Can you compare last-replication-time vs doc.reported_date?

SCdF · 2018-12-13T17:46:11Z

@alxndrsn unless I've missed it we don't store last-replication-time anywhere, it's only available as a live piece of data while angular is running.

We could change that so it writes it somewhere (eg to a local doc we do have access to), and then we use that on boot, presuming if that doesn't exist then we are also allowed to purge because it means no real replication has happened yet.

However, it's another write, which we're performing on every replication, to solve a check that happens once a month that probably shouldn't need to happen given good rules, and would not be reverted (since we don't need it for anything else) once we moved to server-side.

IDK, if people think it's worth it then that's cool and can be done, I personally just didn't have strong interest in it when I was knocking this together.

garethbowen · 2018-12-13T18:55:21Z

It's possible doc updates will still be purged before they're replicated because the code only checks the reported_date. Unfortunately we don't record when a doc was updated so it's not possible currently to check that.

One solution would be to move the code back into the angular app (sorry!) and trigger purging only once replication is completely caught up, but I think the faster execution in bootstrap still wins.

The other solution is running server side but we can do that in the next iteration.

This approach is good enough for now, do it!

SCdF · 2018-12-18T13:17:02Z

Demo: https://www.youtube.com/watch?v=laDvd0ja2ac

#5048

No messages / progress bar for it yet, that is coming next!

This is a halfway solution to making sure that documents are not purged before they are replicated. When running purge outside of angular it is very challenging to work this out, so instead we just make sure newer docs are not allowed to be purged.

SCdF · 2018-12-20T17:15:48Z

@garethbowen THE TESTS PASS.

I am happy for this to be merged at this stage.

I'm currently writing documentation in medic-docs, which will be a separate PR in the next few hours. Feel free to wait for that or not, depending on your schedule.

SCdF · 2018-12-20T17:58:55Z

NB: I got rid of blocking purges for documents younger than 60 days, because I managed to check for upward replication.

We could add this back though, if we want to be even more careful.

Doing it for the last 30 days might be a good idea actually:

Means (if people are using targets properly) that targets can't be broken
Stops overzealous configurers confusing users by purging stuff super fast

SCdF · 2018-12-20T19:29:38Z

Documentation is here: medic/medic-docs#63

garethbowen

Awesome work!

I've put some suggestions inline but no blockers so feel free to do them or not. I've approved this PR so you can go an merge without having to come back for another round.

garethbowen · 2018-12-20T21:08:27Z

+    PURGE_INFO: function(progress) {
+      const n = progress.purged;
+      const percent = Math.floor((progress.processed / progress.total) * 100);
+      return `Cleaned ${n} documents (${percent}% complete)…`;


To make translation easier, perhaps do the calculation first, and call the translator with count and percent. Otherwise each language will have to do this calculation...

garethbowen · 2018-12-20T21:09:27Z

+      const percent = Math.floor((progress.processed / progress.total) * 100);
+      return `Cleaned ${n} documents (${percent}% complete)…`;
+    },
+    PURGE_AFTER: 'Optimising…',


Obviously these two keys need to be translated into all the languages we support. It might be difficult to get this done on short notice so feel free to raise an issue blocking the 3.4.0 release to get the translations for each language.

garethbowen

On second thoughts, I think we may need to handle the failure case more gracefully... comment inline.

SCdF · 2019-01-08T12:34:13Z

@garethbowen OK check out 3f7a58c

We no longer ever hard fail for anything.

Instead, if the purge function cannot be compiled, or a purge set fails for some reason, we generate feedback errors (up to 10 to avoid spam), which are then generated into real feedback documents on successful Angular boot.

Additionally, in testing I decided that it makes sense that purge should re-run if you change the purge function, so I added that as well. It's going to be an interesting decision when moving from client to server, but that's a conversation for another time.

garethbowen

Awesome!

SCdF requested review from alxndrsn and garethbowen December 6, 2018 16:48

alxndrsn reviewed Dec 10, 2018

View reviewed changes

SCdF force-pushed the 5048_purge_client_side branch from 6be639b to 39a5175 Compare December 11, 2018 12:20

SCdF force-pushed the 5048_purge_client_side branch from 12965cf to 1713e17 Compare December 18, 2018 09:57

SCdF and others added 16 commits December 20, 2018 16:03

Purging draft

fa08c47

#5048

Actually we have auto-compaction turned on

92797b4

Removing to appease the Travis G-ds

d16a1a5

Missed this due to bad git add -p

cd21fce

Moved to bootstrapper phase

04135b3

Formatting

bbcb13f

First cut of rewriting into a pre-angular event emitter

29fe8bc

Purging now happens at startup

b1f56bf

No messages / progress bar for it yet, that is coming next!

Better initial purging

56e7b6a

Adding event handlers so the UI displays purge progress

37639f0

First I'd like to thank my Mum, JSHint...

b0da36c

Reverting bad merge, purge config fix

5d23cb4

Fixing tests

7f71eed

Removing .only

39601e9

Added post-run compaction.

ab53751

Stefan du Fresne added 7 commits December 20, 2018 16:03

ahem

6f66050

Hark! An e2e test!

ab6abf8

Always forgetting

dbb0843

oy with the poodles already

fdd46c1

Throwing stuff to the wall to see what sticks

737df94

Whoops

0be9e37

OK now we're ready to roll

de9e4aa

SCdF force-pushed the 5048_purge_client_side branch from e7f42e6 to de9e4aa Compare December 20, 2018 16:56

SCdF changed the title ~~Client-side Purging [DRAFT]~~ Client-side Purging Dec 20, 2018

garethbowen approved these changes Dec 20, 2018

View reviewed changes

garethbowen reviewed Dec 20, 2018

View reviewed changes

Comment thread webapp/src/js/bootstrapper/index.js

garethbowen suggested changes Dec 20, 2018

View reviewed changes

Stefan du Fresne added 2 commits December 21, 2018 08:58

PR feedback, plus adding checks around purge function calls

4c2aef5

Removing so we can put it in its own PR

cb25dc1

SCdF mentioned this pull request Dec 21, 2018

Support binding to fields.patient_uuid #5132

Merged

Generate feedback documents, better new purge detection

3f7a58c

SCdF requested a review from garethbowen January 8, 2019 12:29

JSHint fixes

061a6a5

garethbowen approved these changes Jan 8, 2019

View reviewed changes

garethbowen merged commit 4a6972e into master Jan 8, 2019

garethbowen deleted the 5048_purge_client_side branch January 8, 2019 22:16

Uh oh!

Uh oh!

Conversation

SCdF commented Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SCdF commented Dec 6, 2018

Uh oh!

garethbowen commented Dec 9, 2018

Uh oh!

SCdF commented Dec 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SCdF commented Dec 12, 2018

Uh oh!

garethbowen commented Dec 12, 2018

Uh oh!

alxndrsn commented Dec 13, 2018

Uh oh!

SCdF commented Dec 13, 2018

Uh oh!

garethbowen commented Dec 13, 2018

Uh oh!

SCdF commented Dec 18, 2018

Uh oh!

SCdF commented Dec 20, 2018

Uh oh!

SCdF commented Dec 20, 2018

Uh oh!

SCdF commented Dec 20, 2018

Uh oh!

garethbowen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

garethbowen left a comment

Choose a reason for hiding this comment

Uh oh!

SCdF commented Jan 8, 2019

Uh oh!

garethbowen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SCdF commented Dec 6, 2018 •

edited

Loading