Closed
Bug 890040
Opened 11 years ago
Closed 11 years ago
Ensure orphaning resistance and multi-delete
Categories
(Firefox Health Report Graveyard :: Client: Android, defect)
Tracking
(firefox24 fixed, firefox25 fixed)
RESOLVED
FIXED
Firefox 25
People
(Reporter: rnewman, Assigned: nalexander)
References
()
Details
(Keywords: qawanted)
Attachments
(1 file)
53.19 KB,
patch
|
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Just making sure we have a bug to track this.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → nalexander
Assignee | ||
Comment 1•11 years ago
|
||
This can be tested with http://people.mozilla.com/~nalexander/background-services-lab.xpi.
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 2•11 years ago
|
||
https://hg.mozilla.org/services/services-central/rev/4e0a9e8a698c Pushed to s-c since I didn't have a recent m-c or m-i build to test against.
Status: NEW → ASSIGNED
Whiteboard: [fixed in services]
Target Milestone: --- → Firefox 25
Assignee | ||
Comment 3•11 years ago
|
||
tracy, can we get a smoke test on the background uploader? (Against the s-c APK built above. You could try the new Background Services Lab xpi above, too!) 1) let's be sure that regular upload is looking okay -- uploads succeed and delete the old id. The lab will show you the obsolete document map, and the messages should say "obsoleting 1 id". 2) let's be sure that when you turn uploading off, any remaining document ids are queued for deletion (and deleted). Thanks!
Keywords: qawanted
Assignee | ||
Comment 4•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4e0a9e8a698c
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Assignee | ||
Comment 5•11 years ago
|
||
[Approval Request Comment] Bug caused by (feature/regressing bug #): This is a correctness patch that takes care to avoid orphaned FHR records. User impact if declined: higher possibility of orphaned FHR records. Testing completed (on m-c, etc.): baked on m-c for a few weeks. No crashes seen. I believe we are not yet analyzing any of the Android data, so I can't say yet say this is helping or hurting in the wild. Risk to taking this patch (and alternatives if risky): moderate risk. There's always a chance this will hit problems in the larger Aurora and Beta population; lifting to Beta would land it on Release in a week. We're not yet analyzing Android data, so this could ruin everything; but on the other hand, since we're not yet analyzing, this theoretically better code could give us a better baseline for future analysis. String or IDL/UUID changes made by this patch: none.
Attachment #785168 -
Flags: approval-mozilla-beta?
Attachment #785168 -
Flags: approval-mozilla-aurora?
Comment 6•11 years ago
|
||
The ship for Fx23 has already sailed and this cannot land into beta. Can we help run an analysis on nightly starting when the patch landed to confirm if this is working as expected in order to uplift it to aurora? Given the risk and uncertainty I'd rather wait to make sure this is being helpful for analysis rather than ruining it before realizing it too late.
Updated•11 years ago
|
Flags: needinfo?(nalexander)
Assignee | ||
Comment 7•11 years ago
|
||
Hello release drivers, sorry to let this one sit over the weekend, but it was a special Canadian holiday weekend and I was enjoying the sun. lsblakk asked on IRC what leaving this for another six weeks cost us, and the answer is that I can't quantify that. I haven't pushed for better Android data and Metrics has been busy processing desktop data so I can't say what effect is has had and will have. Therefore, I think we play it safe and let this wait. We dealt with orphaning problems on desktop; if they arise, we can deal with them on Android.
Flags: needinfo?(nalexander)
Comment 8•11 years ago
|
||
I suspect we should take this regardless of the data: the risk of taking it is probably less than the risk of not taking it. That said, bcolloran should be able to check some stats.
Flags: needinfo?(bcolloran)
Updated•11 years ago
|
Attachment #785168 -
Flags: approval-mozilla-aurora?
no one on the stats team is currently looking at fennec, including orphaning. no one is yet spun up on the v3 payload format, so even if this is made top priority within our team, it would be some time before anything can be said about orphaning at fennec.
Flags: needinfo?(bcolloran)
Comment 10•11 years ago
|
||
(In reply to Nick Alexander :nalexander from comment #5) > Created attachment 785168 [details] [diff] [review] > Patch that landed on m-c > > [Approval Request Comment] > Bug caused by (feature/regressing bug #): This is a correctness patch that > takes care to avoid orphaned FHR records. > > User impact if declined: higher possibility of orphaned FHR records. > > Testing completed (on m-c, etc.): baked on m-c for a few weeks. No crashes > seen. I believe we are not yet analyzing any of the Android data, so I > can't say yet say this is helping or hurting in the wild. > > Risk to taking this patch (and alternatives if risky): moderate risk. > There's always a chance this will hit problems in the larger Aurora and Beta > population; lifting to Beta would land it on Release in a week. We're not > yet analyzing Android data, so this could ruin everything; but on the other > hand, since we're not yet analyzing, this theoretically better code could > give us a better baseline for future analysis. > > String or IDL/UUID changes made by this patch: none. (In reply to Benjamin Smedberg [:bsmedberg] PTO 8-Aug until 18-Aug, workweek high latency 19-Aug through 23-Aug from comment #8) > I suspect we should take this regardless of the data: the risk of taking it > is probably less than the risk of not taking it. That said, bcolloran should > be able to check some stats. Bsmedberg, I was worried about this as it was called out in the risk that this could ruin everything.But your risk assessment does not comply with that.Can we please get some more information here ? looks like my idea about getting the data may not work so what are are other alternatives ? If this really helps and is low risk I am happy to take it for Beta 2, but I do not want to take something that someone may have already called out as a potential hazard.
Comment 11•11 years ago
|
||
I touch based with :nalexander since benjamin is away, given there is no metrics data he is going to run a few tests to be more assuring that the patch will be helpful. Based on his response we should uplift to beta asap.
Assignee | ||
Comment 12•11 years ago
|
||
(In reply to bhavana bajaj [:bajaj] (on vacation until 08/14/2013) from comment #11) > I touch based with :nalexander since benjamin is away, given there is no > metrics data he is going to run a few tests to be more assuring that the > patch will be helpful. > > Based on his response we should uplift to beta asap. bajaj and I talked about testing a possible backout and how that would affect beta users. I tested by backing out (against mozilla-central) https://hg.mozilla.org/mozilla-central/rev/3d58bd987452 (Bug 893910, which landed on top) and then https://hg.mozilla.org/mozilla-central/rev/4e0a9e8a698c (Bug 890040, this bug). I saw no adverse runtime effects. I'm happy that backout won't strand beta users (other than possibly orphaning FHR records, which would have been orphaned with the old code as well).
Assignee | ||
Comment 13•11 years ago
|
||
(In reply to Nick Alexander :nalexander from comment #12) > (In reply to bhavana bajaj [:bajaj] (on vacation until 08/14/2013) from > comment #11) > > I touch based with :nalexander since benjamin is away, given there is no > > metrics data he is going to run a few tests to be more assuring that the > > patch will be helpful. > > > > Based on his response we should uplift to beta asap. > > bajaj and I talked about testing a possible backout and how that would > affect beta users. I tested by backing out (against mozilla-central) > > https://hg.mozilla.org/mozilla-central/rev/3d58bd987452 (Bug 893910, which > landed on top) > and then > https://hg.mozilla.org/mozilla-central/rev/4e0a9e8a698c (Bug 890040, this > bug). > > I saw no adverse runtime effects. I should have been more detailed. I tested by running m-c, ensuring that FHR uploads occurred, and then installing a new m-c with the changesets above backed out. Then I verified that FHR was still functioning and uploading continued apace.
Comment 14•11 years ago
|
||
Comment on attachment 785168 [details] [diff] [review] Patch that landed on m-c Ok let's get this into our second Beta then so we have as much time as possible to ensure no unexpected fallout.
Attachment #785168 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 15•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/e79e40a382d4
Whiteboard: [status-firefox24:fixed]
Updated•11 years ago
|
Assignee | ||
Comment 16•11 years ago
|
||
Build bustage on beta: https://tbpl.mozilla.org/?tree=Mozilla-Beta&rev=027478bc86e2 I'm going to investigate and will either backout or push a fix ASAP.
Assignee | ||
Comment 17•11 years ago
|
||
For reasons unknown, the patch that landed as part of Bug 870169 did not land the appropriate build system change, which slipped in to this patch. I'm going to revert the offending line, r=me.
Assignee | ||
Comment 18•11 years ago
|
||
Lukas, I carried forward your a= but doing so was a little duplicitous. Please correct me if I should do otherwise.
Assignee | ||
Comment 19•11 years ago
|
||
Sigh, two follow-ups because I'm a nob. https://hg.mozilla.org/releases/mozilla-beta/rev/95c3d9020d19 https://hg.mozilla.org/releases/mozilla-beta/rev/8d59a33ddcab
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
•