You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "mlsorensen (via GitHub)" <gi...@apache.org> on 2023/08/29 16:52:11 UTC

[GitHub] [cloudstack] mlsorensen opened a new pull request, #7922: Add index on cluster_details.name for FirstFitPlanner speedup

mlsorensen opened a new pull request, #7922:
URL: https://github.com/apache/cloudstack/pull/7922

   ### Description
   
   This PR addresses slowness in FirstFitPlanner, looking for a cluster with capacity for new VM.
   
   Adding an index to `cluster_details.name` yields a 5-10x speed improvement in this query, and 2-3x improvement in overall time to create 1000 VMs in parallel during scale testing.
   
   Using slow query logging, creating 1000 VMs with 100 parallel workers:
   
   ```
   # Query_time: 19.002664  
   SELECT DISTINCT capacity.cluster_id  FROM `cloud`.`op_host_capacity` capacity INNER JOIN `cloud`.`cluster` cluster on (cluster.id = capacity.cluster_id AND cluster.removed is NULL)   INNER JOIN `cloud`.`cluster_details` cluster_details ON (cluster.id = cluster_details.cluster_id ) WHERE capacity.data_center_id = 1 AND capacity_type = 1 AND cluster_details.name= 'cpuOvercommitRatio' AND ((total_capacity * cluster_details.value ) - used_capacity + reserved_capacity) >= 100 AND capacity.cluster_id IN (SELECT distinct capacity.cluster_id  FROM `cloud`.`op_host_capacity` capacity INNER JOIN  `cloud`.`cluster_details` cluster_details ON (capacity.cluster_id = cluster_details.cluster_id ) WHERE capacity.data_center_id = 1 AND capacity_type = 0 AND cluster_details.name= 'memoryOvercommitRatio' AND ((total_capacity * cluster_details.value) - used_capacity + reserved_capacity) >= 1073741824);


   
   # Query_time: 20.131617
   SELECT DISTINCT capacity.cluster_id  FROM `cloud`.`op_host_capacity` capacity INNER JOIN `cloud`.`cluster` cluster on (cluster.id = capacity.cluster_id AND cluster.removed is NULL)   INNER JOIN `cloud`.`cluster_details` cluster_details ON (cluster.id = cluster_details.cluster_id ) WHERE capacity.data_center_id = 1 AND capacity_type = 1 AND cluster_details.name= 'cpuOvercommitRatio' AND ((total_capacity * cluster_details.value ) - used_capacity + reserved_capacity) >= 100 AND capacity.cluster_id IN (SELECT distinct capacity.cluster_id  FROM `cloud`.`op_host_capacity` capacity INNER JOIN  `cloud`.`cluster_details` cluster_details ON (capacity.cluster_id = cluster_details.cluster_id ) WHERE capacity.data_center_id = 1 AND capacity_type = 0 AND cluster_details.name= 'memoryOvercommitRatio' AND ((total_capacity * cluster_details.value) - used_capacity + reserved_capacity) >= 1073741824);
   ```
   
   Same test after index:
   
   ```
   # Query_time: 1.139716 
   SELECT DISTINCT capacity.cluster_id  FROM `cloud`.`op_host_capacity` capacity INNER JOIN `cloud`.`cluster` cluster on (cluster.id = capacity.cluster_id AND cluster.removed is NULL)   INNER JOIN `cloud`.`cluster_details` cluster_details ON (cluster.id = cluster_details.cluster_id ) WHERE capacity.data_center_id = 1 AND capacity_type = 1 AND cluster_details.name= 'cpuOvercommitRatio' AND ((total_capacity * cluster_details.value ) - used_capacity + reserved_capacity) >= 100 AND capacity.cluster_id IN (SELECT distinct capacity.cluster_id  FROM `cloud`.`op_host_capacity` capacity INNER JOIN  `cloud`.`cluster_details` cluster_details ON (capacity.cluster_id = cluster_details.cluster_id ) WHERE capacity.data_center_id = 1 AND capacity_type = 0 AND cluster_details.name= 'memoryOvercommitRatio' AND ((total_capacity * cluster_details.value) - used_capacity + reserved_capacity) >= 1073741824);


   
   # Query_time: 1.025688  
   SELECT DISTINCT capacity.cluster_id  FROM `cloud`.`op_host_capacity` capacity INNER JOIN `cloud`.`cluster` cluster on (cluster.id = capacity.cluster_id AND cluster.removed is NULL)   INNER JOIN `cloud`.`cluster_details` cluster_details ON (cluster.id = cluster_details.cluster_id ) WHERE capacity.data_center_id = 1 AND capacity_type = 1 AND cluster_details.name= 'cpuOvercommitRatio' AND ((total_capacity * cluster_details.value ) - used_capacity + reserved_capacity) >= 100 AND capacity.cluster_id IN (SELECT distinct capacity.cluster_id  FROM `cloud`.`op_host_capacity` capacity INNER JOIN  `cloud`.`cluster_details` cluster_details ON (capacity.cluster_id = cluster_details.cluster_id ) WHERE capacity.data_center_id = 1 AND capacity_type = 0 AND cluster_details.name= 'memoryOvercommitRatio' AND ((total_capacity * cluster_details.value) - used_capacity + reserved_capacity) >= 1073741824);
   ```
   
   ### 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)
   - [x] 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
   - x ] Minor
   
   #### Bug Severity
   
   - [ ] BLOCKER
   - [ ] Critical
   - [ ] Major
   - [x] Minor
   - [ ] Trivial
   
   
   ### Screenshots (if appropriate):
   
   
   ### How Has This Been Tested?
   Tested upgrade path locally:
   
   ```
   4.18.0.0 to 4.18.1.0:
   
   DEBUG [c.c.u.d.DatabaseAccessObject] (main:null) (logid:) Created index i_cluster_details__name
   ...
   DEBUG [c.c.u.DatabaseUpgradeChecker] (main:null) (logid:) Upgrade completed for version 4.18.1.0
   
   mysql> show indexes from cluster_details where Key_name="i_cluster_details__name";
   +-----------------+------------+-------------------------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+---------------+---------+------------+
   | Table           | Non_unique | Key_name                | Seq_in_index | Column_name | Collation | Cardinality | Sub_part | Packed | Null | Index_type | Comment | Index_comment | Visible | Expression |
   +-----------------+------------+-------------------------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+---------------+---------+------------+
   | cluster_details |          1 | i_cluster_details__name |            1 | name        | A         |           2 |     NULL |   NULL |      | BTREE      |         |               | YES     | NULL       |
   +-----------------+------------+-------------------------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+---------------+---------+------------+
   ```
   
   In case index already exists:
   ```
   4.18.0.0 to 4.18.1.0 where index already exists:
   
   DEBUG [c.c.u.d.DatabaseAccessObject] (main:null) (logid:) Index i_cluster_details__name already exists
   ...
   DEBUG [c.c.u.DatabaseUpgradeChecker] (main:null) (logid:) Upgrade completed for version 4.18.1.0
   ```
   
   
   <!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] weizhouapache commented on a diff in pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#discussion_r1309727936


##########
engine/schema/src/main/java/com/cloud/upgrade/dao/DbUpgradeUtils.java:
##########
@@ -21,7 +21,15 @@
 
 public class DbUpgradeUtils {
 
-    private static DatabaseAccessObject dao = new DatabaseAccessObject();
+    private static final DatabaseAccessObject dao = new DatabaseAccessObject();

Review Comment:
   it seems this causes the test error
   
   ```
   08:07:02 [ERROR] testDropPrimaryKey(com.cloud.upgrade.dao.DbUpgradeUtilsTest)  Time elapsed: 0.706 s  <<< ERROR!
   08:07:02 java.lang.NullPointerException
   08:07:02 	at com.cloud.upgrade.dao.DbUpgradeUtilsTest.testDropPrimaryKey(DbUpgradeUtilsTest.java:96)
   ```



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] weizhouapache commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1697933222

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

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] mlsorensen commented on a diff in pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "mlsorensen (via GitHub)" <gi...@apache.org>.
mlsorensen commented on code in PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#discussion_r1309212546


##########
engine/schema/src/main/java/com/cloud/upgrade/dao/DatabaseAccessObject.java:
##########
@@ -84,6 +85,31 @@ public boolean columnExists(Connection conn, String tableName, String columnName
         return columnExists;
     }
 
