Emit completion ops for tombstones #53

linabutler · 2019-09-10T07:51:56Z

This commit adds completion ops for deleting local items (to apply
remote tombstones), inserting new tombstones (to delete non-syncable
and invalid items), and uploading tombstones (to avoid an extra table
scan when staging outgoing tombstones).

These ops also help avoid extra work when applying tombstones for items
that don't exist locally, or uploading tombstones for items that don't
exist remotely.

It's unclear if `flag_{for_upload, as_merged}` and `skip_upload` refer to local or remote items. This commit renames them to be more consistent: * `SetLocalUnmerged` (formerly `FlagForUpload`) marks a local item as needing to be merged. * `SetLocalMerged` (formerly `SkipUpload`) removes the "needs merge" flag from a local item. * `SetRemoteMerged` (formerly `FlagAsMerged`) removes the "needs merge" flag from a _remote_ item. This commit also renames `Upload` to `UploadItems`, since we'll be adding completion ops for uploading tombstones next.

This commit adds completion ops for deleting local items (to apply remote tombstones), inserting new tombstones (to delete non-syncable and invalid items), and uploading tombstones (to avoid an extra table scan when staging outgoing tombstones). These ops also help avoid extra work when applying tombstones for items that don't exist locally, or uploading tombstones for items that don't exist remotely.

linabutler · 2019-09-11T05:26:53Z

src/merge.rs

