-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
chore(data-warehouse): dw billing #18168
Conversation
…dw-airbyte-billing
…dw-airbyte-billing
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.
A couple optimizations (maybe?) but I think this all makes sense!
posthog/tasks/warehouse.py
Outdated
def check_external_data_source_billing_limit_by_team(team_id: int) -> None: | ||
from posthog.warehouse.external_data_source.connection import deactivate_connection_by_id, activate_connection_by_id | ||
from ee.billing.quota_limiting import list_limited_team_tokens, QuotaResource | ||
|
||
limited_teams_rows_synced = list_limited_team_tokens(QuotaResource.ROWS_SYNCED) | ||
|
||
team = Team.objects.get(pk=team_id) | ||
all_active_connections = ExternalDataSource.objects.filter(team=team, status__in=["running", "succeeded"]) | ||
all_inactive_connections = ExternalDataSource.objects.filter(team=team, status="inactive") | ||
|
||
# TODO: consider more boundaries | ||
if team_id in limited_teams_rows_synced: |
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.
If I'm understanding this correctly, this only runs every two hours. But the quota_limiting tasks run every 30 mins. So we could theoretically up to 2 hours behind in turning off their syncs. Is there a reason to do it here with the task that runs every 2 hours instead of when we check the current usage against billing limits every 30 mins?
In other words, as soon as we realize in quota_limiting.py
that we should be turning them off, why don't we do it then?
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.
yeah we could, then there would be no need for the tokens. I was trying to follow the pattern where we set the teams that will be limited and then use the token to determine which teams to limit (like how events and recordings are done)
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 up to you, either works 👍
Hey @EDsCODE! 👋 |
Problem
Changes
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
How did you test this code?