Closed Bug 810132 Opened 7 years ago Closed 7 years ago

Add remote data deletion to policy and scheduling

Categories

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

defect
Not set

Tracking

(firefox18 fixed, firefox19 fixed, firefox20 fixed)

RESOLVED FIXED
Firefox 20
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(1 file, 1 obsolete file)

FHR clients need a way to request deletion of remote server data. The naive implementation simply issues the Bagheera HTTP request. If it works, great. But, if it fails, data isn't actually deleted. Maybe we try again. What's reflected in UIs? If the application shuts down before the response is received, what happens?

There's a fair bit of complexity around remote data deletion. When we take it all in, deletion of remote server data starts to look an awful lot like data uploading (at least in terms of scheduling, retries, etc). I think we should integrate data deletion into the policy/scheduler and it should have the same or nearly the same behavior as uploading.

I'm thinking we could store a flag in a preference saying "client requests remote data deletion." The policy can periodically issue requests to its listener to delete remote data. It can respond in much the same way as data uploading. The only real difference from data uploading is that deletion would take priority above all other requests (including upload). And, deletion would be allowed to occur even if the kill switches are turned off.

Implementing this should be relatively straightforward. It may involve refactoring some blocks into functions. It will likely entail some variable name changes and possibly some prefs changes. Because of this, I'd like to get it in before the merge.
I think this does it. I'm really tired and have a non-0 ABV, so please exercise strict scrutiny, especially in the naming department.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #679955 - Flags: review?(rnewman)
Blocks: 718066
Attachment #679955 - Attachment is patch: true
> The only real difference from data uploading is that deletion would take priority
> above all other requests (including upload). And, deletion would be allowed to occur
> even if the kill switches are turned off.

versus

> +   * This is the master switch for remote server communication. If it is
> +   * false, we never request upload or deletion.

Which is true? It looks like the code disables all submissions (as stated by the comment in the patch), rather than just uploads. I prefer the previously described approach that deletion is allowed regardless of the state of "dataSubmissionEnabled".

And, as requested, 2 nits re: naming:
> +   * If this is true, the policy will request that remote data be deleted.
> +   * Furthermore, no new data will be submitted (if it's even allowed) until
> +   * the remote deletion is fulfilled.

s/submitted/uploaded/ in keeping with the new terminology.

> +      if (request.isDelete) {
> +        this.pendingDeleteRemoteData = false;
> +        this._log.info("Successful data delete reported.");
> +      } else {
> +        this._log.info("Successful data submission reported.");
> +      }

s/submission/upload/ in keeping with the new terminology.
Addressed feedback. Added some more comments.
Attachment #679955 - Attachment is obsolete: true
Attachment #679955 - Flags: review?(rnewman)
Attachment #680107 - Flags: review?(rnewman)
(In reply to Mark Reid [:mreid] from comment #2)

> Which is true? It looks like the code disables all submissions (as stated by
> the comment in the patch), rather than just uploads. I prefer the previously
> described approach that deletion is allowed regardless of the state of
> "dataSubmissionEnabled".

I saw this before I went to bed, and elected to finish the review in the morning :)

I just deleted my three paragraphs of discussing this, and suffice to say: I agree; we'll respond to more nuanced decisions of whether to upload or not (e.g., background data disabled) _in the uploading code_.
Comment on attachment 680107 [details] [diff] [review]
Add deletion requests to policy and scheduling, v2

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

::: services/healthreport/policy.jsm
@@ +337,5 @@
>    STATE_NOTIFY_COMPLETE: "ok",
>  
> +  REQUIRED_LISTENERS: [
> +    "onRequestDataUpload",
> +    "onRequestRemoteDelete",

This feels janky. When I request upload, I shouldn't have to provide a deletion handler.

I am torn between simplicity+jankiness and the slight additional complexity of defining DataUploadRequest and DataDeletionRequest subclasses. There's a growing little forest of `if (foo.isDelete)` further down this file, which means we're probably missing an accurate abstraction somewhere.

Give it some thought.
The policy is tightly coupled with data management. And, upload and deletion are part of that. I think it's appropriate to require both listeners.

I agree we could probably split into 2 *Request types. I reused the existing because, well, it fits the model quite well. Essentially the only difference is isDelete. The logic for processing responses is nearly identical and the code would be used by both request types. I think a isDelete forest would merely turn into an "instanceof" forest.
(In reply to Gregory Szorc [:gps] from comment #6)
> I think a isDelete forest would
> merely turn into an "instanceof" forest.

Method dispatch! :D
Blocks: 808219
Attachment #680107 - Flags: review?(rnewman) → review+
Comment on attachment 680107 [details] [diff] [review]
Add deletion requests to policy and scheduling, v2

Bulk-setting approval flags for FHR landing for FxOS ADU ping (Bug 788894).
Attachment #680107 - Flags: approval-mozilla-beta?
Attachment #680107 - Flags: approval-mozilla-aurora?
Target Milestone: --- → mozilla20
https://hg.mozilla.org/mozilla-central/rev/525e8539150a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment on attachment 680107 [details] [diff] [review]
Add deletion requests to policy and scheduling, v2

FHR for B2G ADU ping, won't be built/enabled for Mobile/Desktop.
Attachment #680107 - Flags: approval-mozilla-beta?
Attachment #680107 - Flags: approval-mozilla-beta+
Attachment #680107 - Flags: approval-mozilla-aurora?
Attachment #680107 - Flags: approval-mozilla-aurora+
Component: Metrics and Firefox Health Report → Client: Desktop
Flags: in-testsuite+
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla20 → Firefox 20
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.