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

switch parent to ancestry (miq_ae_namespaces) #7249

Merged
merged 1 commit into from
Aug 17, 2020
Merged

switch parent to ancestry (miq_ae_namespaces) #7249

merged 1 commit into from
Aug 17, 2020

Conversation

ahrechushkin
Copy link
Contributor

@ahrechushkin ahrechushkin commented Aug 7, 2020

Actually we used ancestry instead of parent_id

[----] F, [2020-08-07T08:53:02.418285 #9:2aecc843f884] FATAL -- : Error caught: [ActiveRecord::StatementInvalid] PG::UndefinedColumn: ERROR:  column miq_ae_namespaces.parent does not exist
LINE 1: ...spaces" WHERE "miq_ae_namespaces"."name" = $1 AND "miq_ae_na...
                                                             ^
: SELECT  "miq_ae_namespaces".* FROM "miq_ae_namespaces" WHERE "miq_ae_namespaces"."name" = $1 AND "miq_ae_namespaces"."parent" IS NULL LIMIT $2
/usr/share/gems/gems/activerecord-5.1.7/lib/active_record/connection_adapters/postgresql_adapter.rb:624:in `exec_params'
/usr/share/gems/gems/activerecord-5.1.7/lib/active_record/connection_adapters/postgresql_adapter.rb:624:in `block (2 levels) in exec_no_cache'
/usr/share/gems/gems/activesupport-5.1.7/lib/active_support/dependencies/interlock.rb:46:in `block in permit_concurrent_loads'
/usr/share/gems/gems/activesupport-5.1.7/lib/active_support/concurrency/share_lock.rb:185:in `yield_shares'
/usr/share/gems/gems/activesupport-5.1.7/lib/active_support/dependencies/interlock.rb:45:in `permit_concurrent_loads'
/usr/share/gems/gems/activerecord-5.1.7/lib/active_record/connection_adapters/postgresql_adapter.rb:623:in `block in exec_no_cache'
/usr/share/gems/gems/activerecord-5.1.7/lib/active_record/connection_adapters/abstract_adapter.rb:613:in `block (2 levels) in log'
/usr/share/ruby/monitor.rb:230:in `mon_synchronize'

ManageIQ/manageiq-schema#455

Actually we used ancestry instead of parent_id
```
[----] F, [2020-08-07T08:53:02.418285 #9:2aecc843f884] FATAL -- : Error caught: [ActiveRecord::StatementInvalid] PG::UndefinedColumn: ERROR:  column miq_ae_namespaces.parent does not exist
LINE 1: ...spaces" WHERE "miq_ae_namespaces"."name" = $1 AND "miq_ae_na...
                                                             ^
: SELECT  "miq_ae_namespaces".* FROM "miq_ae_namespaces" WHERE "miq_ae_namespaces"."name" = $1 AND "miq_ae_namespaces"."parent" IS NULL LIMIT $2
/usr/share/gems/gems/activerecord-5.1.7/lib/active_record/connection_adapters/postgresql_adapter.rb:624:in `exec_params'
/usr/share/gems/gems/activerecord-5.1.7/lib/active_record/connection_adapters/postgresql_adapter.rb:624:in `block (2 levels) in exec_no_cache'
/usr/share/gems/gems/activesupport-5.1.7/lib/active_support/dependencies/interlock.rb:46:in `block in permit_concurrent_loads'
/usr/share/gems/gems/activesupport-5.1.7/lib/active_support/concurrency/share_lock.rb:185:in `yield_shares'
/usr/share/gems/gems/activesupport-5.1.7/lib/active_support/dependencies/interlock.rb:45:in `permit_concurrent_loads'
/usr/share/gems/gems/activerecord-5.1.7/lib/active_record/connection_adapters/postgresql_adapter.rb:623:in `block in exec_no_cache'
/usr/share/gems/gems/activerecord-5.1.7/lib/active_record/connection_adapters/abstract_adapter.rb:613:in `block (2 levels) in log'
/usr/share/ruby/monitor.rb:230:in `mon_synchronize'
````
ManageIQ/manageiq-schema#455
@miq-bot
Copy link
Member

miq-bot commented Aug 7, 2020

Checked commit https://github.com/ahrechushkin/manageiq-ui-classic/commit/a598af7e061450257d030e89555af943526f364f with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 🍰

@ahrechushkin
Copy link
Contributor Author

@skateman Hello, can you check?

@chessbyte
Copy link
Member

@Fryguy @lfu seems like this one was missed in the changes around ManageIQ/manageiq-schema#455

@skateman
Copy link
Member

skateman commented Aug 7, 2020

@kbrock need your 👀

@@ -56,7 +56,7 @@ def automate_find_hierarchy(fqname)
begin
# Collect all the db records based on the parsed fqname
open_nodes = namespace.split('/').each_with_object([]) do |ns, items|
items << MiqAeNamespace.find_by!(:name => ns, :parent => items.last)
Copy link
Member

Choose a reason for hiding this comment

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

If items.last is a namespace object, this is the correct way to specify a parent.

:ancestry is column that stores all path ids from the root to the parent for every node, in the format like "100018/100019".

What are the steps to re-create the issue? Thanks.

Copy link
Contributor Author

@ahrechushkin ahrechushkin Aug 10, 2020

Choose a reason for hiding this comment

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

@lfu
I create customization dialog, add dynamic fields, assign automation instance to fiield, save dialog
Next i tried to change assigned instance and got error.

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.

sorry for the delay.

To me this looks like a single lookup for the instances, and then from there fetching the namespaces and classes

To be honest, not sure if we really need to fetch all of those objects.

Just fetching the instance can provide all the metadata we need. (but that is probably a micro optimization

@@ -56,7 +56,7 @@ def automate_find_hierarchy(fqname)
begin
# Collect all the db records based on the parsed fqname
open_nodes = namespace.split('/').each_with_object([]) do |ns, items|
items << MiqAeNamespace.find_by!(:name => ns, :parent => items.last)
items << MiqAeNamespace.find_by!(:name => ns, :ancestry => items.last)
Copy link
Member

Choose a reason for hiding this comment

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

  • Really like to avoid using ancestry column in code.
  • This N+1 looking caused by the each_with_object can be avoided. In concept, it is fetching all ancestors of the namespace.split('/').last node.
  • knowing the class of the lowest node would be helpful. Not sure if that is possible, but it will tell us where to start. the call to automate parse suggests that this is possible.
  • This will work for 2 levels, but will not find nodes under that, since the ancestry will require the name of multiple nodes.
  • lets use path (need to verify that path is in the desired miq version)

To be honest you should be able to get the lowest child and ask for all the parents in just a few queries. If nothing else, fetching all the namespaces in a single query

kbrock added a commit to kbrock/manageiq-ui-classic that referenced this pull request Aug 13, 2020
looking up the instance by relative path in a single query rather
than traversing all these object

fixes ManageIQ#7249
kbrock added a commit to kbrock/manageiq-ui-classic that referenced this pull request Aug 13, 2020
looking up the instance by relative path in a single query rather
than traversing all these object

fixes ManageIQ#7249
kbrock added a commit to kbrock/manageiq-ui-classic that referenced this pull request Aug 16, 2020
looking up the instance by relative path in a single query rather
than traversing all these object

fixes ManageIQ#7249
@lfu lfu mentioned this pull request Aug 17, 2020
1 task
@h-kataria h-kataria merged commit f4d3d29 into ManageIQ:master Aug 17, 2020
kbrock added a commit to kbrock/manageiq-ui-classic that referenced this pull request Aug 17, 2020
looking up the instance by relative path in a single query rather
than traversing all these object

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

Successfully merging this pull request may close these issues.

None yet

7 participants