+    public void addIndexIfNeeded(Connection conn, String tableName, String columnName) {

Review Comment:
   Before I do this rework, looking for feedback. Do we want to:
   
   1) create index naming function to generate index name
   2) call indexExists with tableName and indexName
   3) call createIndex with tableName and indexName
   
   It seems to me we would prefer to keep the index naming scheme private to outside callers so it isn't something overridable and no potential for mismatches between checking existence and creating - which leads me to think we would want to retain a single entry point function like the `addIndexIfNeeded` to generate the name privately and then call `indexExists` and `createIndex` if we still want to break these out.



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] blueorangutan commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1698565279

   <b>[SF] Trillian test result (tid-7603)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 41398 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7922-t7603-kvm-centos7.zip
   Smoke tests completed. 108 look OK, 0 have errors, 0 did not run
   Only failed and skipped 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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] blueorangutan commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1697930847

   Packaging result [SF]: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 6935


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] weizhouapache commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1698541443

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

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] mlsorensen commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "mlsorensen (via GitHub)" <gi...@apache.org>.
mlsorensen commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1699399507

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

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] mlsorensen commented on a diff in pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "mlsorensen (via GitHub)" <gi...@apache.org>.
mlsorensen commented on code in PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#discussion_r1309204284


##########
engine/schema/src/main/java/com/cloud/upgrade/dao/DatabaseAccessObject.java:
##########
@@ -84,6 +85,31 @@ public boolean columnExists(Connection conn, String tableName, String columnName
         return columnExists;
     }
 
+    public void addIndexIfNeeded(Connection conn, String tableName, String columnName) {

Review Comment:
   Yeah, I was thinking about that but then I have to create a separate index naming function, etc. I guess I was just being lazy.



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] blueorangutan commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1701673155

   <b>[SF] Trillian test result (tid-7615)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 42941 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7922-t7615-kvm-centos7.zip
   Smoke tests completed. 108 look OK, 0 have errors, 0 did not run
   Only failed and skipped 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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] rohityadavcloud commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1697936845

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

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] weizhouapache commented on a diff in pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#discussion_r1309375977


##########
engine/schema/src/main/java/com/cloud/upgrade/dao/DatabaseAccessObject.java:
##########
@@ -84,6 +85,31 @@ public boolean columnExists(Connection conn, String tableName, String columnName
         return columnExists;
     }
 
+    public void addIndexIfNeeded(Connection conn, String tableName, String columnName) {

Review Comment:
   Nice, looks good to me



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] weizhouapache commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1698174348

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

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] blueorangutan commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1697937610

   @rohityadavcloud a [SF] 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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] weizhouapache commented on a diff in pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#discussion_r1309173902


##########
engine/schema/src/main/java/com/cloud/upgrade/dao/DatabaseAccessObject.java:
##########
@@ -84,6 +85,31 @@ public boolean columnExists(Connection conn, String tableName, String columnName
         return columnExists;
     }
 
+    public void addIndexIfNeeded(Connection conn, String tableName, String columnName) {

Review Comment:
   this can be extracted to 2 atom operations
   - indexExists
   - createIndex



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] blueorangutan commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1698542325

   @weizhouapache a [SF] 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.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] weizhouapache commented on a diff in pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#discussion_r1309225760


##########
engine/schema/src/main/java/com/cloud/upgrade/dao/DatabaseAccessObject.java:
##########
@@ -84,6 +85,31 @@ public boolean columnExists(Connection conn, String tableName, String columnName
         return columnExists;
     }
 
+    public void addIndexIfNeeded(Connection conn, String tableName, String columnName) {

Review Comment:
   agree with you
   - add `indexExists` and `createIndex` in `DatabaseAccessObject`
   - add `addIndexIfNeeded` in `DbUpgradeUtils`, which calls `indexExists` and `createIndex` 
   - call `DbUpgradeUtils.addIndexIfNeeded` in Upgrade*.java



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] weizhouapache merged pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache merged PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] weizhouapache commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1700487233

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

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] blueorangutan commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1697865577

   @weizhouapache a [SF] 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.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] mlsorensen commented on a diff in pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "mlsorensen (via GitHub)" <gi...@apache.org>.
mlsorensen commented on code in PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#discussion_r1309212546


##########
engine/schema/src/main/java/com/cloud/upgrade/dao/DatabaseAccessObject.java:
##########
@@ -84,6 +85,31 @@ public boolean columnExists(Connection conn, String tableName, String columnName
         return columnExists;
     }
 
+    public void addIndexIfNeeded(Connection conn, String tableName, String columnName) {

Review Comment:
   Before I do this rework @weizhouapache, looking for feedback. Do we want to:
   
   1) create index naming function to generate index name
   2) call indexExists with tableName and indexName
   3) call createIndex with tableName and indexName
   
   It seems to me we would prefer to keep the index naming scheme private to outside callers so it isn't something overridable and no potential for mismatches between checking existence and creating - which leads me to think we would want to retain a single entry point function like the `addIndexIfNeeded` to generate the name privately and then call `indexExists` and `createIndex` if we still want to break these out. Maybe moving `addIndexIfNeeded` to the `DbUpgradeUtils()` in the process?



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] blueorangutan commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1698219674

   Packaging result [SF]: :heavy_check_mark: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: el9 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 6936


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] blueorangutan commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1698175267

   @weizhouapache a [SF] 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.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] blueorangutan commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1699402154

   @mlsorensen a [SF] 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.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] blueorangutan commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1700480713

   Packaging result [SF]: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 6950


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] weizhouapache commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1697862595

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

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] codecov[bot] commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1697889249

   ## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/7922?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   > Merging [#7922](https://app.codecov.io/gh/apache/cloudstack/pull/7922?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (b677858) into [4.18](https://app.codecov.io/gh/apache/cloudstack/commit/439d70fd2b907a4ed12ed9b67b3be78969b01a8b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (439d70f) will **decrease** coverage by `0.01%`.
   > The diff coverage is `0.00%`.
   
   ```diff
   @@             Coverage Diff              @@
   ##               4.18    #7922      +/-   ##
   ============================================
   - Coverage     13.06%   13.05%   -0.01%     
   + Complexity     9093     9092       -1     
   ============================================
     Files          2720     2720              
     Lines        257431   257454      +23     
     Branches      40141    40143       +2     
   ============================================
   - Hits          33622    33620       -2     
   - Misses       219582   219608      +26     
   + Partials       4227     4226       -1     
   ```
   
   
   | [Files Changed](https://app.codecov.io/gh/apache/cloudstack/pull/7922?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...va/com/cloud/upgrade/dao/DatabaseAccessObject.java](https://app.codecov.io/gh/apache/cloudstack/pull/7922?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZW5naW5lL3NjaGVtYS9zcmMvbWFpbi9qYXZhL2NvbS9jbG91ZC91cGdyYWRlL2Rhby9EYXRhYmFzZUFjY2Vzc09iamVjdC5qYXZh) | `59.09% <0.00%> (-22.16%)` | :arrow_down: |
   | [...ain/java/com/cloud/upgrade/dao/DbUpgradeUtils.java](https://app.codecov.io/gh/apache/cloudstack/pull/7922?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZW5naW5lL3NjaGVtYS9zcmMvbWFpbi9qYXZhL2NvbS9jbG91ZC91cGdyYWRlL2Rhby9EYlVwZ3JhZGVVdGlscy5qYXZh) | `70.58% <0.00%> (-9.42%)` | :arrow_down: |
   | [...ava/com/cloud/upgrade/dao/Upgrade41800to41810.java](https://app.codecov.io/gh/apache/cloudstack/pull/7922?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZW5naW5lL3NjaGVtYS9zcmMvbWFpbi9qYXZhL2NvbS9jbG91ZC91cGdyYWRlL2Rhby9VcGdyYWRlNDE4MDB0bzQxODEwLmphdmE=) | `2.85% <0.00%> (-0.07%)` | :arrow_down: |
   
   ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/apache/cloudstack/pull/7922/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] mlsorensen commented on a diff in pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "mlsorensen (via GitHub)" <gi...@apache.org>.
mlsorensen commented on code in PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#discussion_r1309204284


##########
engine/schema/src/main/java/com/cloud/upgrade/dao/DatabaseAccessObject.java:
##########
@@ -84,6 +85,31 @@ public boolean columnExists(Connection conn, String tableName, String columnName
         return columnExists;
     }
 
+    public void addIndexIfNeeded(Connection conn, String tableName, String columnName) {

Review Comment:
   Yeah, I was thinking about that but then I have to create a separate index naming function to tie them together, etc. I guess I was just being lazy.



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] blueorangutan commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1700489495

   @weizhouapache a [SF] Trillian-Jenkins matrix job (centos7 mgmt + xenserver71, rocky8 mgmt + vmware67u3, 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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] blueorangutan commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1699496012

   Packaging result [SF]: :heavy_check_mark: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: el9 :heavy_check_mark: debian :heavy_multiplication_x: suse15. SL-JID 6942


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] blueorangutan commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1700425471

   @weizhouapache a [SF] 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.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] blueorangutan commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1698548792

   Packaging result [SF]: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: el9 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 6939


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] mlsorensen commented on a diff in pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "mlsorensen (via GitHub)" <gi...@apache.org>.
mlsorensen commented on code in PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#discussion_r1309212546


##########
engine/schema/src/main/java/com/cloud/upgrade/dao/DatabaseAccessObject.java:
##########
@@ -84,6 +85,31 @@ public boolean columnExists(Connection conn, String tableName, String columnName
         return columnExists;
     }
 
+    public void addIndexIfNeeded(Connection conn, String tableName, String columnName) {

Review Comment:
   Before I do this rework @weizhouapache, looking for feedback. Do we want to:
   
   1) create index naming function to generate index name
   2) call indexExists with tableName and indexName
   3) call createIndex with tableName and indexName
   
   It seems to me we would prefer to keep the index naming scheme private to outside callers so it isn't something overridable and no potential for mismatches between checking existence and creating - which leads me to think we would want to retain a single entry point function like the `addIndexIfNeeded` to generate the name privately and then call `indexExists` and `createIndex` if we still want to break these out.



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] mlsorensen commented on a diff in pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "mlsorensen (via GitHub)" <gi...@apache.org>.
mlsorensen commented on code in PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#discussion_r1309360436


##########
engine/schema/src/main/java/com/cloud/upgrade/dao/DatabaseAccessObject.java:
##########
@@ -84,6 +85,31 @@ public boolean columnExists(Connection conn, String tableName, String columnName
         return columnExists;
     }
 
+    public void addIndexIfNeeded(Connection conn, String tableName, String columnName) {

Review Comment:
   Cool, see update @weizhouapache. Tested this as well.



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] weizhouapache commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1700424250

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

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] weizhouapache commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1700799148

   code lgtm
   
   This will be merged when trillian tests finish.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] blueorangutan commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1701537875

   <b>[SF] Trillian test result (tid-7613)</b>
   Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 38117 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7922-t7613-xenserver-71.zip
   Smoke tests completed. 108 look OK, 0 have errors, 0 did not run
   Only failed and skipped 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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] blueorangutan commented on pull request #7922: Add index on cluster_details.name for FirstFitPlanner speedup

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7922:
URL: https://github.com/apache/cloudstack/pull/7922#issuecomment-1701914735

   <b>[SF] Trillian test result (tid-7614)</b>
   Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server r8
   Total time taken: 57667 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7922-t7614-vmware-67u3.zip
   Smoke tests completed. 105 look OK, 3 have errors, 0 did not run
   Only failed and skipped tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_02_upgrade_kubernetes_cluster | `Failure` | 571.07 | test_kubernetes_clusters.py
   test_01_deploy_vm_on_specific_host | `Error` | 21.22 | test_vm_deployment_planner.py
   test_02_deploy_vm_on_specific_cluster | `Error` | 3602.15 | test_vm_deployment_planner.py
   test_03_deploy_vm_on_specific_pod | `Error` | 3.42 | test_vm_deployment_planner.py
   test_04_deploy_vm_on_host_override_pod_and_cluster | `Error` | 1.35 | test_vm_deployment_planner.py
   test_05_deploy_vm_on_cluster_override_pod | `Error` | 2.30 | test_vm_deployment_planner.py
   test_09_expunge_vm | `Failure` | 424.64 | test_vm_life_cycle.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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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