Usage parsers refactoring #11097
Usage parsers refactoring #11097
Conversation
DaanHoogland
requested review from
DaanHoogland,
Copilot and
winterhazel
and removed request for
winterhazel
|
thanks for this initiative @erikbocks , I’ll look at this (including doing some testing) |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors individual Usage parser classes to inherit from a new abstract UsageParser, replaces static methods/fields with instance methods and injected DAOs, unifies logging behavior, and simplifies UsageManagerImpl.parseHelperTables() by injecting and iterating a list of parser beans.
- All parser classes now extend
UsageParser, remove static parsing logic, and use an injectedUsageDao - Introduced common logging (
beforeParse) and standardized debug messages - Updated
UsageManagerImplto injectList<UsageParser>and loop through parsers instead of calling each statically
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
UsageParser.java |
New base class with doParsing(), beforeParse() and common usageDao |
UsageManagerImpl.java |
Injects List<UsageParser> and iterates in parseHelperTables() |
usage/src/main/java/com/cloud/usage/parser/*.java |
Each parser now extends UsageParser, removes static methods, replaces DAO calls, and switches to instance logger |
Comments suppressed due to low confidence (1)
usage/src/main/java/com/cloud/usage/parser/NetworksUsageParser.java:40
- [nitpick] The parser name is pluralized here, while most other parsers use a singular form. Consider using "Network" for consistency.
return "Networks";
usage/src/main/java/com/cloud/usage/parser/NetworksUsageParser.java
Outdated
Show resolved
Hide resolved
|
@blueorangutan package |
|
@DaanHoogland 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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11097 +/- ##
===========================================
Coverage 16.57% 16.58%
- Complexity 13868 13987 +119
===========================================
Files 5719 5745 +26
Lines 507178 510707 +3529
Branches 61571 62091 +520
===========================================
+ Hits 84085 84696 +611
- Misses 413674 416537 +2863
- Partials 9419 9474 +55
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13935 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
The code looks good. I think we can still do some more refactoring inside the I will do some testing too. |
|
[SF] Trillian test result (tid-13631)
|
|
[SF] Trillian test result (tid-13652)
|
DaanHoogland
left a comment
DaanHoogland
left a comment
There was a problem hiding this comment.
clgtm (with one improvement to propose.
tested (very coursely) and usage is generated as expected. On studying the logs, the only ERROR entry
2025-07-02 00:15:31,984 ERROR [cloud.usage.UsageManagerImpl_EnhancerByCloudStack_5fb70218] (Usage-Job-1:[]) (logid:) Found duplicate usage entry for volume: 365 assigned to account: 230; marking as deleted…
is actually due to a fix by @winterhazel
the remark on further improvement; at the moment the parsers are called in sequence. I think we can call them in parallel, in the future.
winterhazel
left a comment
winterhazel
left a comment
There was a problem hiding this comment.
Tested by running the Usage job and verifying the usage record creation. It is working as expected. Just one minor comment.
winterhazel
added
status:waiting-for-author
and removed
status:waiting-for-reviewer
status:waiting-for-smoketest-results
labels
winterhazel
added
status:ready-for-merge
and removed
status:waiting-for-author
status:ready-for-merge
labels
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14130 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-13762)
|
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-13780)
|
4 participants
Description
Currently, all Usage parsers implement a static method with an identical signature, which is invoked by the
UsageManagerImpl#parseHelperTables()method. This PR refactors these parser classes by standardizing them to extend theUsageParserclass and requiring an override of theparsemethod, improving code consistency, maintainability, and extensibility.Additionally, by leveraging Spring Dependency Injection, it was possible to provide a major cleanup in the
UsageManagerImpl#parseHelperTables()method, by injecting allUsageParserimplementations into a list, then iterating over it and calling theparsemethod. The parse log was also improved to provide better information about the parsing result, the object being parsed and the target account.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Screenshots (if appropriate):
How Has This Been Tested?
Update log example 1:
Footnotes
During the tests, I validated that the
UUIDcolumn ofcloud_usage.accountsis set to NULL after the accounts import from theclouddatabase. An issue was mapped to fix this behaviour (NULL value in uuid column of cloud_usage.account table #11077) ↩