Replace remove duplicate timestamp by sql version for chargeback #17538
Conversation
- rewrite remove_duplicate_timestamps to SQL - rewrite also "group by tenant" feature - reduce returned object from BIG base_rollup query
lpichler
force-pushed
the
replace_remove_duplicate_timestamp_by_sql_version
branch
from
c7ddebd
to
a518a17
Compare
|
Checked commit lpichler@a518a17 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
lpichler
changed the title
| options.group_with(records).each_value do |metric_rollup_records| | ||
|
|
||
| records.each_value do |rollup_record_ids| | ||
| metric_rollup_records = base_rollup.where(:id => rollup_record_ids).to_a |
There was a problem hiding this comment.
you're introducing an N+1 here. Is this really what you want?
|
|
||
| def self.uniq_timestamp_record_map(report_scope, group_by_tenant = false) | ||
| main_select = MetricRollup.select(:id, :resource_id).arel.ast.to_sql | ||
| .gsub("SELECT", "DISTINCT ON (resource_type, resource_id, timestamp)") |
There was a problem hiding this comment.
have you tried
.distinct(:resource_type, :resource_id, :timestamp)
I'd almost prefer raw sql over sql generation and munging.
|
@miq-bot add_label blocker |
|
@lpichler if this can be backported, please add the gaprindashvili/yes label |
|
@kbrock thanks for now, I'll add big picture in GH issue , briefly - reduce memory in chargeback, this is first part of 4. |
| records = cb_class.where_clause(records, options, region) | ||
| records = Metric::Helper.remove_duplicate_timestamps(records) |
There was a problem hiding this comment.
It seems like this will result in a change in behavior, maybe it's ok but I want to acknowledge it here - the original code in remove_duplicate_timestamps merges all metric rows it finds with the same timestamp - https://github.com/manageiq/manageiq/blob/b6bd309650ee46fedcdf6d9e00af4f06685bd7f8/app/models/metric/helper.rb#L139. This code will keep one of them and discard the other.
|
@lpichler please review feedback here, thanks! |
|
@jrafanie yes and I probably found solution also for @gtanzillo's finding in #17538 (review) comment. And also I am putting together some stats. |
|
@miq-bot add_label gaprindashvili/yes |
…stamp_by_sql_version Replace remove duplicate timestamp by sql version for chargeback (cherry picked from commit 887cc81) https://bugzilla.redhat.com/show_bug.cgi?id=1591939
|
Gaprindashvili backport details: |
This is first part of decreasing memory in chargeback. I think that there is not too much reducing memory but this change is absolutely important for next step in other PRs in Links section.
This is adding possibility to avoid array of AR objects and we go with query.
This code is written by @NickLaMuro (almost). Many thanks! it helped me a lot in order do further changes with signification memory reduction.
Links
https://bugzilla.redhat.com/show_bug.cgi?id=1566452
#17538 - first part
#17552 - second part
#17558 - third part - Can be merged before first and second part
#17560 - last part
cc @NickLaMuro @kbrock
@miq-bot assign @gtanzillo