You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2020/10/05 14:59:33 UTC

[GitHub] [cloudstack] davidjumani opened a new pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

davidjumani opened a new pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374


   ## Description
   Fixes https://github.com/apache/cloudstack/issues/4372
   
   Ensures that the count returned by searchAndCount searchAndDistinctCount do not include removed items
   
   ## Types of changes
   <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
   - [ ] Breaking change (fix or feature that would cause existing functionality to change)
   - [ ] New feature (non-breaking change which adds functionality)
   - [x] 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?
   TODO


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] davidjumani commented on a change in pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
davidjumani commented on a change in pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#discussion_r500060364



##########
File path: framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java
##########
@@ -1935,6 +1918,23 @@ public boolean configure(final String name, final Map<String, Object> params) th
         return builder.create();
     }
 
+    private SearchCriteria<T> checkAndSetRemovedIsNull(SearchCriteria<T> sc) {
+        if (_removed != null) {
+            if (sc == null) {
+                sc = createSearchCriteria();
+            }
+            sc.addAnd(_removed.second().field.getName(), SearchCriteria.Op.NULL);
+        }
+        return sc;
+    }
+
+    public Integer getDistinctCount(SearchCriteria<T> sc, boolean removed) {
+        if (!removed) {

Review comment:
       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`  ?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#discussion_r500068971



##########
File path: framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java
##########
@@ -1935,6 +1918,23 @@ public boolean configure(final String name, final Map<String, Object> params) th
         return builder.create();
     }
 
+    private SearchCriteria<T> checkAndSetRemovedIsNull(SearchCriteria<T> sc) {
+        if (_removed != null) {
+            if (sc == null) {
+                sc = createSearchCriteria();
+            }
+            sc.addAnd(_removed.second().field.getName(), SearchCriteria.Op.NULL);
+        }
+        return sc;
+    }
+
+    public Integer getDistinctCount(SearchCriteria<T> sc, boolean removed) {
+        if (!removed) {

Review comment:
       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.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] davidjumani commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
davidjumani commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-704282027


   @blueorangutan test


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] davidjumani commented on a change in pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
davidjumani commented on a change in pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#discussion_r500059151



##########
File path: framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java
##########
@@ -519,7 +509,6 @@ public T lockOneRandomRow(final SearchCriteria<T> sc, final boolean exclusive) {
         if (_removed != null) {
             sc.addAnd(_removed.second().field.getName(), SearchCriteria.Op.NULL);

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




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-703735933


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] davidjumani commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
davidjumani commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-704219094


   @blueorangutan package


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-704628881


   <b>Trillian test result (tid-2901)</b>
   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
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#discussion_r500051857



##########
File path: framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java
##########
@@ -519,7 +509,6 @@ public T lockOneRandomRow(final SearchCriteria<T> sc, final boolean exclusive) {
         if (_removed != null) {
             sc.addAnd(_removed.second().field.getName(), SearchCriteria.Op.NULL);

Review comment:
       can use _checkAndSetRemovedIsNull()_ here ? 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#discussion_r500068971



##########
File path: framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java
##########
@@ -1935,6 +1918,23 @@ public boolean configure(final String name, final Map<String, Object> params) th
         return builder.create();
     }
 
+    private SearchCriteria<T> checkAndSetRemovedIsNull(SearchCriteria<T> sc) {
+        if (_removed != null) {
+            if (sc == null) {
+                sc = createSearchCriteria();
+            }
+            sc.addAnd(_removed.second().field.getName(), SearchCriteria.Op.NULL);
+        }
+        return sc;
+    }
+
+    public Integer getDistinctCount(SearchCriteria<T> sc, boolean removed) {
+        if (!removed) {

Review comment:
       Default method should not include removed records, explicitly mentioned methods "_*IncludingRemovedBy()_" should.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-705620652


   @blueorangutan test centos7 xs71
   @blueorangutan test centos7 kvmcentos7


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-705152458


   <b>Trillian test result (tid-2905)</b>
   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
   --- | --- | --- | ---
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-706937468


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-704269441


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] davidjumani commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
davidjumani commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-703716408


   @blueorangutan package


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-704180635


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#discussion_r500065456



##########
File path: framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java
##########
@@ -519,7 +509,6 @@ public T lockOneRandomRow(final SearchCriteria<T> sc, final boolean exclusive) {
         if (_removed != null) {
             sc.addAnd(_removed.second().field.getName(), SearchCriteria.Op.NULL);

Review comment:
       ok @davidjumani 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-703717089


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-703745379


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-707231124


   <b>Trillian test result (tid-2926)</b>
   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
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-706937211


   @blueorangutan test centos7 kvm-centos7


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-706174878


   @blueorangutan test centos7 xenserver-71


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#discussion_r500057043



##########
File path: framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java
##########
@@ -1935,6 +1918,23 @@ public boolean configure(final String name, final Map<String, Object> params) th
         return builder.create();
     }
 
+    private SearchCriteria<T> checkAndSetRemovedIsNull(SearchCriteria<T> sc) {
+        if (_removed != null) {
+            if (sc == null) {
+                sc = createSearchCriteria();
+            }
+            sc.addAnd(_removed.second().field.getName(), SearchCriteria.Op.NULL);
+        }
+        return sc;
+    }
+
+    public Integer getDistinctCount(SearchCriteria<T> sc, boolean removed) {
+        if (!removed) {

Review comment:
       `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.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] davidjumani commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
davidjumani commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-704180400


   @blueorangutan package


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#discussion_r500090332



##########
File path: framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java
##########
@@ -1935,6 +1918,23 @@ public boolean configure(final String name, final Map<String, Object> params) th
         return builder.create();
     }
 