+                (Record::Unmentioned, Record::Deleted) => {
+                    // The item isn't mentioned locally, but is deleted
+                    // remotely. This is the case for a first sync, and will
+                    // change if we store tombstones (bug 1343103). For now,


This does mean we'll need to merge all tombstones again if we ever decide to store them, but we can cross that bridge later. Marking remote tombstones as merged also means we can filter them out in subsequent merges, which is going to make application more efficient.

linabutler · 2019-09-11T05:28:18Z

src/merge.rs

+                    // The item isn't mentioned locally, but exists remotely.
+                    // This is the case for invalid remote-only items. Upload a
+                    // tombstone, but don't flag it as remotely merged; we'll do
+                    // that when we store the uploaded tombstone.


This is assuming we write records back to the mirror, like Desktop does now—but pretty much all completion ops assume a database like Desktop Places, so that's fine.

linabutler · 2019-09-11T05:30:38Z

src/merge.rs

+                }
+                (Record::Deleted, Record::Unmentioned) => {
+                    // Uploading a tombstone for a nonexistent item. Nothing
+                    // to do here.


We might as well upload it. We don't have a "remove tombstone" op, and not uploading here means there's a difference between what we upload, and what's in moz_bookmarks_deleted. Ditto for (Record::Deleted, Record::Deleted) below.

src/tree.rs

linabutler · 2019-09-11T05:35:09Z

src/tree.rs

 /// A node in a bookmark tree that knows its parent and children, and
 /// dereferences to its item.
 #[derive(Clone, Copy, Debug)]
 pub struct Node<'t>(&'t Tree, &'t TreeEntry);

 impl<'t> Node<'t> {
+    /// Returns the item for this node.
+    #[inline]
+    pub fn item(&self) -> &'t Item {


Makes lifetimes easier—Deref returns a reference tied to the node's lifetime (&self), but tying it to the entry's lifetime (&'t) means we can store references to the item and its fields.

We now emit ops in two steps. The first step removes all local tombstones, and flags remote tombstones as merged, for all revived items. The second handles the actual deletion, on either or both sides. * Don't add deletions from local and remote trees that aren't mentioned in `delete_remotely` and `delete_locally`, because we emit different completion ops for those. * Change `Tree::guids` to only return GUIDs that exist in the tree, not deleted GUIDs. * Change `Tree::deletions` to return a `HashSet` instead of an iterator, so that we can use the `difference` method. * Add `Tree::exists`. * Remove `Tree::record_for_guid`. * Add `CompletionOps::summarize` for tests and logging. * Remove `StructureCounts::merged_deletions`, since it's not accurate anymore.

This commit adds `MergedRoot::completion_ops_with_signal`, and implements `MergedRoot::completion_ops` in terms of it.

codecov-io · 2019-09-14T05:20:08Z

Codecov Report

Merging #53 into master will decrease coverage by 0.15%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
- Coverage   75.14%   74.98%   -0.16%     
==========================================
  Files           7        7              
  Lines        2655     2726      +71     
==========================================
+ Hits         1995     2044      +49     
- Misses        660      682      +22

Impacted Files	Coverage Δ
src/driver.rs	`73.07% <ø> (ø)`	⬆️
src/store.rs	`0% <0%> (ø)`	⬆️
src/tree.rs	`64.46% <53.84%> (-0.35%)`	⬇️
src/merge.rs	`72.25% <65.26%> (+0.31%)`	⬆️
src/tests.rs	`97.33% <97.05%> (-0.74%)`	⬇️
src/guid.rs	`68.08% <0%> (-2.13%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c94b19...b6a900c. Read the comment docs.

linabutler · 2019-09-15T18:15:11Z

src/driver.rs

@@ -68,6 +68,7 @@ pub enum TelemetryEvent {
 pub struct TreeStats {
    pub time: Duration,
    pub items: usize,
+    pub deletions: usize,


merged_deletions is gone, but we can still report the number of local and remote deletions in shutdown hangs.

linabutler · 2019-09-15T18:16:17Z

src/merge.rs

@@ -50,22 +50,12 @@ pub struct StructureCounts {
    /// Total number of nodes in the merged tree, excluding the
    /// root.
    pub merged_nodes: usize,
-    /// Total number of deletions to apply, local and remote.
-    pub merged_deletions: usize,


Removed because it's not accurate anymore—tombstones on one side for items that don't exist on the other won't be merged anymore. We'll handle them specially when we build the list of completion ops.

linabutler · 2019-09-15T18:29:22Z

src/merge.rs

-                ops.flag_as_merged.push(flag_as_merged);
+        accumulate(signal, &mut ops, self.node(), 1, false)?;
+
+        // Clean up tombstones for local and remote items that are revived on


These works like this:

Any local tombstones that aren't in delete_remotely are removed. They're for revived (deleted locally, but changed remotely) items.

Any tombstones on both sides (if you delete the same bookmark on two devices while they're both offline, then sync) are removed and flagged as remotely merged.

Any remote tombstones for items that aren't in delete_locally, and that don't exist locally, are flagged as remotely merged. The extra check avoids a redundant UPDATE items SET needsMerge = 1 for the remote tombstone. Since we'll be reuploading the local item, anyway, it doesn't make sense to update its needsMerge flag, only to replace the whole row later when we write the uploaded item back into the mirror.

Any accepted deletions for items that exist on both sides must be because the items are invalid or non-syncable (left pane roots and queries). We delete the item locally, insert a tombstone into moz_bookmarks_deleted in case upload is interrupted, and flag the tombstone for upload.

Any deletions for items that only exist locally must be either incoming tombstones ({ id: "xyz", deleted: true }), or local non-syncable items. Delete them, and also flag the remote tombstone as merged if one exists.

Any deletions for items that only exist remotely must be either tombstones in moz_bookmarks_deleted that we're uploading, or deletions of invalid or non-syncable remote items (left pane on the server, bookmarks with invalid URLs, and such). Insert a tombstone into moz_bookmarks_deleted if one doesn't already exist, and flag it for upload.

src/merge.rs

linabutler · 2019-09-15T18:37:19Z

src/merge.rs

+    }
+
+    /// Returns a printable summary of all completion ops to apply.
+    pub fn summarize(&self) -> Vec<String> {


This is to make testing easier, but I can also imagine adding these to trace logs.

src/merge.rs

Instead of panicking if a GUID in `delete_locally` or `delete_remotely` doesn't exist on both sides, we now clean up local tombstones, and flag the remote tombstone as merged. With this change, we no longer need to filter out local and remote tombstones for items that don't exist on the other side. That means we can revert `Tree::guids` to return all GUIDs, including deletions, and have `MergedRoot::{local, remote}_deletions` return a more accurate set of tombstones for logging.

linabutler added 2 commits September 10, 2019 00:30

linabutler requested a review from mhammond September 10, 2019 07:51

linabutler commented Sep 11, 2019

View reviewed changes

linabutler added 3 commits September 13, 2019 16:11

Report local and remote deletions in tree stats.

af00f14

Make fetching completion ops abortable.

5707548

This commit adds `MergedRoot::completion_ops_with_signal`, and implements `MergedRoot::completion_ops` in terms of it.

linabutler commented Sep 15, 2019

View reviewed changes

linabutler added 4 commits September 16, 2019 09:59

Improve completion op summaries.

51e638e

Add a to_strings helper.

558ee66

Remove unnecessary closure for Guid::as_str.

b6a900c

linabutler merged commit b6a900c into master Sep 16, 2019

linabutler deleted the smarter-deletions branch September 17, 2019 22:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit completion ops for tombstones #53

Emit completion ops for tombstones #53

linabutler commented Sep 10, 2019

linabutler Sep 11, 2019

linabutler Sep 11, 2019

linabutler Sep 11, 2019

linabutler Sep 11, 2019

codecov-io commented Sep 14, 2019 •

edited

linabutler Sep 15, 2019

linabutler Sep 15, 2019

linabutler Sep 15, 2019

linabutler Sep 15, 2019

Emit completion ops for tombstones #53

Emit completion ops for tombstones #53

Conversation

linabutler commented Sep 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 14, 2019 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 14, 2019 •

edited