Re: [PR] Add batch deletion support to `removeRawUsageRecords` [cloudstack]
winterhazel commented on code in PR #12522:
URL: https://github.com/apache/cloudstack/pull/12522#discussion_r2754432664
##
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##
@@ -527,7 +527,7 @@ public class ConfigurationManagerImpl extends ManagerBase
implements Configurati
public static final ConfigKey DELETE_QUERY_BATCH_SIZE = new
ConfigKey<>("Advanced", Long.class, "delete.query.batch.size", "0",
"Indicates the limit applied while deleting entries in bulk. With
this, the delete query will apply the limit as many times as necessary," +
" to delete all the entries. This is advised when
retaining several days of records, which can lead to slowness. <= 0 means that
no limit will " +
-"be applied. Default value is 0. For now, this is used for
deletion of vm & volume stats only.", true);
+"be applied. Default value is 0. For now, this is used for
deletion of VM stats, volume stats, and usage records.", true);
Review Comment:
@DaanHoogland at least 500,000 by default seems like a good option for me. I
did some testing in a test environment with MariaDB and 80 million entries over
a laptop. Each stats/usage records batch took at most 5 seconds to delete, and
did not reach any timeouts. We need to validate how MySQL would perform, but
even 1,000,000 may be reasonable.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Add batch deletion support to `removeRawUsageRecords` [cloudstack]
winterhazel commented on code in PR #12522:
URL: https://github.com/apache/cloudstack/pull/12522#discussion_r2754432664
##
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##
@@ -527,7 +527,7 @@ public class ConfigurationManagerImpl extends ManagerBase
implements Configurati
public static final ConfigKey DELETE_QUERY_BATCH_SIZE = new
ConfigKey<>("Advanced", Long.class, "delete.query.batch.size", "0",
"Indicates the limit applied while deleting entries in bulk. With
this, the delete query will apply the limit as many times as necessary," +
" to delete all the entries. This is advised when
retaining several days of records, which can lead to slowness. <= 0 means that
no limit will " +
-"be applied. Default value is 0. For now, this is used for
deletion of vm & volume stats only.", true);
+"be applied. Default value is 0. For now, this is used for
deletion of VM stats, volume stats, and usage records.", true);
Review Comment:
At least 500,000 by default seems like a good option for me. I did some
testing in a test environment with MariaDB over a laptop. Each stats/usage
records batch took at most 5 seconds to delete, and did not reach any timeouts.
We need to validate how MySQL would perform, but even 1,000,000 may be
reasonable.
##
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##
@@ -527,7 +527,7 @@ public class ConfigurationManagerImpl extends ManagerBase
implements Configurati
public static final ConfigKey DELETE_QUERY_BATCH_SIZE = new
ConfigKey<>("Advanced", Long.class, "delete.query.batch.size", "0",
"Indicates the limit applied while deleting entries in bulk. With
this, the delete query will apply the limit as many times as necessary," +
" to delete all the entries. This is advised when
retaining several days of records, which can lead to slowness. <= 0 means that
no limit will " +
-"be applied. Default value is 0. For now, this is used for
deletion of vm & volume stats only.", true);
+"be applied. Default value is 0. For now, this is used for
deletion of VM stats, volume stats, and usage records.", true);
Review Comment:
@DaanHoogland at least 500,000 by default seems like a good option for me. I
did some testing in a test environment with MariaDB over a laptop. Each
stats/usage records batch took at most 5 seconds to delete, and did not reach
any timeouts. We need to validate how MySQL would perform, but even 1,000,000
may be reasonable.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Add batch deletion support to `removeRawUsageRecords` [cloudstack]
borisstoyanov merged PR #12522: URL: https://github.com/apache/cloudstack/pull/12522 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add batch deletion support to `removeRawUsageRecords` [cloudstack]
blueorangutan commented on PR #12522: URL: https://github.com/apache/cloudstack/pull/12522#issuecomment-3814375571 [SF] Trillian test result (tid-15316) Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8 Total time taken: 53815 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr12522-t15316-kvm-ol8.zip Smoke tests completed. 141 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add batch deletion support to `removeRawUsageRecords` [cloudstack]
blueorangutan commented on PR #12522: URL: https://github.com/apache/cloudstack/pull/12522#issuecomment-3809464929 @borisstoyanov a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add batch deletion support to `removeRawUsageRecords` [cloudstack]
borisstoyanov commented on PR #12522: URL: https://github.com/apache/cloudstack/pull/12522#issuecomment-3809456816 @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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add batch deletion support to `removeRawUsageRecords` [cloudstack]
blueorangutan commented on PR #12522: URL: https://github.com/apache/cloudstack/pull/12522#issuecomment-3807697924 Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16569 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add batch deletion support to `removeRawUsageRecords` [cloudstack]
blueorangutan commented on PR #12522: URL: https://github.com/apache/cloudstack/pull/12522#issuecomment-3807477056 @winterhazel a [SL] 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add batch deletion support to `removeRawUsageRecords` [cloudstack]
winterhazel commented on PR #12522: URL: https://github.com/apache/cloudstack/pull/12522#issuecomment-3807471570 @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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add batch deletion support to `removeRawUsageRecords` [cloudstack]
winterhazel commented on code in PR #12522:
URL: https://github.com/apache/cloudstack/pull/12522#discussion_r2733266346
##
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##
@@ -527,7 +527,7 @@ public class ConfigurationManagerImpl extends ManagerBase
implements Configurati
public static final ConfigKey DELETE_QUERY_BATCH_SIZE = new
ConfigKey<>("Advanced", Long.class, "delete.query.batch.size", "0",
"Indicates the limit applied while deleting entries in bulk. With
this, the delete query will apply the limit as many times as necessary," +
" to delete all the entries. This is advised when
retaining several days of records, which can lead to slowness. <= 0 means that
no limit will " +
-"be applied. Default value is 0. For now, this is used for
deletion of vm & volume stats only.", true);
+"be applied. Default value is 0. For now, this is used for
deletion of VM stats, volume stats, and usage records.", true);
Review Comment:
Yes, I think it would be good to enable batch deletion by default. 10,000 is
too low and might end up slowing down deletion too much though. I've seen
`vm_stats` with over 100 million entries for deletion, which would take too
long with batches of 10,000. I will check how some different values behave
later.
By the way, I'm putting this in draft because I noticed an issue in the code.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Add batch deletion support to `removeRawUsageRecords` [cloudstack]
DaanHoogland commented on code in PR #12522:
URL: https://github.com/apache/cloudstack/pull/12522#discussion_r2732463581
##
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##
@@ -527,7 +527,7 @@ public class ConfigurationManagerImpl extends ManagerBase
implements Configurati
public static final ConfigKey DELETE_QUERY_BATCH_SIZE = new
ConfigKey<>("Advanced", Long.class, "delete.query.batch.size", "0",
"Indicates the limit applied while deleting entries in bulk. With
this, the delete query will apply the limit as many times as necessary," +
" to delete all the entries. This is advised when
retaining several days of records, which can lead to slowness. <= 0 means that
no limit will " +
-"be applied. Default value is 0. For now, this is used for
deletion of vm & volume stats only.", true);
+"be applied. Default value is 0. For now, this is used for
deletion of VM stats, volume stats, and usage records.", true);
Review Comment:
should we make the default some biug value to protect the operator from
trying to delete 50+ records? (i am thinking about 1)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Add batch deletion support to `removeRawUsageRecords` [cloudstack]
codecov[bot] commented on PR #12522: URL: https://github.com/apache/cloudstack/pull/12522#issuecomment-3801505192 ## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/12522?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report :white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 4.03%. Comparing base ([`7536516`](https://app.codecov.io/gh/apache/cloudstack/commit/7536516e41636c9ae77ddda89c4f9827bfe55ba5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c303a18`](https://app.codecov.io/gh/apache/cloudstack/commit/c303a18b05919526aa7f0753d2cbb76740bf3f72?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)). :warning: Report is 3 commits behind head on 4.20. Additional details and impacted files ```diff @@Coverage Diff@@ ##4.20 #12522 +/- ## = - Coverage 4.03%4.03% -0.01% = Files402 402 Lines 3271532721 +6 Branches5831 5832 +1 = Hits1319 1319 - Misses 3124131247 +6 Partials 155 155 ``` | [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/12522/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | |---|---|---| | [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/12522/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.03% <ø> (-0.01%)` | :arrow_down: | | [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/12522/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more. [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/cloudstack/pull/12522?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :rocket: New features to boost your workflow: - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems. - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