+    private SearchCriteria<T> checkAndSetRemovedIsNull(SearchCriteria<T> sc) {
+        if (_removed != null) {
+            if (sc == null) {
+                sc = createSearchCriteria();
+            }
+            sc.addAnd(_removed.second().field.getName(), SearchCriteria.Op.NULL);
+        }
+        return sc;
+    }
+
+    public Integer getDistinctCount(SearchCriteria<T> sc, boolean removed) {
+        if (!removed) {

Review comment:
       Perfect, changes LGTM.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-704245260


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland merged pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
DaanHoogland merged pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-704219366


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-705621151






----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-704806815


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] davidjumani commented on a change in pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
davidjumani commented on a change in pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#discussion_r500073001



##########
File path: framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java
##########
@@ -1935,6 +1918,23 @@ public boolean configure(final String name, final Map<String, Object> params) th
         return builder.create();
     }
 
+    private SearchCriteria<T> checkAndSetRemovedIsNull(SearchCriteria<T> sc) {
+        if (_removed != null) {
+            if (sc == null) {
+                sc = createSearchCriteria();
+            }
+            sc.addAnd(_removed.second().field.getName(), SearchCriteria.Op.NULL);
+        }
+        return sc;
+    }
+
+    public Integer getDistinctCount(SearchCriteria<T> sc, boolean removed) {
+        if (!removed) {

Review comment:
       Added! Thanks @sureshanaparti 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] davidjumani commented on a change in pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
davidjumani commented on a change in pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#discussion_r500060364



##########
File path: framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java
##########
@@ -1935,6 +1918,23 @@ public boolean configure(final String name, final Map<String, Object> params) th
         return builder.create();
     }
 
+    private SearchCriteria<T> checkAndSetRemovedIsNull(SearchCriteria<T> sc) {
+        if (_removed != null) {
+            if (sc == null) {
+                sc = createSearchCriteria();
+            }
+            sc.addAnd(_removed.second().field.getName(), SearchCriteria.Op.NULL);
+        }
+        return sc;
+    }
+
+    public Integer getDistinctCount(SearchCriteria<T> sc, boolean removed) {
+        if (!removed) {

Review comment:
       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




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan removed a comment on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-704245260


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-704282412


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-706175512


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-705621151


   @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`


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-705682354


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-704197688


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-705682354


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#discussion_r500066734



##########
File path: framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java
##########
@@ -1935,6 +1918,23 @@ public boolean configure(final String name, final Map<String, Object> params) th
         return builder.create();
     }
 
+    private SearchCriteria<T> checkAndSetRemovedIsNull(SearchCriteria<T> sc) {
+        if (_removed != null) {
+            if (sc == null) {
+                sc = createSearchCriteria();
+            }
+            sc.addAnd(_removed.second().field.getName(), SearchCriteria.Op.NULL);
+        }
+        return sc;
+    }
+
+    public Integer getDistinctCount(SearchCriteria<T> sc, boolean removed) {
+        if (!removed) {

Review comment:
       _getDistinctCountExcludingRemoved_ is not in line with other methods, so I do not recommend.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-705620652






----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] davidjumani commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
davidjumani commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-703744568


   @blueorangutan test


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] davidjumani commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
davidjumani commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-704806066


   @blueorangutan test matrix


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-705683084


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4374: Fixing searchAndCount searchAndDistinctCount when sc is null

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4374:
URL: https://github.com/apache/cloudstack/pull/4374#issuecomment-706448006


   <b>Trillian test result (tid-2920)</b>
   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
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org