backport: usage: widen usage_volume unique key to include vm_id (#13909) - #13933
backport: usage: widen usage_volume unique key to include vm_id (#13909)#13933Alpha162 wants to merge 1 commit into
Conversation
Backport of apache#13909 to 4.22, targeting 4.22.2. This also creates the 4.22.1.0 to 4.22.2.0 upgrade path, which didn't exist yet: schema-42210to42220.sql, its cleanup companion, the Upgrade42210to42220 class, and the registration in DatabaseUpgradeChecker. The CALLs can't go into schema-42200to42210.sql instead, because every 4.22.1.0 install has already run that file and those are the affected clusters.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.22 #13933 +/- ##
=========================================
Coverage 17.77% 17.77%
- Complexity 15982 15983 +1
=========================================
Files 5928 5929 +1
Lines 534301 534309 +8
Branches 65382 65382
=========================================
+ Hits 94987 94989 +2
- Misses 428568 428574 +6
Partials 10746 10746
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Also worth a |
|
@blueorangutan package |
|
@weizhouapache 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. |
3 participants
Description
Backport of #13909 to
4.22, per @winterhazel's guidance on that PR, targeting 4.22.2 as @abh1sar flagged. The DDL is identical to what merged on main.It also creates the 4.22.1.0 to 4.22.2.0 upgrade path, which didn't exist yet. The chain in
DatabaseUpgradeCheckerended atUpgrade42200to42210, and since the branch is already4.22.2.0-SNAPSHOT, 4.22.2.0 is the declared next version. Nothing had needed a schema change on 4.22 since 4.22.1.0 was cut, so nobody had reason to add it.The CALLs can't go into
schema-42200to42210.sqlinstead. Every 4.22.1.0 install has already run that file, and those are the affected clusters. The-cleanup.sqlhas no statements in it, but the file has to exist:getCleanupScripts()throws if it's absent.See #13909 for the note on NULL semantics in the widened key, and on this preventing the problem rather than remediating clusters that already have it.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Not tested end to end. The environment where this was found has since been rebuilt, so I can't exercise a 4.22.1.0 to 4.22.2.0 upgrade myself.
What I can attest to:
UNIQUE KEY id (volume_id, created)was confirmed bySHOW CREATE TABLEagainst a restored backup of a live 4.22.1.0 install, alongside thevm_idcolumn added by Track volume usage data at a vm granularity as well #11531.idis set explicitly byschema-41600to41610.sql:72, so the drop targets a name present on any install that has passed through 4.16.1.0.CONTINUE HANDLERs (1091 and 1061), so a re-run or a resumed upgrade is a no-op.The new upgrade path is the part that genuinely wants a reviewer with an environment.
Upgrade42210to42220is the first 4.22.1 to 4.22.2 step on this branch, and its script filenames are derived fromgetUpgradableVersionRange()rather than declared, so the naming is mechanically correct. But nobody has run a real upgrade through it. I'd appreciate someone doing that before merge.How did you try to break this feature and the system with this change?
Reasoned through the failure modes rather than executing them, since I have no environment to run an upgrade in.
Upgrade42200to42210runs first and addsvm_idwith the narrow key, thenUpgrade42210to42220widens it. Both populations land correct.IDEMPOTENT_DROP_INDEXdrops the three-column key andIDEMPOTENT_ADD_UNIQUE_INDEXrecreates it identically, so the net effect is the same.getCleanupScripts()throwsCloudRuntimeExceptionwhen the file is absent, so that fails the upgrade rather than silently skipping it. Both script names derive fromgetUpgradableVersionRange(), so they can't drift from the class referencing them.