Closed Bug 890040 Opened 7 years ago Closed 7 years ago

Ensure orphaning resistance and multi-delete

Categories

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

All
Android
defect
Not set

Tracking

(firefox24 fixed, firefox25 fixed)

RESOLVED FIXED
Firefox 25
Tracking Status
firefox24 --- fixed
firefox25 --- fixed

People

(Reporter: rnewman, Assigned: nalexander)

References

()

Details

(Keywords: qawanted)

Attachments

(1 file)

Just making sure we have a bug to track this.
Assignee: nobody → nalexander
Blocks: 894194
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
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
https://hg.mozilla.org/mozilla-central/rev/4e0a9e8a698c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
[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?
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.
Flags: needinfo?(nalexander)
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)
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)
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)
(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.
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.
(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).
(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 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+
Whiteboard: [status-firefox24:fixed]
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.
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.
Lukas, I carried forward your a= but doing so was a little duplicitous.  Please correct me if I should do otherwise.
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.