Fixing searchAndCount searchAndDistinctCount when sc is null by davidjumani · Pull Request #4374 · apache/cloudstack

davidjumani · 2020-10-05T14:59:24Z

Description

Ensures that the count returned by searchAndCount searchAndDistinctCount do not include removed items
Before, it would return the count of removed items as well when the search criteria is null (since sc is created and modified in search which doesn't reflect when it tires to get the count)
Also refactored a bit to make life easieer for the next person who comes along

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)

How Has This Been Tested?

Before :

(localcloud) SBCM5> > list roles filter=
{
  "count": 11,
  "role": [
    {},
    {},
    {},
    {},
    {},
    {},
    {},
    {},
    {}
  ]
}

After :

(localcloud) SBCM5> > list roles filter=
{
  "count": 9,
  "role": [
    {},
    {},
    {},
    {},
    {},
    {},
    {},
    {},
    {}
  ]
}

davidjumani · 2020-10-05T15:43:46Z

@blueorangutan package

blueorangutan · 2020-10-05T15:44:55Z

@davidjumani a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-10-05T16:16:33Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2118

davidjumani · 2020-10-05T16:31:31Z

@blueorangutan test

blueorangutan · 2020-10-05T16:32:50Z

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

DaanHoogland

code looks good. I think the search and count method should however have a single set of criteria for both the results and the count and not need any adjustments down the line. starting with an sc only containing "true", could be a solution (as well or on top of this, no pref)

sureshanaparti · 2020-10-06T07:11:05Z

framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java

