[PATCH conductor] BZ #794759 - Pool Family page no longer runs thousands of SQL queries
Tomas Sedovic
tsedovic at redhat.com
Mon Feb 20 15:38:08 UTC 2012
On 02/17/2012 05:47 PM, Matt Wagner wrote:
> This beats PoolFamily#statistics and Pool#statistics into shape, and
> also avoids invoking the method over and over again in the views.
>
> Resolves https://bugzilla.redhat.com/show_bug.cgi?id=794759
> ---
> src/app/models/instance.rb | 2 +
> src/app/models/pool.rb | 3 +-
> src/app/models/pool_family.rb | 9 ++++---
> src/app/views/pool_families/_list.html.haml | 28 +++++++++++++------------
> src/app/views/pool_families/_pools.html.haml | 15 +++++++------
> src/app/views/pools/_list.html.haml | 9 ++++---
> 6 files changed, 36 insertions(+), 30 deletions(-)
>
> diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb
> index a7a82b5..c2a1d04 100644
> --- a/src/app/models/instance.rb
> +++ b/src/app/models/instance.rb
> @@ -119,6 +119,8 @@ class Instance< ActiveRecord::Base
> scope :pending, :conditions => { :state => [STATE_NEW, STATE_PENDING] }
> # FIXME: "failed" is misleading too...
> scope :failed, :conditions => { :state => [STATE_CREATE_FAILED, STATE_ERROR, STATE_VANISHED] }
> + scope :stopped, :conditions => {:state => STATE_STOPPED}
> + scope :not_stopped, :conditions => "state<> 'stopped'"
> scope :stopable, :conditions => { :state => [STATE_NEW, STATE_PENDING, STATE_RUNNING] }
> scope :ascending_by_name, :order => 'instances.name ASC'
>
> diff --git a/src/app/models/pool.rb b/src/app/models/pool.rb
> index 8529a9d..b1fc41e 100644
> --- a/src/app/models/pool.rb
> +++ b/src/app/models/pool.rb
> @@ -92,8 +92,7 @@ class Pool< ActiveRecord::Base
> statistics = {
> :cloud_providers => instances.collect{|i| i.provider_account}.uniq.count,
> :deployments => deployments.count,
> - :total_instances => (instances.deployed.count +
> - instances.pending.count + instances.failed.count),
> + :total_instances => instances.not_stopped.count,
> :instances_deployed => instances.deployed.count,
> :instances_pending => instances.pending.count,
> :instances_failed => failed,
> diff --git a/src/app/models/pool_family.rb b/src/app/models/pool_family.rb
> index c75de3d..f1d0746 100644
> --- a/src/app/models/pool_family.rb
> +++ b/src/app/models/pool_family.rb
> @@ -74,11 +74,12 @@ class PoolFamily< ActiveRecord::Base
> max = quota.maximum_running_instances
> total = quota.running_instances
> avail = max - total unless max.nil?
> + my_pools = pools
could you change this to `cached_pools` or add a little comment there?
Just so that someone doesn't change it back to the `pools` call in 6 months.
> statistics = {
> - :deployments => pools.collect{|p| p.statistics[:deployments]}.sum,
> - :total_instances => pools.collect{|p| p.statistics[:total_instances]}.sum,
> - :instances_pending => pools.collect{|p| p.statistics[:instances_pending]}.sum,
> - :instances_failed => pools.collect{|p| p.statistics[:instances_failed_count]}.sum,
> + :deployments => my_pools.collect{|p| p.deployments.count}.sum,
> + :total_instances => my_pools.collect{|p| p.instances.not_stopped.count}.sum,
> + :instances_pending => my_pools.collect{|p| p.instances.pending.count}.sum,
> + :instances_failed => my_pools.collect{|p| p.instances.failed.count}.sum,
> :used_quota => total,
> :quota_percent => number_to_percentage(quota.percentage_used,
> :precision => 0),
> diff --git a/src/app/views/pool_families/_list.html.haml b/src/app/views/pool_families/_list.html.haml
> index 40ca206..cde9905 100644
> --- a/src/app/views/pool_families/_list.html.haml
> +++ b/src/app/views/pool_families/_list.html.haml
> @@ -7,6 +7,7 @@
>
> .content
> - @pool_families.each do |pool_family|
> + - @family_stats = pool_family.statistics
Any reason to not use a local variable here ^ instead?
> %table.pool_families
> %caption
> .caption_content
> @@ -31,27 +32,28 @@
> %th= t("pool_families.index.catalog")
> %th
> - pool_family.pools.each do |pool|
> + - @pool_stats = pool.statistics
ditto ^
> %tr
> %td= link_to pool.name, pool
> %td= pool.deployments.count
> - %td= pool.statistics[:total_instances]
> - %td= pool.statistics[:instances_pending]
> - %td= pool.statistics[:instances_failed_count]
> - %td= pool.statistics[:quota_percent]
> - %td= pool.statistics[:used_quota]
> - %td= pool.statistics[:available_quota].nil? ? raw('∞') : pool.statistics[:available_quota]
> + %td= @pool_stats[:total_instances]
> + %td= @pool_stats[:instances_pending]
> + %td= @pool_stats[:instances_failed_count]
> + %td= @pool_stats[:quota_percent]
> + %td= @pool_stats[:used_quota]
> + %td= @pool_stats[:available_quota].nil? ? raw('∞') : @pool_stats[:available_quota]
> %td= pool.enabled? ? t("pool_families.index.answer_yes") : t("pool_families.index.answer_no")
> %td= link_to pool.catalogs.first.name, catalog_path(pool.catalogs.first) if pool.catalogs.any?
> %td= link_to t(:edit), edit_pool_path(pool), :class => 'pool_family_button'
> %tr
> %td= t("pool_families.index.total_statistics")
> - %td= pool_family.statistics[:deployments]
> - %td= pool_family.statistics[:total_instances]
> - %td= pool_family.statistics[:instances_pending]
> - %td= pool_family.statistics[:instances_failed]
> - %td= pool_family.statistics[:quota_percent]
> - %td= pool_family.statistics[:used_quota]
> - %td= pool_family.statistics[:available_quota].nil? ? raw('∞') : pool_family.statistics[:available_quota]
> + %td= @family_stats[:deployments]
> + %td= @family_stats[:total_instances]
> + %td= @family_stats[:instances_pending]
> + %td= @family_stats[:instances_failed]
> + %td= @family_stats[:quota_percent]
> + %td= @family_stats[:used_quota]
> + %td= @family_stats[:available_quota].nil? ? raw('∞') : @family_stats[:available_quota]
> %td
> %td
> %td
> diff --git a/src/app/views/pool_families/_pools.html.haml b/src/app/views/pool_families/_pools.html.haml
> index 68de8c6..e9b03d0 100644
> --- a/src/app/views/pool_families/_pools.html.haml
> +++ b/src/app/views/pool_families/_pools.html.haml
> @@ -1,12 +1,13 @@
> = filter_table(@pools_header, @pool_family.pools) do |pool|
> + - pool_stats = pool.statistics
> %tr{:class => cycle('nostripe','stripe')}
> %td= link_to pool.name, pool
> %td{:class => 'center'}= pool.deployments.count
> - %td{:class => 'center'}= pool.statistics[:total_instances]
> - %td{:class => 'center'}= pool.statistics[:instances_pending]
> - %td{:class => 'center'}= pool.statistics[:instances_failed_count]
> - %td{:class => 'center'}= pool.statistics[:quota_percent]
> - %td{:class => 'center'}= pool.statistics[:used_quota]
> - %td{:class => 'center'}= pool.statistics[:available_quota].nil? ? raw('∞') : pool.statistics[:available_quota]
> + %td{:class => 'center'}= pool_stats[:total_instances]
> + %td{:class => 'center'}= pool_stats[:instances_pending]
> + %td{:class => 'center'}= pool_stats[:instances_failed_count]
> + %td{:class => 'center'}= pool_stats[:quota_percent]
> + %td{:class => 'center'}= pool_stats[:used_quota]
> + %td{:class => 'center'}= pool_stats[:available_quota].nil? ? raw('∞') : pool_stats[:available_quota]
> %td= link_to pool.catalogs.first.name, catalog_path(pool.catalogs.first) if pool.catalogs.any?
> - %td= link_to t(:edit), edit_pool_path(pool), :class => 'rounded-link'
> \ No newline at end of file
> + %td= link_to t(:edit), edit_pool_path(pool), :class => 'rounded-link'
> diff --git a/src/app/views/pools/_list.html.haml b/src/app/views/pools/_list.html.haml
> index 10bd342..5dc5e5d 100644
> --- a/src/app/views/pools/_list.html.haml
> +++ b/src/app/views/pools/_list.html.haml
> @@ -24,16 +24,17 @@
> });
>
> = filter_table(@header, @pools) do |pool|
> + - pool_stats = pool.statistics
> %tr{:class => cycle('nostripe','stripe')}
> %td{:class => 'checkbox'}
> %input{:name => "pools_selected[]", :type => "checkbox", :value => pool.id, :id => "pool_checkbox_#{pool.id}" }
> %td{:class => 'alert'}= (@statistics[:instances_failed]& pool.instances).present? ? image_tag("sb_icon_instance_failure.png") : ""
> %td= link_to(pool.name, pool_path(pool, :view => :filter))
> %td{:class => 'center'}= pool.deployments.count
> - %td{:class => 'center'}= pool.statistics[:total_instances]
> - %td{:class => 'center'}= pool.statistics[:instances_pending]
> - %td{:class => 'center'}= pool.statistics[:instances_failed_count]
> - %td{:class => 'center'}= pool.statistics[:quota_percent]
> + %td{:class => 'center'}= pool_stats[:total_instances]
> + %td{:class => 'center'}= pool_stats[:instances_pending]
> + %td{:class => 'center'}= pool_stats[:instances_failed_count]
> + %td{:class => 'center'}= pool_stats[:quota_percent]
> - if check_privilege(Privilege::VIEW, pool.pool_family)
> %td= link_to(pool.pool_family.name, pool.pool_family)
> - else
The speedup on my side was much less pronounced (~150ms) but we should
not be calling the database like that, so ACK.
There's a couple of notes inline, feel free to disregard them if you
disagree.
More information about the aeolus-devel
mailing list