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

Search for resource type efficiently #6242

Merged
merged 2 commits into from
Jan 12, 2023
Merged

Conversation

ravening
Copy link
Member

Description

Use hashmap to search for resource type rather than iterating over
all of them

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

Copy link
Member

@rohityadavcloud rohityadavcloud left a comment

Choose a reason for hiding this comment

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

LGTM, subject to @Pearl1594 's review/testing who worked on the original feature

@rohityadavcloud
Copy link
Member

@blueorangutan package

@@ -91,6 +96,53 @@ public boolean resourceMetadataSupport() {
public boolean resourceIconSupport() {
return resourceIconSupport;
}

static {
resourceObjectTypeMap.put("uservm", UserVm);
Copy link
Member

Choose a reason for hiding this comment

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

@ravening is there any automatic way to have lowered-case object-type name map to the ResourceObjectType? Otherwise, this creates issue of manually maintaining this hashmap.

Copy link
Member Author

Choose a reason for hiding this comment

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

@rohityadavcloud didnt get your point. can you explain more?

Copy link
Member

Choose a reason for hiding this comment

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

For every ResourceObjectType, you're manually entering a map in resourceObjectTypeMap. Why not use reflections or other ways to generate this map automatically (essentially iterate all ResourceObjectType and set the "lowercaseclassname" to the Class) @ravening ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@rohityadavcloud optimized it. review it again.

@acs-robot
Copy link
Collaborator

Found UI changes, kicking a new UI QA build
@blueorangutan ui

@acs-robot
Copy link
Collaborator

Found Java/XML changes, kicking packaging job
@blueorangutan package

@blueorangutan
Copy link

@acs-robot a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result: ✔️ el7 ✖️ el8 ✖️ debian ✖️ suse15. SL-JID 3211

@acs-robot
Copy link
Collaborator

PR Coverage Report

CLASS INSTRUCTION MISSED INSTRUCTION COVERED BRANCH MISSED BRANCH COVERED LINE MISSED LINE COVERED
ClusteredAgentManagerImpl 2361 0 242 0 536 0
DefaultVMSnapshotStrategy 486 677 52 30 100 140
LibvirtVMDef 37 114 6 4 11 36
VmwareManagerImpl 2613 528 296 38 615 117
VmwareStorageProcessor 9886 9 940 0 2121 2
NetScalerControlCenterResource 1943 0 144 0 468 0
NetscalerResource 6882 0 806 0 1623 0
DateraPrimaryDataStoreDriver 3195 0 283 0 748 0
CloudStackPrimaryDataStoreDriverImpl 903 0 114 0 229 0
LinstorPrimaryDataStoreDriverImpl 1442 0 91 0 348 0
ScaleIOPrimaryDataStoreDriver 2537 0 246 0 537 0
SolidFirePrimaryDataStoreDriver 3347 0 284 0 697 0
SAMLUtils 202 465 41 11 53 108
VolumeApiServiceImpl 10826 0 1498 0 2031 0
ResourceManagerUtilImpl 341 0 20 0 69 0
TaggedResourceManagerImpl 473 0 58 0 96 0
PremiumSecondaryStorageManagerImpl 775 0 64 0 116 0
SecondaryStorageManagerImpl 3494 149 343 11 623 32

@github-actions
Copy link

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

Rakesh Venkatesh added 2 commits April 25, 2022 14:21
Use hashmap to search for resource type rather than iterating over
all of them
@acs-robot
Copy link
Collaborator

Found UI changes, kicking a new UI QA build
@blueorangutan ui

@blueorangutan
Copy link

@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

@blueorangutan
Copy link

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6242 (SL-JID-1463)

@acs-robot
Copy link
Collaborator

PR Coverage Report

CLASS INSTRUCTION MISSED INSTRUCTION COVERED BRANCH MISSED BRANCH COVERED LINE MISSED LINE COVERED
ResourceManagerUtilImpl 341 0 20 0 69 0

@DaanHoogland
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@apache apache deleted a comment from rohityadavcloud Dec 13, 2022
@apache apache deleted a comment from blueorangutan Dec 13, 2022
@apache apache deleted a comment from blueorangutan Dec 13, 2022
@apache apache deleted a comment from blueorangutan Dec 13, 2022
@apache apache deleted a comment from blueorangutan Dec 13, 2022
@blueorangutan
Copy link

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✖️ suse15. SL-JID 4903

@DaanHoogland
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

Trillian Build Failed (tid-5514)

@blueorangutan
Copy link

Trillian Build Failed (tid-5524)

@DaanHoogland
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 5021

@DaanHoogland
Copy link
Contributor

@blueorangutan test

@DaanHoogland
Copy link
Contributor

@ravening can you add some tests? (or a #!human test description)

@blueorangutan
Copy link

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

Trillian test result (tid-5571)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 41771 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6242-t5571-kvm-centos7.zip
Smoke tests completed. 105 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File

Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

clgtm

@DaanHoogland
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 5222

@DaanHoogland
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

Trillian test result (tid-5789)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 53933 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6242-t5789-kvm-centos7.zip
Smoke tests completed. 104 look OK, 3 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_08_upgrade_kubernetes_ha_cluster Failure 565.56 test_kubernetes_clusters.py
test_01_verify_ipv6_vpc Failure 927.22 test_vpc_ipv6.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL Failure 657.93 test_vpc_redundant.py
test_05_rvpc_multi_tiers Failure 600.08 test_vpc_redundant.py
test_05_rvpc_multi_tiers Error 600.10 test_vpc_redundant.py

@DaanHoogland
Copy link
Contributor

errors seem not related. making sure:
@blueorangutan test

@blueorangutan
Copy link

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

Trillian test result (tid-5794)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 46655 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6242-t5794-kvm-centos7.zip
Smoke tests completed. 107 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File

@DaanHoogland DaanHoogland merged commit aad0353 into apache:main Jan 12, 2023
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.

5 participants