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.

Sign up for GitHub

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

Memoize cols_on_metric_rollup in chargeback #17927

Merged
merged 2 commits into from
Sep 10, 2018
Merged

Memoize cols_on_metric_rollup in chargeback #17927

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf2b318
Memoize methods in chargeback to reduce some queires
lpichler Jun 14, 2018
131cf41
Ensure uniqueness in chargeable_cols_on_metric_rollup in ChargeableField
lpichler Jun 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions app/models/chargeable_field.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,9 @@ def rate_name
end

def self.cols_on_metric_rollup
(%w(id tag_names resource_id) + chargeable_cols_on_metric_rollup).uniq
@cols_on_metric_rollup ||= begin
(%w(id tag_names resource_id) + chargeable_cols_on_metric_rollup).uniq
end
end

def self.col_index(column)
Expand Down Expand Up @@ -136,8 +138,10 @@ def self.seed_data
end

def self.chargeable_cols_on_metric_rollup
existing_cols = MetricRollup.attribute_names
chargeable_cols = pluck(:metric) & existing_cols
chargeable_cols.map! { |x| VIRTUAL_COL_USES[x] || x }.sort
@chargeable_cols_on_metric_rollup ||= begin
existing_cols = MetricRollup.attribute_names
chargeable_cols = pluck(:metric) & existing_cols
Copy link
Member

Choose a reason for hiding this comment

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

Could new records be created with new data in the metric column that would never be seen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes but not during running application. It could be added only by adding record (this happens very rarely) to chargeable_field.yml and based on the yaml, table chargeable_field table is filled up in the seed process.

thanks

Copy link
Member

Choose a reason for hiding this comment

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

@libor, I think this line may be causing the sporadic test failures. If this code is called before ChargeableField is seeded it will memoize an empty array. The other tests will use the memoized value and potentially get the nil error. I found that consumption_with_rollups_spec.rb calls this code in a bunch of places without seeding ChargeableField. The fix may be to just make sure the class is seeded first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes! and finally I probably found a culprit. ./spec/models/chargeback/consumption_with_rollups_spec.rb but I am not sure it if it is the only culprit.

thanks

chargeable_cols.map! { |x| VIRTUAL_COL_USES[x] || x }.sort.uniq
end
end
end