Closed
Bug 810132
Opened 12 years ago
Closed 12 years ago
Add remote data deletion to policy and scheduling
Categories
(Firefox Health Report Graveyard :: Client: Desktop, defect)
Firefox Health Report Graveyard
Client: Desktop
Tracking
(firefox18 fixed, firefox19 fixed, firefox20 fixed)
RESOLVED
FIXED
Firefox 20
People
(Reporter: gps, Assigned: gps)
References
Details
Attachments
(1 file, 1 obsolete file)
35.43 KB,
patch
|
rnewman
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
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.
Updated•12 years ago
|
Attachment #679955 -
Attachment is patch: true
Comment 2•12 years ago
|
||
> 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.
Assignee | ||
Comment 3•12 years ago
|
||
Addressed feedback. Added some more comments.
Attachment #679955 -
Attachment is obsolete: true
Attachment #679955 -
Flags: review?(rnewman)
Attachment #680107 -
Flags: review?(rnewman)
Comment 4•12 years ago
|
||
(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 5•12 years ago
|
||
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.
Assignee | ||
Comment 6•12 years ago
|
||
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.
Comment 7•12 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #6) > I think a isDelete forest would > merely turn into an "instanceof" forest. Method dispatch! :D
Updated•12 years ago
|
Attachment #680107 -
Flags: review?(rnewman) → review+
Assignee | ||
Comment 8•12 years ago
|
||
https://hg.mozilla.org/projects/larch/rev/03d595b3a5e1
Whiteboard: [fixed-in-larch]
Comment 9•12 years ago
|
||
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?
Comment 10•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/525e8539150a
Whiteboard: [fixed-in-larch]
Updated•12 years ago
|
Target Milestone: --- → mozilla20
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/525e8539150a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment 12•12 years ago
|
||
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+
Comment 13•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/2e7cd0dfaa96 https://hg.mozilla.org/releases/mozilla-beta/rev/7c38a784b962
Updated•11 years ago
|
Component: Metrics and Firefox Health Report → Client: Desktop
Flags: in-testsuite+
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla20 → Firefox 20
Updated•6 years ago
|
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•