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

Jump to bottom

[WIP] Add cache columns to operating_systems table #322

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[WIP] Add cache columns to operating_systems table #322

wants to merge 1 commit into from

Conversation

NickLaMuro
Copy link
Member

Both platform and image_name are relatively stable values that get re-calculated every time they are accessed, and in the worst case, causes it to traverse through many different sources to try and find a proper platform/image_name. This is compounded when you are trying to fetch thousands of records at once.

This patch adds the columns to be cache store for those values, and should be triggered on every update of those records, and related records (not addressed in this patch).

Links to associated manageiq implementation and BZ related PRs to come, but those are what will provide more context to the advantages to this change.

Considerations

  • Is this the best way to handle this problem?
  • Are these columns going to add much size to the DB?
  • Is there a better size for some of these columns?
  • Should these be enum cols?
  • Is there a way these could become out of sync? Does that matter?
  • Is there a faster way to run this migration? (example run below)

Example Output

This was done with a DB that had a single Amazon EMS with public images that have been included in a refresh:

$ bin/rake db:migrate
** Using session_store: ActionDispatch::Session::MemCacheStore
** ManageIQ master, codename: Ivanchuk
== 20190108031411 AddImageNameToOperatingSystems: migrating ===================
-- add_column(:operating_systems, :platform, :string)
   -> 0.0073s
-- add_column(:operating_systems, :image_name, :string)
   -> 0.0004s
-- Updating platform and image_name in OperatingSystems
   -> Processing 121475 rows
   -> 1000 rows (0.82% - 1000 total - 2.45s - ETA: 5 minutes)
   -> 1000 rows (1.65% - 2000 total - 2.39s - ETA: 5 minutes)
   -> 1000 rows (2.47% - 3000 total - 2.29s - ETA: 5 minutes)
   -> 1000 rows (3.29% - 4000 total - 2.39s - ETA: 5 minutes)
   -> 1000 rows (4.12% - 5000 total - 2.27s - ETA: 5 minutes)
   -> 1000 rows (4.94% - 6000 total - 2.44s - ETA: 5 minutes)
   -> 1000 rows (5.76% - 7000 total - 2.25s - ETA: 4 minutes)
   -> 1000 rows (6.59% - 8000 total - 2.30s - ETA: 4 minutes)
   -> 1000 rows (7.41% - 9000 total - 2.40s - ETA: 4 minutes)
   -> 1000 rows (8.23% - 10000 total - 2.32s - ETA: 4 minutes)
   -> 1000 rows (9.06% - 11000 total - 2.29s - ETA: 4 minutes)
   -> 1000 rows (9.88% - 12000 total - 2.42s - ETA: 4 minutes)
   -> 1000 rows (10.70% - 13000 total - 2.24s - ETA: 4 minutes)
   -> 1000 rows (11.53% - 14000 total - 2.29s - ETA: 4 minutes)
   -> 1000 rows (12.35% - 15000 total - 2.44s - ETA: 4 minutes)
   -> 1000 rows (13.17% - 16000 total - 2.30s - ETA: 4 minutes)
   -> 1000 rows (13.99% - 17000 total - 2.38s - ETA: 4 minutes)
   -> 1000 rows (14.82% - 18000 total - 2.75s - ETA: 4 minutes)
   -> 1000 rows (15.64% - 19000 total - 2.29s - ETA: 4 minutes)
   -> 1000 rows (16.46% - 20000 total - 2.29s - ETA: 4 minutes)
   -> 1000 rows (17.29% - 21000 total - 2.41s - ETA: 4 minutes)
   -> 1000 rows (18.11% - 22000 total - 2.31s - ETA: 4 minutes)
   -> 1000 rows (18.93% - 23000 total - 2.27s - ETA: 4 minutes)
   -> 1000 rows (19.76% - 24000 total - 2.52s - ETA: 4 minutes)
   -> 1000 rows (20.58% - 25000 total - 2.29s - ETA: 4 minutes)
   -> 1000 rows (21.40% - 26000 total - 2.29s - ETA: 4 minutes)
   -> 1000 rows (22.23% - 27000 total - 2.39s - ETA: 4 minutes)
   -> 1000 rows (23.05% - 28000 total - 2.26s - ETA: 4 minutes)
   -> 1000 rows (23.87% - 29000 total - 2.27s - ETA: 4 minutes)
   -> 1000 rows (24.70% - 30000 total - 2.50s - ETA: 4 minutes)
   -> 1000 rows (25.52% - 31000 total - 2.28s - ETA: 4 minutes)
   -> 1000 rows (26.34% - 32000 total - 2.28s - ETA: 4 minutes)
   -> 1000 rows (27.17% - 33000 total - 2.43s - ETA: 3 minutes)
   -> 1000 rows (27.99% - 34000 total - 2.29s - ETA: 3 minutes)
   -> 1000 rows (28.81% - 35000 total - 2.33s - ETA: 3 minutes)
   -> 1000 rows (29.64% - 36000 total - 2.48s - ETA: 3 minutes)
   -> 1000 rows (30.46% - 37000 total - 2.26s - ETA: 3 minutes)
   -> 1000 rows (31.28% - 38000 total - 2.54s - ETA: 3 minutes)
   -> 1000 rows (32.11% - 39000 total - 2.71s - ETA: 3 minutes)
   -> 1000 rows (32.93% - 40000 total - 2.26s - ETA: 3 minutes)
   -> 1000 rows (33.75% - 41000 total - 2.31s - ETA: 3 minutes)
   -> 1000 rows (34.58% - 42000 total - 2.53s - ETA: 3 minutes)
   -> 1000 rows (35.40% - 43000 total - 2.29s - ETA: 3 minutes)
   -> 1000 rows (36.22% - 44000 total - 2.34s - ETA: 3 minutes)
   -> 1000 rows (37.04% - 45000 total - 2.42s - ETA: 3 minutes)
   -> 1000 rows (37.87% - 46000 total - 2.30s - ETA: 3 minutes)
   -> 1000 rows (38.69% - 47000 total - 2.31s - ETA: 3 minutes)
   -> 1000 rows (39.51% - 48000 total - 2.44s - ETA: 3 minutes)
   -> 1000 rows (40.34% - 49000 total - 2.43s - ETA: 3 minutes)
   -> 1000 rows (41.16% - 50000 total - 2.98s - ETA: 3 minutes)
   -> 1000 rows (41.98% - 51000 total - 2.95s - ETA: 3 minutes)
   -> 1000 rows (42.81% - 52000 total - 2.66s - ETA: 3 minutes)
   -> 1000 rows (43.63% - 53000 total - 2.83s - ETA: 3 minutes)
   -> 1000 rows (44.45% - 54000 total - 2.68s - ETA: 3 minutes)
   -> 1000 rows (45.28% - 55000 total - 2.47s - ETA: 3 minutes)
   -> 1000 rows (46.10% - 56000 total - 2.43s - ETA: 3 minutes)
   -> 1000 rows (46.92% - 57000 total - 2.68s - ETA: 3 minutes)
   -> 1000 rows (47.75% - 58000 total - 2.57s - ETA: 3 minutes)
   -> 1000 rows (48.57% - 59000 total - 2.45s - ETA: 3 minutes)
   -> 1000 rows (49.39% - 60000 total - 2.59s - ETA: 2 minutes)
   -> 1000 rows (50.22% - 61000 total - 2.51s - ETA: 2 minutes)
   -> 1000 rows (51.04% - 62000 total - 2.50s - ETA: 2 minutes)
   -> 1000 rows (51.86% - 63000 total - 2.66s - ETA: 2 minutes)
   -> 1000 rows (52.69% - 64000 total - 2.51s - ETA: 2 minutes)
   -> 1000 rows (53.51% - 65000 total - 2.50s - ETA: 2 minutes)
   -> 1000 rows (54.33% - 66000 total - 2.69s - ETA: 2 minutes)
   -> 1000 rows (55.16% - 67000 total - 2.53s - ETA: 2 minutes)
   -> 1000 rows (55.98% - 68000 total - 2.45s - ETA: 2 minutes)
   -> 1000 rows (56.80% - 69000 total - 2.59s - ETA: 2 minutes)
   -> 1000 rows (57.63% - 70000 total - 2.42s - ETA: 2 minutes)
   -> 1000 rows (58.45% - 71000 total - 2.47s - ETA: 2 minutes)
   -> 1000 rows (59.27% - 72000 total - 2.57s - ETA: 2 minutes)
   -> 1000 rows (60.09% - 73000 total - 2.34s - ETA: 2 minutes)
   -> 1000 rows (60.92% - 74000 total - 2.47s - ETA: 2 minutes)
   -> 1000 rows (61.74% - 75000 total - 2.71s - ETA: 2 minutes)
   -> 1000 rows (62.56% - 76000 total - 2.48s - ETA: 2 minutes)
   -> 1000 rows (63.39% - 77000 total - 2.58s - ETA: 2 minutes)
   -> 1000 rows (64.21% - 78000 total - 2.96s - ETA: 2 minutes)
   -> 1000 rows (65.03% - 79000 total - 3.22s - ETA: 2 minutes)
   -> 1000 rows (65.86% - 80000 total - 2.72s - ETA: 2 minutes)
   -> 1000 rows (66.68% - 81000 total - 2.59s - ETA: 2 minutes)
   -> 1000 rows (67.50% - 82000 total - 2.48s - ETA: 2 minutes)
   -> 1000 rows (68.33% - 83000 total - 2.46s - ETA: 2 minutes)
   -> 1000 rows (69.15% - 84000 total - 2.56s - ETA: 2 minutes)
   -> 1000 rows (69.97% - 85000 total - 2.40s - ETA: 1 minute)
   -> 1000 rows (70.80% - 86000 total - 2.44s - ETA: 1 minute)
   -> 1000 rows (71.62% - 87000 total - 2.59s - ETA: 1 minute)
   -> 1000 rows (72.44% - 88000 total - 2.46s - ETA: 1 minute)
   -> 1000 rows (73.27% - 89000 total - 2.52s - ETA: 1 minute)
   -> 1000 rows (74.09% - 90000 total - 2.55s - ETA: 1 minute)
   -> 1000 rows (74.91% - 91000 total - 2.43s - ETA: 1 minute)
   -> 1000 rows (75.74% - 92000 total - 2.46s - ETA: 1 minute)
   -> 1000 rows (76.56% - 93000 total - 2.60s - ETA: 1 minute)
   -> 1000 rows (77.38% - 94000 total - 2.41s - ETA: 1 minute)
   -> 1000 rows (78.21% - 95000 total - 2.45s - ETA: 1 minute)
   -> 1000 rows (79.03% - 96000 total - 2.60s - ETA: 1 minute)
   -> 1000 rows (79.85% - 97000 total - 2.60s - ETA: 1 minute)
   -> 1000 rows (80.68% - 98000 total - 2.58s - ETA: less than a minute)
   -> 1000 rows (81.50% - 99000 total - 2.63s - ETA: less than a minute)
   -> 1000 rows (82.32% - 100000 total - 2.53s - ETA: less than a minute)
   -> 1000 rows (83.14% - 101000 total - 2.62s - ETA: less than a minute)
   -> 1000 rows (83.97% - 102000 total - 2.68s - ETA: less than a minute)
   -> 1000 rows (84.79% - 103000 total - 2.59s - ETA: less than a minute)
   -> 1000 rows (85.61% - 104000 total - 2.52s - ETA: less than a minute)
   -> 1000 rows (86.44% - 105000 total - 2.44s - ETA: less than a minute)
   -> 1000 rows (87.26% - 106000 total - 2.48s - ETA: half a minute)
   -> 1000 rows (88.08% - 107000 total - 2.49s - ETA: half a minute)
   -> 1000 rows (88.91% - 108000 total - 2.43s - ETA: half a minute)
   -> 1000 rows (89.73% - 109000 total - 2.28s - ETA: half a minute)
   -> 1000 rows (90.55% - 110000 total - 2.31s - ETA: half a minute)
   -> 1000 rows (91.38% - 111000 total - 2.46s - ETA: half a minute)
   -> 1000 rows (92.20% - 112000 total - 2.28s - ETA: half a minute)
   -> 1000 rows (93.02% - 113000 total - 2.37s - ETA: half a minute)
   -> 1000 rows (93.85% - 114000 total - 2.46s - ETA: less than 20 seconds)
   -> 1000 rows (94.67% - 115000 total - 2.39s - ETA: less than 20 seconds)
   -> 1000 rows (95.49% - 116000 total - 2.38s - ETA: less than 20 seconds)
   -> 1000 rows (96.32% - 117000 total - 2.59s - ETA: less than 20 seconds)
   -> 1000 rows (97.14% - 118000 total - 2.45s - ETA: less than 10 seconds)
   -> 1000 rows (97.96% - 119000 total - 2.47s - ETA: less than 10 seconds)
   -> 1000 rows (98.79% - 120000 total - 2.63s - ETA: less than 5 seconds)
   -> 1000 rows (99.61% - 121000 total - 2.45s - ETA: less than 5 seconds)
   -> 475 rows (100.00% - 121475 total - 1.22s - ETA: less than 5 seconds)
   -> 300.5599s
== 20190108031411 AddImageNameToOperatingSystems: migrated (300.5679s) ========

@miq-bot miq-bot added the wip label Jan 11, 2019
This was referenced Jan 11, 2019
Closed
Merged
@NickLaMuro NickLaMuro force-pushed the add_platform_cache_to_operating_system branch 2 times, most recently from 67cb826 to fb9ac4b Compare January 14, 2019 23:59
@NickLaMuro
Copy link
Member Author

Regarding the remaining CodeClimate/rubocop issues:

  • I think the case looks better with what I have, but am not tied to it.
  • The redundant self "might" be required, but I will have to mess around with it. I was having trouble getting something to work with the migration, and that might have been it. I will test later and re-affirm
  • The issues with CodeClimate are related to code that I ripped off from the main app, and should be refactored there, not here. There were a few issues that I needed to address to make these methods work, and will make sure to address them in some comments (TODO).

@NickLaMuro NickLaMuro mentioned this pull request Jan 16, 2019
Closed
kbrock
kbrock reviewed May 29, 2019
View reviewed changes
Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

Little screwed up by the active record N+1 to save

But all in all, this looks good.

I Realize these methods are probably the same as methods somewhere else - I'll find those in your PR and make a comment elsewhere

db/migrate/20190108031411_add_image_name_to_operating_systems.rb
return a[0] unless findStr.index(n).nil?
end
end
"unknown"
Copy link
Member

Choose a reason for hiding this comment

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

does it make sense to not return "unknown"?

If you want unknown, you could use a || "unknown" (or pass in a boolean for default_value or allow_unknown)

Copy link
Member Author

Choose a reason for hiding this comment

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

@kbrock This is a complete clone of what exists in OperatingSystem already:

https://github.com/ManageIQ/manageiq/blob/184bb9d3864d84b91903bc0003a9d77362f5863c/app/models/operating_system.rb#L73-L81

db/migrate/20190108031411_add_image_name_to_operating_systems.rb Outdated
os = obj.operating_system
if os
# check the given field names for possible matching value
osName = [:distribution, :product_type, :product_name].each do |field| # rubocop:disable Style/SymbolArray
Copy link
Member

Choose a reason for hiding this comment

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

if "unknown" isn't in play, maybe you could just use detect?

Suggested change
osName = [:distribution, :product_type, :product_name].each do |field| # rubocop:disable Style/SymbolArray
osName = [:distribution, :product_type, :product_name].detect do |field|
os_field = os.send(field)
os_field if normalize_os_name(os_field)
end

Copy link
Member Author

Choose a reason for hiding this comment

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

Same here:

https://github.com/ManageIQ/manageiq/blob/184bb9d3864d84b91903bc0003a9d77362f5863c/app/models/operating_system.rb#L83

db/migrate/20190108031411_add_image_name_to_operating_systems.rb
end

# If the OS Name is still blank check the 'user_assigned_os'
if osName.nil? && obj.respond_to?(:user_assigned_os) && obj.user_assigned_os
Copy link
Member

Choose a reason for hiding this comment

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

This is probably a naive suggestion but...

Suggested change
if osName.nil? && obj.respond_to?(:user_assigned_os) && obj.user_assigned_os
osName ||= obj.try(:user_assigned_os)

Copy link
Member Author

Choose a reason for hiding this comment

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

Still same...

https://github.com/ManageIQ/manageiq/blob/184bb9d3864d84b91903bc0003a9d77362f5863c/app/models/operating_system.rb#L83

db/migrate/20190108031411_add_image_name_to_operating_systems.rb
say_batch_started(base_relation.size)

base_relation.find_in_batches do |group|
group.each(&:save)
Copy link
Member

Choose a reason for hiding this comment

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

love the simplicity here

db/migrate/20190108031411_add_image_name_to_operating_systems.rb Outdated
before_save :update_platform_and_image_name

def update_platform_and_image_name
obj = case
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
obj = case
obj = host || vm_or_template || computer_system || StubOperatingSystemObj.new(self)

Copy link
Member Author

Choose a reason for hiding this comment

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

But is that under 80 characters?

Copy link
Member

Choose a reason for hiding this comment

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

close? 84 (not counting the indentation)

kbrock
kbrock reviewed May 29, 2019
View reviewed changes
db/migrate/20190108031411_add_image_name_to_operating_systems.rb
end

# If the normalized name comes back as unknown, nil out the value so we can get it from another field
if osName.kind_of?(String)
Copy link
Member

Choose a reason for hiding this comment

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

(still selling my idea) I don't think this is necessary if "unknown" wouldn't come back :)

Copy link
Member Author

@NickLaMuro NickLaMuro May 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Still "selling my idea" of "I copy pasted of what is in the existing OperatingSystem model" and not trying to refactor that in this PR. Just trying to replicate what would be done when saving normally and trying to calculate the value as it would be generated today.

I think the bigger question to ask is not "can this code be done better", but does the idea of "caching this value" make sense. Hence the [WIP] and [POC] labels, since I really want a this to be agreed on before moving forward.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, see "Considerations" section in the PR description... which I even forgot about...

djberg96
djberg96 reviewed Aug 26, 2019
View reviewed changes
db/migrate/20190108031411_add_image_name_to_operating_systems.rb
end

@@os_map = [ # rubocop:disable Style/ClassVars
["windows_generic", %w(winnetenterprise w2k3 win2k3 server2003 winnetstandard servernt)],
Copy link
Contributor

Choose a reason for hiding this comment

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

Is winnetenterprise right? I'm guessing it's winntenterprise, but thought I'd double check.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just copied-pased this from the class I think, but let me double check.

Copy link
Member Author

Choose a reason for hiding this comment

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

@djberg96 I did:

https://github.com/ManageIQ/manageiq/blob/184bb9d3864d84b91903bc0003a9d77362f5863c/app/models/operating_system.rb#L14

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that's correct: https://communities.vmware.com/thread/42204

@djberg96
Copy link
Contributor

I agree with @NickLaMuro that we should stick with "unknown" since that's the current behavior.

@djberg96
Copy link
Contributor

Side note, I'm having difficulty telling what codeclimate is upset about. There's no analysis there atm.

@NickLaMuro
Copy link
Member Author

Side note, I'm having difficulty telling what codeclimate is upset about. There's no analysis there atm.

@djberg96 probably long methods that I am replicating from the OperatingSystem class in core. Most likely the build is too old that it doesn't exist any more. I could rebase and see if it triggers another build if you want. Probably should update the migration timestamp anyway.

@djberg96
Copy link
Contributor

@NickLaMuro Ok, try rebasing and see if it triggers another build. I tried to request one but it didn't seem to actually do anything.

kbrock
kbrock approved these changes Aug 26, 2019
View reviewed changes
Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

LGTM

@djberg96
Copy link
Contributor

👍

@kbrock
Copy link
Member

kbrock commented Dec 4, 2019

yea, I'm still 👍 on this one

@Fryguy
Copy link
Member

Fryguy commented Dec 4, 2019

cc @agrare moreso on the processing side of this column.

This patch adds the columns to be cache store for those values, and should be triggered on every update of those records, and related records (not addressed in this patch).

Thoughts? That is, should we do it at collector time, persister time, or even lower at model save time? I like the idea of caching the value and not calculating over and over (better for reporting too), but I'm not sure where/when is the best place to do it?

@NickLaMuro NickLaMuro force-pushed the add_platform_cache_to_operating_system branch from fb9ac4b to 1238c6a Compare December 4, 2019 18:04
@NickLaMuro
Copy link
Member Author

Welp, looks like this is being looked at now again. I rebased since I said I was going to do that 4 months ago... oops...

@NickLaMuro
Copy link
Member Author

Son of a bisquick rubocop... brb...

@NickLaMuro NickLaMuro force-pushed the add_platform_cache_to_operating_system branch from 1238c6a to a84a87a Compare December 4, 2019 18:15
@NickLaMuro
Copy link
Member Author

Thoughts? That is, should we do it at collector time, persister time, or even lower at model save time? I like the idea of caching the value and not calculating over and over (better for reporting too), but I'm not sure where/when is the best place to do it?

@Fryguy @agrare For what it is worth, I "started" doing some of that here:

ManageIQ/manageiq#18354

But I agree, some decisions should be made before we move forward with that one.

@NickLaMuro NickLaMuro force-pushed the add_platform_cache_to_operating_system branch from a84a87a to 1081ffe Compare December 4, 2019 18:23
@NickLaMuro
Add cache columns to operating_systems table
1081ffe 
Both platform and image_name are relatively stable values that get
re-calculated every time they are accessed, and in the worst case,
causes it to traverse through many different sources to try and find a
proper `platform`/`image_name`.  This is compounded when you are trying
to fetch thousands of records at once.

This adds the columns to be cache store for those values, and should be
triggered on every update of those records, and related records (not
addressed in this patch).
@miq-bot
Copy link
Member

miq-bot commented Dec 4, 2019

Checked commit NickLaMuro@1081ffe with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

@kbrock
Copy link
Member

kbrock commented Jan 10, 2020

:shipit: ?

@miq-bot miq-bot added the stale label Jun 11, 2020
@miq-bot
Copy link
Member

miq-bot commented Jun 11, 2020

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s)

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the traige process documentation.

