Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes #14076 - remove unused Puppetclass hostgroups counter #3285

Closed
wants to merge 1 commit into from

Conversation

domcleal
Copy link
Contributor

@domcleal domcleal commented Mar 7, 2016

Replaces #3282.

def down
add_column :puppetclasses, :hostgroups_count, :integer, :default => 0
HostgroupClass.all.each do |hgc|
Puppetclass.increment_counter(:hostgroups_count, hgc.puppetclass_id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it's worth the trouble for a down migration, but we can calculate this entirely in the DB with a single SQL query.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A query to group HostgroupClass by puppetclass_id and then update each puppetclass_id's counter?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something along the lines of:

UPDATE puppetclasses SET hostgroups_count = (SELECT count(*) FROM hostgroup_classes WHERE hostgroup_classes.class_id = puppetclasses.id)

Technically a nested query, but I think most DBs should be able to optimize it quite well. And without instantiating possibly thousands of objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, updated.

@tbrisker
Copy link
Member

tbrisker commented Mar 7, 2016

This also needs fixing on this line:
https://github.com/theforeman/foreman/blob/develop/lib/tasks/fix_cached_counters.rake#L13
Other than that, LGTM.

@domcleal
Copy link
Contributor Author

domcleal commented Mar 7, 2016

Thanks, fixed the rake task to not update this counter.

@tbrisker
Copy link
Member

tbrisker commented Mar 8, 2016

Merged as be6e6ef, thanks @domcleal!

@tbrisker tbrisker closed this Mar 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants