-
Notifications
You must be signed in to change notification settings - Fork 248
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
[batch] Stop writing to v2 billing tables #13892
Conversation
78fdfeb
to
f50b57d
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.
This is awesome! Just a couple sanity check questions. I'll give this a full second pass as well just to be extra careful.
batch/sql/estimated-current.sql
Outdated
INSERT INTO aggregated_billing_project_user_resources_v3 (billing_project, user, resource_id, token, `usage`) | ||
SELECT batches.billing_project, batches.`user`, | ||
attempt_resources.deduped_resource_id, | ||
attempt_resources.resource_id, |
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.
Can you remind me why this column name is changing?
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.
At this point resource_id
should always equal deduped_resource_id
with the previous fixes to how we insert resources to not be duplicated. I made this change here in case we do decide to swap the columns "deduped_resource_id" -> "resource_id" and drop the original resource id column (has duplicates in it). We can save that for a separate step if that's easier.
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.
Ok, that checks out but ya I think I works prefer just doing that separately if that's alright.
@@ -883,74 +825,25 @@ BEGIN | |||
SET cur_billing_date = CAST(UTC_DATE() AS DATE); | |||
|
|||
IF msec_diff_rollup != 0 THEN | |||
INSERT INTO aggregated_billing_project_user_resources_v2 (billing_project, user, resource_id, token, `usage`) | |||
INSERT INTO aggregated_billing_project_user_resources_v3 (billing_project, user, resource_id, token, `usage`) |
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.
And just checking here to make absolutely sure, resource_id
and deduped_resource_id
are the same in this table?
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.
In the billing tables, there is no deduped_resource_id. The v2 table is essentially the "resource_id" in attempt_resources and the v3 table "resource_id" is equivalent to the "deduped_resource_id" in the attempt resources table.
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.
Awesome, thanks!
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.
Awesome! I think I would rather separate out the rename but otherwise all good to go
e4853b5
to
b282abb
Compare
@daniel-goldstein Can you re-review this? I rebased on latest main. |
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.
@jigold Everything checks out to me
This PR modifies the billing triggers to stop writing to the v2 billing tables as well as remove the check for whether the equivalent v2 rows have been "migrated" when writing to the v3 tables. Stacked on #13891.