@NickLaMuro NickLaMuro mentioned this pull request Jun 25, 2020
Closed
@gtanzillo
Copy link
Member

@NickLaMuro What do you want to do with this one?

@gtanzillo gtanzillo removed the stale label Jul 20, 2020
@NickLaMuro
Copy link
Member Author

@gtanzillo I covered a lot of my rational in the related PR here:

ManageIQ/manageiq-automation_engine#446 (comment)

And really, I am waiting on approval of this approach, or if this is something we instead don't want to support. The BZ that was originally associated with this fix became less relevant since AWS was being de-prioritized at the time, but if that has changed then perhaps this should be re-visited and re-reviewed.

@miq-bot miq-bot added the stale label Mar 6, 2023
@miq-bot
Copy link
Member

miq-bot commented Mar 6, 2023

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s)

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

@kbrock kbrock removed the stale label Apr 12, 2023
@kbrock
Copy link
Member

kbrock commented Apr 12, 2023

@Fryguy @agrare Do we want to cache the OS value (since it is basically static?)

This has been hanging out for 4 years. would be nice to give a yea/nay and be done with it

@Fryguy
Copy link
Member

Fryguy commented Apr 12, 2023

My thoughts were here and are the same #322 (comment)

@Fryguy
Copy link
Member

Fryguy commented Apr 12, 2023

FWIW, I think this should be done at model save time (calculate during record save and update if necessary) and I'm ok with caching these as columns, but really want @agrare 's opinion.

@agrare
Copy link
Member

agrare commented Apr 12, 2023

Saving this to the database instead of calculating it every time at display time makes a lot of sense.

This sounds a lot like raw_power_state / power_state where raw_power_state= sets the normalized power_state based on a provider specific lookup table. We could implement this the same way, but any model-level setters like this mean we can't use concurrent-safe batch saving (not a real issue right now since we can't use this for VMs/Hosts anyway for other reasons).

@miq-bot miq-bot added the stale label Jul 17, 2023
@miq-bot
Copy link
Member

miq-bot commented Jul 17, 2023

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s)

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

@miq-bot miq-bot closed this Oct 23, 2023
@miq-bot
Copy link
Member

miq-bot commented Oct 23, 2023

This pull request has been automatically closed because it has not been updated for at least 3 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

@kbrock kbrock removed the stale label Oct 23, 2023
@kbrock kbrock reopened this Oct 23, 2023
@miq-bot miq-bot added the stale label Jan 29, 2024
@miq-bot
Copy link
Member

miq-bot commented Jan 29, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

3 similar comments
@miq-bot
Copy link
Member

miq-bot commented May 13, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

@miq-bot
Copy link
Member

miq-bot commented Aug 19, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

@miq-bot
Copy link
Member

miq-bot commented Nov 25, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djberg96 djberg96 djberg96 left review comments

@kbrock kbrock kbrock approved these changes

Assignees
No one assigned
Labels
performance stale wip
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@NickLaMuro @djberg96 @kbrock @Fryguy @miq-bot @gtanzillo @agrare @mfeifer