@@ -519,7 +509,6 @@ public <M> List<M> customSearch(SearchCriteria<M> sc, final Filter filter) {
        if (_removed != null) {
            sc.addAnd(_removed.second().field.getName(), SearchCriteria.Op.NULL);


can use checkAndSetRemovedIsNull() here ?

Makes it a bit difficult since it is of a different type M not T

ok @davidjumani

sureshanaparti · 2020-10-06T07:21:20Z

framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java

+    }
+
+    public Integer getDistinctCount(SearchCriteria<T> sc, boolean removed) {
+        if (!removed) {


removed & _removed seems to be redundant here & wherever used in similar way, can you directly set / unset _removed as per its usage ? If the purpose is to include / exclude removed records, you can use methods with naming *IncludingRemovedBy() [for ex: getDistinctCountIncludingRemovedBy()] to be in sync with the other methods.

As of now getDistinctCount returns including removed so changing it would require changing where it's been referenced, would it be better to create getDistinctCountExcludingRemoved ?
Edit : Nevermind. Isn't used much elsewhere. Will make the changes

getDistinctCountExcludingRemoved is not in line with other methods, so I do not recommend.

Default method should not include removed records, explicitly mentioned methods "*IncludingRemovedBy()" should. so, may be you can change current method to "*IncludingRemovedBy()" and add other method with current name.

Added! Thanks @sureshanaparti

Perfect, changes LGTM.

davidjumani · 2020-10-06T10:32:21Z

@blueorangutan package

blueorangutan · 2020-10-06T10:32:54Z

@davidjumani a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-10-06T11:10:42Z

Packaging result: ✖centos7 ✖centos8 ✔debian. JID-2125

davidjumani · 2020-10-06T11:57:23Z

@blueorangutan package

blueorangutan · 2020-10-06T11:57:54Z

@davidjumani a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-10-06T13:29:15Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2134

davidjumani · 2020-10-06T13:49:24Z

@blueorangutan test

blueorangutan · 2020-10-06T13:49:59Z

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

blueorangutan · 2020-10-07T00:51:11Z

Trillian test result (tid-2901)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37746 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4374-t2901-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 83 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers	`Failure`	290.08	test_vpc_redundant.py
test_hostha_kvm_host_fencing	`Error`	99.19	test_hostha_kvm.py

davidjumani · 2020-10-07T09:16:34Z

@blueorangutan test matrix

blueorangutan · 2020-10-07T09:17:58Z

@davidjumani a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware67, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests

blueorangutan · 2020-10-07T19:40:18Z

Trillian test result (tid-2905)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 35549 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4374-t2905-vmware-67u3.zip
Intermittent failure detected: /marvin/tests/smoke/test_host_maintenance.py
Smoke tests completed. 83 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

DaanHoogland · 2020-10-08T14:46:11Z

@blueorangutan test centos7 xs71
@blueorangutan test centos7 kvmcentos7

blueorangutan · 2020-10-08T14:47:03Z

@DaanHoogland unsupported parameters provided. Supported mgmt server os are: centos6, centos7, centos8, ubuntu. Supported hypervisors are: kvm-centos6, kvm-centos7, kvm-centos8, kvm-ubuntu, xenserver-71, xenserver-65sp1, vmware-67u3, vmware-65u2, vmware-60u2, vmware-55u3, xcpng76, xcpng80, xcpng81, xenserver-74, xcpng74

sureshanaparti · 2020-10-08T16:26:56Z

@blueorangutan test centos7 xenserver-71
@blueorangutan test centos7 kvm-centos7

blueorangutan · 2020-10-08T16:28:08Z

@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + xenserver-71) has been kicked to run smoke tests

DaanHoogland · 2020-10-09T13:17:00Z

@blueorangutan test centos7 xenserver-71

blueorangutan · 2020-10-09T13:18:12Z

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

blueorangutan · 2020-10-09T23:56:04Z

Trillian test result (tid-2920)
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36403 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4374-t2920-xenserver-71.zip
Intermittent failure detected: /marvin/tests/smoke/test_scale_vm.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 84 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_scale_vm	`Failure`	9.40	test_scale_vm.py

DaanHoogland · 2020-10-12T07:36:50Z

@blueorangutan test centos7 kvm-centos7

blueorangutan · 2020-10-12T07:37:19Z

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

blueorangutan · 2020-10-12T16:47:34Z

Trillian test result (tid-2926)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31223 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4374-t2926-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 84 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_hostha_kvm_host_fencing	`Error`	101.42	test_hostha_kvm.py

davidjumani force-pushed the fix-searchandcount branch from b4de2a4 to a430bf6 Compare October 5, 2020 15:40

Fixing searchAndCount searchAndDistinctCount when sc is null

120ee25

davidjumani force-pushed the fix-searchandcount branch from a430bf6 to 120ee25 Compare October 5, 2020 15:42

davidjumani requested a review from DaanHoogland October 5, 2020 15:44

davidjumani mentioned this pull request Oct 5, 2020

Fixing count for findHostsForMigration #4375

Merged

5 tasks

davidjumani added this to the 4.15.0.0 milestone Oct 5, 2020

DaanHoogland approved these changes Oct 6, 2020

View reviewed changes

sureshanaparti reviewed Oct 6, 2020

View reviewed changes

Adding getCountIncludingRemoved and getDistinctCountIncludingRemoved

9a7be55

sureshanaparti approved these changes Oct 6, 2020

View reviewed changes

apache deleted a comment from blueorangutan Oct 6, 2020

davidjumani marked this pull request as ready for review October 7, 2020 04:57

DaanHoogland merged commit aab8df0 into apache:master Oct 13, 2020

DaanHoogland deleted the fix-searchandcount branch October 13, 2020 09:30

DaanHoogland added the type:bug label Oct 21, 2020

DaanHoogland added the Severity:Major label Nov 10, 2020

DaanHoogland mentioned this pull request Feb 5, 2021

Count mismatch for list diskofferings api #4195

Closed

		@@ -519,7 +509,6 @@ public <M> List<M> customSearch(SearchCriteria<M> sc, final Filter filter) {
		if (_removed != null) {
		sc.addAnd(_removed.second().field.getName(), SearchCriteria.Op.NULL);

Conversation

davidjumani commented Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

How Has This Been Tested?

Uh oh!

davidjumani commented Oct 5, 2020

Uh oh!

blueorangutan commented Oct 5, 2020

Uh oh!

blueorangutan commented Oct 5, 2020

Uh oh!

davidjumani commented Oct 5, 2020

Uh oh!

blueorangutan commented Oct 5, 2020

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

sureshanaparti Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

davidjumani Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

sureshanaparti Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

sureshanaparti Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

davidjumani Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sureshanaparti Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

sureshanaparti Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidjumani Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

sureshanaparti Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

davidjumani commented Oct 6, 2020

Uh oh!

blueorangutan commented Oct 6, 2020

Uh oh!

blueorangutan commented Oct 6, 2020

Uh oh!

davidjumani commented Oct 6, 2020

Uh oh!

blueorangutan commented Oct 6, 2020

Uh oh!

blueorangutan commented Oct 6, 2020

Uh oh!

davidjumani commented Oct 6, 2020

Uh oh!

blueorangutan commented Oct 6, 2020

Uh oh!

blueorangutan commented Oct 7, 2020

Uh oh!

davidjumani commented Oct 7, 2020

Uh oh!

blueorangutan commented Oct 7, 2020

Uh oh!

blueorangutan commented Oct 7, 2020

Uh oh!

DaanHoogland commented Oct 8, 2020

Uh oh!

blueorangutan commented Oct 8, 2020

Uh oh!

sureshanaparti commented Oct 8, 2020

Uh oh!

blueorangutan commented Oct 8, 2020

davidjumani commented Oct 5, 2020 •

edited

Loading

davidjumani Oct 6, 2020 •

edited

Loading

sureshanaparti Oct 6, 2020 •

edited

Loading