Closed Bug 764587 Opened 12 years ago Closed 12 years ago

When downloading a complete mar after the partial fails it shouldn't be throttled.

Categories

(Toolkit :: Application Update, defect)

Product:
Toolkit
Component:
Application Update
Version:
15 Branch
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla16
Tracking Flags:
Tracking Status
firefox15 + fixed

People

(Reporter: robert.strong.bugs, Assigned: ehsan.akhgari)

References

Details

Attachments

(1 file)

Patch (v1)
3.25 KB, patch
robert.strong.bugs
: review+
akeybl
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
Ehsan, it *might* make sense to download unthrottled when it fails in this case but I haven't had a chance to think it through
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#1027
Hmm, isn't this just a dupe of bug 764377?
(In reply to Robert Strong [:rstrong] (do not email) from comment #1)
> Ehsan, it *might* make sense to download unthrottled when it fails in this
> case but I haven't had a chance to think it through
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/
> nsUpdateService.js#1027

Hmm, if we have concerns about our mirror network capacity, I'd guess that the same concerns apply here too, right?  Cause most people want to download and use the partials, which are way smaller than complete mars.
No, what he is asking for is that background downloads aren't throttled in the way they have always been. See bug 764377 comment #26, specifically, "Even in the case where the UI is not displayed it makes no sense to make a 30 second download take almost an hour and a half..."
(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #1)
> > Ehsan, it *might* make sense to download unthrottled when it fails in this
> > case but I haven't had a chance to think it through
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/
> > nsUpdateService.js#1027
> 
> Hmm, if we have concerns about our mirror network capacity, I'd guess that
> the same concerns apply here too, right?  Cause most people want to download
> and use the partials, which are way smaller than complete mars.
Only if it is a common case. For example, when we are unable to apply a mar we download unthrottled when we start back up. So yes, it is a concern if this is common and it is not if it is not common.
Heh, ok fair enough.  :-)
Assignee: nobody → ehsan
tracking-firefox15: ? → +
(In reply to Robert Strong [:rstrong] (do not email) from comment #5)
> (In reply to Ehsan Akhgari [:ehsan] from comment #3)
> > (In reply to Robert Strong [:rstrong] (do not email) from comment #1)
> > > Ehsan, it *might* make sense to download unthrottled when it fails in this
> > > case but I haven't had a chance to think it through
> > > http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/
> > > nsUpdateService.js#1027
> > 
> > Hmm, if we have concerns about our mirror network capacity, I'd guess that
> > the same concerns apply here too, right?  Cause most people want to download
> > and use the partials, which are way smaller than complete mars.
> Only if it is a common case. For example, when we are unable to apply a mar
> we download unthrottled when we start back up. So yes, it is a concern if
> this is common and it is not if it is not common.

To the best of my knowledge, it should not be common.  But I don't have hard evidence to support this claim.  :-)
The reason this is common is probably because of bug 764269.
Understood and both regressions need to be fixed.
I really wish you people would follow these bugs without me having to point things out to you particularly since you seem to be adverse to accepting my input on things.
Can you please stop with these type of comments. I and others follow the app update component so yes, I am following this and the other bugs. Your input has been and is accepted... I gave you a path forward by filing a mirror network bug to get your input regarding the mirror network into the hands of the people that can do something with it but you seem dead set on trying to get those that can't do anything with it directly to use it. After I have some breathing room I'll even file the bug if you don't but as far as this bug is concerned we already understand what is going on as I explained to you several times in bug 764377 so no more input is needed.
(In reply to Robert Strong [:rstrong] (do not email) from comment #11)
> Can you please stop with these type of comments. I and others follow the app
> update component so yes, I am following this and the other bugs. Your input
> has been and is accepted... I gave you a path forward by filing a mirror
> network bug to get your input regarding the mirror network into the hands of
> the people that can do something with it but you seem dead set on trying to
> get those that can't do anything with it directly to use it. After I have
> some breathing room I'll even file the bug if you don't but as far as this
> bug is concerned we already understand what is going on as I explained to
> you several times in bug 764377 so no more input is needed.

But my issue is that I don't think that this is a mirror network issue.  Not sure why you don't get this.  I guess you really don't pay any attention to what I really say in my comments.
It is a mirror network issue in that the throttling was implemented to prevent the mirror network from being overloaded. Any changes to the client side throttling would first need to be evaluated and agreed to by the people that manage the mirror networks.
And PLEASE, take your comments back to bug 764377. This bug is specifically about the regression caused by bgupdates.
This is a bug I filed.  It is about what I say it is about and not what you say it is about.
No, I filed this bug to specifically be about the regression for bgupdates. You filed bug 764377.
(In reply to Robert Strong [:rstrong] (do not email) from comment #1)
> Ehsan, it *might* make sense to download unthrottled when it fails in this
> case but I haven't had a chance to think it through
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/
> nsUpdateService.js#1027

After more thought, I think this makes sense.  Patch forthcoming.
Attached patch Patch (v1) — — Splinter Review 

        Attachment #632992 -
        Flags: review?(robert.bugzilla)

        Attachment #632992 -
        Flags: review?(robert.bugzilla) → review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/15aaf2a0cb1d
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16

    
    

    
  
Could you summarise the change this makes ? What scenarios will be download the complete at full speed where we would have done background before ?

    
    

    
  
Previously, if the partial download failed or the mar didn't pass any of the checks it would download the complete. This would be done in the UI and it would be without throttling. This makes it so it will do the same for the background updates case.

    
    

    
  
background staging of the update that is

    
    

    
  
Thanks Robert.

    
  

          status-firefox15:
          --- → affected

    
    

    
  
Comment on attachment 632992 [details] [diff] [review]
Patch (v1)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 759065
User impact if declined: See comment 0
Testing completed (on m-c, etc.): Both locally and has baked for a few days on m-c
Risk to taking this patch (and alternatives if risky): Minimal
String or UUID changes made by this patch: none

        Attachment #632992 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Comment on attachment 632992 [details] [diff] [review]
Patch (v1)

[Triage Comment]
Let's land on Aurora 15 in support of re-enabling the feature.

        Attachment #632992 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: