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

Extract rates cache to separate class #12608

Merged
merged 2 commits into from
Nov 26, 2016

Conversation

isimluk
Copy link
Member

@isimluk isimluk commented Nov 14, 2016

Previously, the Chargeback class and its descendants managed relevant ChargebackRates. The Chargeback class suffers from feature-creep, responsibility-creep. In regards to rates, it caches them, defines assignments and selects rate to define interval.

And we have more features coming (1) see #12534, (2) dynamic rates for dynamic disk types. And there is more. In future, chargeback plans for more intelligent Chargeback rate selection and caching. I have heard of rates priorities, etc. That all is better achieved in separate class. Personally, I want the cache to drop the rates that charge for 0 in the future (that will improve performance of the cb calculation itself).

@miq-bot add_label chargeback, refactoring, euwe/no
@miq-bot assign @gtanzillo

This will help us to dispart Chargeback calculations and rates cache.
This will enable us to create more advanced rate selection processes.

This will also enable us to cache all the rates in advance
@miq-bot
Copy link
Member

miq-bot commented Nov 15, 2016

Checked commits isimluk/manageiq@07cd0ff~...72e3dfc with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
8 files checked, 0 offenses detected
Everything looks good. 🏆

@rates ||= {}
@rates[perf.hash_features_affecting_rate] ||=
begin
prefix = tag_prefix(perf)
Copy link
Contributor

@lpichler lpichler Nov 15, 2016

Choose a reason for hiding this comment

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

@isimluk is this same as

  prefix = Chargeback.report_cb_model(self.class.name).underscore

?

We was deriving prefix from chargeback class, (basically is report for VM, Projects or Contr. ) now you are doing it from perf record.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is the same. 👍

Each Chargeback.descendant has where_clause method. This where_class limits the resource_type of perf records coming here.

@lpichler
Copy link
Contributor

LGTM 👍

@martinpovolny martinpovolny merged commit 8b53948 into ManageIQ:master Nov 26, 2016
@martinpovolny martinpovolny added this to the Sprint 50 Ending Dec 5, 2016 milestone Nov 26, 2016
@gtanzillo
Copy link
Member

Nice! 👍

@zeari
Copy link

zeari commented Dec 8, 2016

@miq-bot add_label euwe/yes

@zeari
Copy link

zeari commented Dec 8, 2016

@miq-bot remove_label euwe/no

@miq-bot miq-bot removed the euwe/no label Dec 8, 2016
@simaishi
Copy link
Contributor

simaishi commented Jan 6, 2017

@isimluk is there a BZ for this?

@isimluk
Copy link
Member Author

isimluk commented Jan 11, 2017

No bz for this. This cannot be backported alone, this pr has dependencies. I created #13419 to get this backported.

@simaishi
Copy link
Contributor

Backported to Euwe via #13419

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants