-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SWATCH-2286: Change Billable Usage Remittance PK to UUID #3153
Conversation
For reviewers, the following item cannot be done because the result is an aggregation of billable usages, so we cannot provide the UUID of a single billable usage.
|
About "Rollout plan needs to make sure this work does not try to deploy while billable usage is being calculated. ", the previous billable usage messages (without the uuid), it should work as expected, simply the uuid will be null. Therefore, I don't think we need a rollout plan here. |
1688b3c
to
dd45b4c
Compare
@Sgitario Part of the done criteria states that "every existing billable usage remittance DB record has a UUID populated". Is there a part of the migration I am missing that takes care of this? |
Yes and no. I covered that part using the migration you see and I confirmed that populates the existing records, but after trying to fix the same migration for hsql, I accidentally removed the defaultComputedValue property... Will update the PR shortly. |
dd45b4c
to
bbdb56c
Compare
PR updated. |
/retest |
300f7ca
to
7eef78a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functions correctly, but I'd like to see a couple things addressed:
-
The first commit in this PR (downgrade postgresql instance to 12) should have some context in the commit message as to why this change was required. Does this relate to SWATCH-2286?
-
Is there a reason why the uuid is generated with @PrePersist instead of simply annotating the uuid field with
@GeneratedValue(strategy = GenerationType.AUTO)
? Using @GeneratedValue seems a little cleaner IMO.
I had to downgrade the version we're using of postgresql to use the same version than in the stage,prod and ephemeral environments. Before, we were using Postgresql 13 which had some functions to generate UUID that are not by default in Postgresql 12, so I could not spot this when building locally. Using the Postgresql 12 always is safer.
Change the primary key of the to the billable usage remittance records to a UUID, plus existing uniqueness constraint should be maintained.
7eef78a
to
424753c
Compare
Added
At first, I tried with PR updated with these two changes. |
Has this upgrade been tested against a prod sized dataset to determine how long the migration might take? |
It was tested using Stage that has 90k records. Using the prod data that has 141k records took around 5-6 seconds in total:
|
/retest |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://drive.google.com/file/d/1xJp8NVKGCr-TpKTSBfhc5cHwLOO_gTP_/view?usp=drive_link
IQE Testing Steps
-
application.rhsm_subscriptions.create_mock_subscription( product_id=product_id, billing_provider=billing_provider, billing_account_id=billing_account_id, start_day=(datetime.date.today() - datetime.timedelta(days=10)).strftime("%Y-%m-%d"), )
-
application.rhsm_subscriptions.create_metering_event( product_id=product_id, **data )
-
application.rhsm_subscriptions.sync_tally_hourly( product_id=product_id, perform_metering=False, wait=True, wait_for_remittance=True )
-
application.rhsm_subscriptions.get_account_remittance( product_id=product_id, metricId=metric, billingProvider=billing_provider, billingAccountId=billing_account_id, )
UUID present in the remittance records.
Jira issue: SWATCH-2286
Description
Change the primary key of the to the billable usage remittance records to a UUID, plus existing uniqueness constraint should be maintained.
As part of this pull request, I also had to downgrade the version we're using of postgresql to use the same version than in the stage,prod and ephemeral environments. Before, we were using Postgresql 13 which had some functions to generate UUID that are not by default in Postgresql 12, so I could not spot this when building locally. Using the Postgresql 12 always is safer.
Testing
Setup
0.- podman-compose up
1.- ./gradlew liquibaseUpdate
2. Have some cost events from the past:
Steps
1.Start three apps:
swatch-core:
swatch-contract:
swatch-aws:
Verification
billable_usage_remittance
table with the new column uuid populated.