Closed Bug 1168381 Opened 9 years ago Closed 9 years ago

ship-it performances: too many SQL queries

Categories

(Release Engineering :: Applications: Shipit, defect)

Product:
Release Engineering
Component:
Applications: Shipit
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: Sylvestre, Assigned: asilva, Mentored)

References

Details

(Whiteboard: [good first bug][lang=Python])

Attachments

(2 files)

dump
1.56 MB, text/x-sql
Details
product_releases.sql
2.92 KB, application/sql
Details 
I have a recent dump of the database and the /releases.html page triggers more than 3000 queries.

I see, at least, two ideas to decrease this number:
1) For each release, we need to know if a release with a different build number has been shipped or not.
To do this, we are doing a query to know if another release matches the product and filter.
This has been implemented for bug 1121032 ( https://github.com/mozilla/ship-it/commit/c7f959f5eed87c280a26886183481bd347fcb1b0 )

I am pretty sure there is a better way to do this

2) The "Running/Complete" view has all entries (more than a 1 000). In most of the case, we only need the 20 last versions published (and it is ok to have Android, Firefox & Thunderbird)
Yikes, I didn't realize this was happening!

(In reply to Sylvestre Ledru [:sylvestre] from comment #0)
> I have a recent dump of the database and the /releases.html page triggers
> more than 3000 queries.
> 
> I see, at least, two ideas to decrease this number:
> 1) For each release, we need to know if a release with a different build
> number has been shipped or not.
> To do this, we are doing a query to know if another release matches the
> product and filter.
> This has been implemented for bug 1121032 (
> https://github.com/mozilla/ship-it/commit/
> c7f959f5eed87c280a26886183481bd347fcb1b0 )
> 
> I am pretty sure there is a better way to do this

Definitely...maybe we can do one query per table to get the same information?

> 2) The "Running/Complete" view has all entries (more than a 1 000). In most
> of the case, we only need the 20 last versions published (and it is ok to
> have Android, Firefox & Thunderbird)

Good point. It would probably be okay not to show really old stuff on this page anymore, as long as it's accessible through another endpoint (eg, the product details replacement JSON you've been working). Pagination that talks to the server for additional releases might work too.
(In reply to Ben Hearsum [:bhearsum] from comment #1)
> Definitely...maybe we can do one query per table to get the same information?
Probably.
I tried using SQLAlchemy but failed. So, I did it with the lazy way.


> > 2) The "Running/Complete" view has all entries (more than a 1 000). In most
> > of the case, we only need the 20 last versions published (and it is ok to
> > have Android, Firefox & Thunderbird)
> 
> Good point. It would probably be okay not to show really old stuff on this
> page anymore, as long as it's accessible through another endpoint (eg, the
> product details replacement JSON you've been working). 
So, maybe only show releases from the last X months and add an option to get the full list.
I'd like to tackle this bug, though I think I will have to wrap my head around some of the stuff involved. In the meantime, I'll try to learn what I should do! :)
Is there some way I can retrieve a dump of the release database? I'm having trouble figuring out where I might get such a thing for testing.
I know SQLite parses a portion of regular SQL, so I assumed I was just supposed to use it 'as-is'. Is that not the case? kickoff-web.py returns a parsing error when I pass in the database dump.
This is a MySQL dump. You probably want to use this db.
(In reply to Sylvestre Ledru [:sylvestre] from comment #7)
> This is a MySQL dump. You probably want to use this db.

Whoops, my bad. I figured it out eventually, haha. Now I can perhaps actually work on the bug!
Would it be possible to somehow get the same information while we're appending to releases[]? That would at least prevent us from having to recurse into the function again. I'm not sure if it would provide fewer queries / any performance increase, though.

The other idea I had was to parse the tables before even iterating over them to make sure everything that should be marked as shipped is as such.
Cody, I am very sorry, i missed your last comment. Have you been able to make some progress on this? Thanks
Flags: needinfo?(codyw)
I haven't done anything yet, as I was scared of going down the wrong path, haha. If you would like, I can attempt the project on my own, though, and come back with some result.
Flags: needinfo?(codyw)
If you want to give it an easy try, the 2) in comment #2 should be easy.

1) is harder as it requires some better understanding of sqlalchemy
Alrighty - I will give it a shot, then!
I've gone so far as to limit the number of entries appended to releases from the db - should I also be working on pagination? I don't know what server I'd be querying (or if I'd have access to it, etc.)
Flags: needinfo?(sledru)
Pagination would be great. We probably need to still have access to all of them.
Or maybe, we should just go with the 50 last releases and provide an input item to set more if needed.

About the server, not sure that matters but SQLite for dev and MySQL for prod.
Flags: needinfo?(sledru)
Looks like SQLAlchemy has a module for pagination already, so I am taking a look into that; it might be nice to query a database for a given next page asynchronously, so we can get as many releases as we want.
(In reply to Cody Welsh from comment #16)
> Looks like SQLAlchemy has a module for pagination already
That would be perfect :)
Any news? This is starting to be a problem for the release management team. If you don't have time, no worries, I will try to have a look.
Ah, apologies - it has been difficult due to medical issues, so I wouldn't be offended if someone else fixed the bug. Sorry!
I would like to work on this bug, can you please assign this bug to me ?
Tummala, sure, I will assign it as soon as I see a first patch :)
Is anyone working on this bug now?
ex-dev: not that I am aware of.
Ok, I will analyze it.
I saw that the database structure has changed after that you posted the dump, some columns, I guess. Do you have a new dump? (if no, no problem).
The schema didn't change. We added some new fields. See migrate_repo/README to add them
Hi, 
I see a problem to implement pagination with existing code.
getReleases(args) gets the results querying table per table, so if we set the pagination to show 5 lines per page, the results will be 15 lines per page.

My suggestion is create a view(setting null to unmatched columns) that unites the 3 tables consolidating the results. Then apply the pagination.

After will be more easy to think about update the releases list with Ajax.

My question is: -There is some problem to create a view?
Good catch!
I don't think it is a problem as long as the view is correctly managed by sqlite, mysql and postgresql.
Ok, I will start tests with views in some days.
I also saw that the Ajax will be indispensable to improve the performance.
Hi Sylvester,
I'm still working on this bug. The running/complete list is configured by JSON, the logic was moved from html to javascript.

If possible, get my code from GitHub e take a look if the columns behaviors are the same and tell me if pagination is the expected.

https://github.com/ex-Dev/ship-it

For now only the running/complete list was updated.

It is necessary to create a view. I attached the view file.

The View needs to be created before running kickoff.py, because if not a table with same view name is created.
Attached file product_releases.sql — 

    
    

    
  
Allan, could you go a pull request? Thanks

    
  
Assignee: nobody → allan.tavares

    
    

    
  
OK, I will finish the "reviewed releases" list and build the migraterepo version, then I make a pull request.
Thanks!

    
    

    
  
Hi, 
I made a pull request, I dont know how pull requests works(if code is merged or not on original source). For tests, I recommend to get the code from https://github.com/ex-Dev/ship-it

A View was created to fills the two lists (reviewed and running/complete). Now the pagination occurs on server side and a ajax call is made to change the page.
A new getReleases() function was created to prevent impacts on others locations.

    
    

    
  
Deployed, the performance improvement is huge. Many thanks!
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED

    
    

    
  
I saw this in action today and it's *awesome*. Thank you so much!
Blocks: 1251309
Component: Applications: ShipIt (backend) → Applications: ShipIt

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: