872758 - Multi delete documents

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Gregory Szorc [:gps]]

Bagheera is growing the ability to delete multiple documents at the same time in bug 865894. The desktop client should leverage the new APIs.

Comment 1

[Gregory Szorc [:gps]]

According bug 865894 and some IRC chat, the server will soon accept a comma-delimited list of documents in the X-Obsolete-... request header. We should be able to move forward on preliminary implementation.

I'd like this uplifted as soon as the server supports multi delete, as it will help with the orphan issue.

Comment 2

[Gregory Szorc [:gps]]

Rough steps to completion:

1) Implement new server feature in our mock bagheera server (services/common/modules-testing/bagheeraserver.js)
2) Support multi delete in bagheera client (services/common/bagheeraclient.js)
3) Hook up multi delete to FHR client (services/healthreport/healthreporter.jsm)

#3 can't land (at least enabled - we possibly land behind a pref or build flag or something) until the production bagheera server supports the new functionality.

Comment 3

[Stefan Mirea [:smirea]]

Attachment: Enhancing Bagheera to support deleting multiple documents;

- Implemented multi-delete of documents in bagheeraserver.js via a comma separated X-Obsolete
 header
- Updated bagheeraclient.js to take advantage of multi delete
- Updaed healthreporter.js to take advantage of multi delete. As a side effect all the occurences of the (string) "deleteID" option as part of the BagheeraClient::uploadJSON() method were replaced with the (array) "deleteIDs" which is now an array of ids. As a result, all the methods that used this as an attribute have been renamed (where necessary) and altered to work with arrays and backwards compatible methods were provided in some cases
- The "test_state_multiple_remote_ids" test as part of healthreport/tests/xpcshell-tests/test
_healthreporter.js was modified to support the new behaviour which entails the deletion of al
l old remoteIDs when an upload occurs
- As the server currently supports multiple deletion of documents only on upload via the X-Ob
solete header, other methods such as BagheeraClient::deleteDocument() were not modified

Comment 4

[Mark Reid [:mreid]]

Not sure if it's relevant, but just in case you're looking to keep feature parity with the production Bagheera server, it also supports multi-delete by sending multiple X-Obsolete-Doc headers (in addition to a comma-separated list in a single header).

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 764875 [details] [diff] [review]
Enhancing Bagheera to support deleting multiple documents;

Review of attachment 764875 [details] [diff] [review]:
-----------------------------------------------------------------

This is 90% to an r+.

::: services/common/bagheeraclient.js
@@ +120,1 @@
>     *            form.

Since we have an API break, it wouldn't hurt to throw if deleteID is specified in options, just to be on the safe side. The last thing we want is some random client out there silently failing to delete old IDs.

@@ +189,5 @@
>  
>      let result = new BagheeraClientRequestResult();
>      result.namespace = namespace;
>      result.id = id;
> +    result.deleteIDs = deleteIDs;

I'd feel much better if you created a new array object. There's no telling if the caller will mutate the original array and thus mutate result.deleteIDs.

result.deleteIDs = deleteIDs.slice(0);

::: services/common/modules-testing/bagheeraserver.js
@@ +274,5 @@
>      if (request.hasHeader("X-Obsolete-Document")) {
>        let obsolete = request.getHeader("X-Obsolete-Document");
>        this._log.info("Deleting from X-Obsolete-Document header: " + obsolete);
> +      let obsolete_list = obsolete.split(",");
> +      for (let obsolete_id of obsolete_list) {

for (let id of obsolute.split(",")) {

::: services/common/tests/unit/test_bagheera_client.js
@@ +89,5 @@
> +    server.setDocument(ns, id, payload);
> +    do_check_true(server.hasDocument(ns, id));
> +  }
> +
> +  Task.spawn(function test_post_delete_multiple_obsolete_documents () {

Instead of using a Task, you can change this test to add_task() and you can put the body inside a try..finally block.

@@ +90,5 @@
> +    do_check_true(server.hasDocument(ns, id));
> +  }
> +
> +  Task.spawn(function test_post_delete_multiple_obsolete_documents () {
> +    /* test uploading with deleting some documents */

Nit: // Test uploading ... documents.

Only use /* for multiline comments. All comments start with capital letter and end with punctuation.

@@ +99,5 @@
> +    do_check_true(server.hasDocument(namespace, "new-1"));
> +    for (let id of deleteIDs) {
> +      do_check_false(server.hasDocument(namespace, id));
> +    }
> +    // check if the documents that were not staged for deletion are still there

Nit: Comment punctuation.

@@ +100,5 @@
> +    for (let id of deleteIDs) {
> +      do_check_false(server.hasDocument(namespace, id));
> +    }
> +    // check if the documents that were not staged for deletion are still there
> +    for (let [,id,] of documents) {

Oh, I didn't realize you could use empty commas in destructured assignment like that \o/

@@ +101,5 @@
> +      do_check_false(server.hasDocument(namespace, id));
> +    }
> +    // check if the documents that were not staged for deletion are still there
> +    for (let [,id,] of documents) {
> +      do_check_true(deleteIDs.indexOf(id) !== -1 || server.hasDocument(namespace, id));

Can you rewrite this as .indexOf(id) == -1 && server.hasDocument

I think that's slightly easier to read.

@@ +107,2 @@
>  
> +    /* test upload without deleting documents */

Nit: Don't use /* */ here.

::: services/healthreport/healthreporter.jsm
@@ +174,5 @@
>    },
>  
>    removeRemoteID: function (id) {
> +    let idArray = id ? [id] : [];
> +    return this.removeRemoteIDs(idArray);

You could just inline the variable.

@@ +204,5 @@
>    updateLastPingAndRemoveRemoteID: function (date, id) {
> +    let idArray = id ? [id] : [];
> +    return this.updateLastPingAndRemoveRemoteIDs(date, idArray);
> +  },
> +  

Nit: Trailing whitespace.

@@ +1406,2 @@
>          let options = {
> +          deleteIDs: obsolete_ids,

Nit: obsoleteIDs (can this line be inlined?)

::: services/healthreport/tests/xpcshell/test_healthreporter.js
@@ +1020,2 @@
>    let now = new Date(Date.now() - 5000);
> +  

Nit: trailing whitespace

Comment 6

[Gregory Szorc [:gps]]

Adding tracking flags. We'll want this uplifted as soon as the server supports multi-delete.

Comment 7

[Stefan Mirea [:smirea]]

Attachment: Enhancing Bagheera to support deleting multiple documents;

updated to include :gps 's comments

Comment 8

[Gregory Szorc [:gps]]

According to bug 865894, server support has been deployed to production. So, we're probably good to land this in Nightly. Although, I will verify with a local build against production before landing.

Comment 9

[Gregory Szorc [:gps]]

Logs from a local run seem to be good:

371839377722	Services.BagheeraClient	INFO	Uploading data to https://fhr.data.mozilla.com/1.0/submit/metrics/4a735601-5604-9545-9f14-e00c2f49b8b0
1371839377723	Services.BagheeraClient	DEBUG	Will delete 3100640e-34c6-9147-b402-2d64672ac5be, 3f9cb3a1-daa0-11e2-811b-c82a144a924f, 3f9cb3a1-daa1-11e2-811b-c82a144a924f
1371839377723	Services.BagheeraClient	INFO	Request body length: 932
1371839377724	Services.BagheeraClient	DEBUG	POST Length: 932
1371839377726	Sqlite.Connection.healthreport.sqlite	DEBUG	Conn #1: Stmt #85 finished.
1371839377726	Services.Metrics.MetricsStorage	TRACE	Queued operation completed.
1371839377839	Services.BagheeraClient	DEBUG	POST https://fhr.data.mozilla.com/1.0/submit/metrics/4a735601-5604-9545-9f14-e00c2f49b8b0 201
1371839377840	Services.HealthReport.HealthReporter	DEBUG	Received Bagheera result.
1371839377840	Services.Metrics.MetricsStorage	TRACE	Enqueueing operation.
1371839377841	Services.Metrics.MetricsStorage	TRACE	Performing queued operation.
1371839377841	Sqlite.Connection.healthreport.sqlite	TRACE	Conn #1: Stmt #86 INSERT OR REPLACE INTO daily_counters VALUES (:field_id, :days, COALESCE((SELECT value FROM daily_counters WHERE field_id = :field_id AND day = :days ), 0) + :by) - {"field_id":13,"days":15877,"by":1}
1371839377841	Services.HealthReport.HealthReporter	WARN	Marking upload as successful.
1371839377841	Services.HealthReport.HealthReporter	INFO	Recording last ping time and deleted remote document.
1371839377841	Services.HealthReport.HealthReporter	WARN	Removing documents from remote ID list: 3100640e-34c6-9147-b402-2d64672ac5be,3f9cb3a1-daa0-11e2-811b-c82a144a924f,3f9cb3a1-daa1-11e2-811b-c82a144a924f
1371839377841	Services.HealthReport.HealthReporter	INFO	Writing state file: /Users/gps/Library/Application Support/Firefox/Profiles/88fdn15c.fhr00/healthreport/state.json

End-to-end verification that the server is doing the right thing will be desired at some point.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 765980 [details] [diff] [review]
Enhancing Bagheera to support deleting multiple documents;

Review of attachment 765980 [details] [diff] [review]:
-----------------------------------------------------------------

I'll fix things for you and commit this.

::: netwerk/test/httpserver/httpd.js
@@ +281,1 @@
>     *   the date to process

How did you manage to touch this file?

::: services/common/bagheeraclient.js
@@ +163,5 @@
>  
> +    // since API changed, throw on old API usage
> +    if (options.deleteID !== undefined) {
> +      throw new Error("API has changed, use (array) deleteIDs instead");
> +    }

You can also write this as:

if ("deleteID" in options)

I like this because it catches callers still referring to the old API.

@@ +193,5 @@
>  
>      let result = new BagheeraClientRequestResult();
>      result.namespace = namespace;
>      result.id = id;
> +    result.deleteIDs = deleteIDs;

result.deleteIDs = deleteIDs.slice(0);

Comment 11

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/fx-team/rev/98ffaa4da1c7

Comment 12

[Gregory Szorc [:gps]]

This will presumably be in Saturday or Sunday's Nightly 24. Aurora 24 starts next week sometime. And, I've requested uplift to 23 (hopefully in time for beta 1 or beta 2 at the least). So, expect desktop clients to start sending multidelete on upload soon.

If server functionality is broken, we may permanently orphan records on Nightly and Aurora channels.

What I'm trying to say is "heads up: now is a good time to verify multidelete is working properly on production Bagheera."

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

This is the first we're hearing of this so please nominate with a risk/reward assessment so we can evaluate how feasible this is for Beta.

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 765980 [details] [diff] [review]
Enhancing Bagheera to support deleting multiple documents;

[Approval Request Comment]
Bug caused by (feature/regressing bug #): FHR
User impact if declined: There shouldn't be user impact. Impact should only be seen by Mozilla.
Testing completed (on m-c, etc.): Should likely bake a day or two. Metrics needs to verify change is sane.
Risk to taking this patch (and alternatives if risky): Should be very low risk from a client perspective. Most of the risk is on Metrics.
String or IDL/UUID changes made by this patch: None

This is an uplift request for 23. I'm assuming it will be in beta by the time we get around to doing the uplift.

This patch is very important from the perspective of Metrics. It should hopefully severely curtail "orphaned records" on the FHR server. It will allow Metrics to produce better numbers from FHR 6 weeks sooner.

The patch should be low risk from a client perspective. We would have written and landed this earlier (presumably uplifting to Aurora/23), but it was blocked on a server deployment which occurred on 2013-06-21.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/98ffaa4da1c7

Comment 16

[Alex Keybl [:akeybl]]

Comment on attachment 765980 [details] [diff] [review]
Enhancing Bagheera to support deleting multiple documents;

Please make sure to verify this with each client/server combo where this lands so that we don't have surprises later in the cycle. Thanks!

Comment 17

[u279076]

(In reply to Alex Keybl [:akeybl] from comment #16)
> Comment on attachment 765980 [details] [diff] [review]
> Enhancing Bagheera to support deleting multiple documents;
> 
> Please make sure to verify this with each client/server combo where this
> lands so that we don't have surprises later in the cycle. Thanks!

Greg, please advise what Desktop QA can do to assist in the testing of this bug.

Comment 18

[Daniel Einspanjer [:dre] [:deinspanjer]]

Greg can provide details on how to get a client to have multiple docs, but after that is done, would these steps work as a manual QA test?

1. Prep desktop to have multiple documents for deletion (see :gps)

2. Open a bug under Metrics product requesting devops assistance verifying deletion.  Supply the list of IDs that will be deleted in the bug (see :gps).

3. Metrics will verify which of those IDs currently exist.  Note it might take up to an hour after the submission of a document for it to be visible in HBase.

4. After getting verification of which documents currently exist, cause the desktop to perform an FHR submission (see :gps).

5. Verify in logs that multiple deletions were requested (see :gps)

6. Update the Metrics bug with the general time of the new submission

7. Metrics will verify that any of the documents that were previously found are now deleted.  Again, this can take up to an hour from submission.

Comment 19

[u279076]

If that works for Greg and Metrics that works for me.

Comment 20

[Gregory Szorc [:gps]]

The steps in comment #18 look good to me. Getting the client to reference multiple records that exist on the server without actually deleting them will require manual intervention.

You'll want to wait for FHR to upload data. After that, the healthreport/state.json file in the profile should contain a single document UUID. Close Firefox, remove that UUID from the list, save the file, reopen Firefox, and force a data submission. You now have 2 documents on the server. Close Firefox then add the original UUID back into the state.json file. Both documents should be deleted on next upload!

Comment 21

[Gregory Szorc [:gps]]

https://hg.mozilla.org/releases/mozilla-beta/rev/66edadd3a8d8

I went ahead and uplifted this because b1 was cut today and that leaves almost a full week for a beta build to be verified before the patch is released upon users.

Comment 22

[Gregory Szorc [:gps]]

And backed out for xcpshell test failure. I'll build beta locally and pin this down.

https://hg.mozilla.org/releases/mozilla-beta/rev/adc34e09b516

Comment 23

[Gregory Szorc [:gps]]

Looks like test_bagheera_client.js picked up Promise.jsm in this patch. This JSM isn't available in 23. If I replace with promise.js, things should just work.

I'll land that in the morning after testing locally and so I can be around to watch the tree.

Comment 24

[Gregory Szorc [:gps]]

My hypothesis was correct. Using the old promise.js in test_bagheera_client.js fixed it.

https://hg.mozilla.org/releases/mozilla-beta/rev/bc315632a5e7

Comment 25

[u279076]

Where are we at now with the timeline in comment 18?

Comment 26

[u279076]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #25)
> Where are we at now with the timeline in comment 18?

Bump. We're almost halfway through the Beta cycle for Firefox 23.

Comment 27

[Gregory Szorc [:gps]]

It was just stated in the FHR meeting that the orphan rate has apparently decreased dramatically. This might be sufficient verification. But I'll let Brendan or someone else from Metrics weigh in.

Comment 28

[brendan c]

(In reply to Gregory Szorc [:gps] from comment #27)
> It was just stated in the FHR meeting that the orphan rate has apparently
> decreased dramatically. This might be sufficient verification. But I'll let
> Brendan or someone else from Metrics weigh in.

Correct, the orphaning rate has dropped dramatically; however, my analysis does not at all follow the detailed steps Daniel suggested in comment 18. I don't feel that I have the expertise to say whether or not my analysis is sufficient to serve as part of the client QA he outlined. All I can say is that the patch does seem to have had the desired effect of reducing the orphaning rate.

Note also that at this point I cannot say that orphaning has been completely zeroed or anything like that. I also cannot say whether or not the rate has dropped beneath the what it was before the previous major patch that caused it to dramatically jump (the one introduced in nightly and aurora around May 11 and 14 respectively).

Comment 29

[u279076]

(In reply to brendan c from comment #28)
> All I can say is that the patch does seem to have had the desired effect of reducing the
> orphaning rate.

Thanks Brendan, for now I'm going to mark this [qa-] as there doesn't seem to be a good way to 100% verify this is fixed. I trust that we'll know for sure once we collect more data and can follow-up with any bugs as needed.