Skip to content

cache plan name#6338

Merged
pjain1 merged 14 commits intomainfrom
cache_billing_plan
Feb 18, 2025
Merged

cache plan name#6338
pjain1 merged 14 commits intomainfrom
cache_billing_plan

Conversation

@pjain1
Copy link
Copy Markdown
Member

@pjain1 pjain1 commented Jan 2, 2025

Fixes https://github.com/rilldata/rill-private-issues/issues/973

  • Cache and update plan name on performing any subscription related action
  • Cached plan name is returned as part of GetOrganization rpc
  • GetBillingSubscription does not use cache always fetches afresh so should not be used on each page load but only on billing page
  • For handling external changes (directly done in Orb, although it should not be done), webhook are used to update the cache. They can be removed if we are ok with some staleness when external modifications are done.

@pjain1 pjain1 requested a review from begelundmuller January 2, 2025 06:26
@pjain1 pjain1 requested a review from begelundmuller February 6, 2025 11:07
@@ -0,0 +1 @@
ALTER TABLE orgs ADD COLUMN cached_plan_display_name TEXT;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Can we call this billing_plan_display_name? It's nice to prefix with billing_plan and cached feels redundant (I think it's understood that this gets synced from Orb)
  2. Could we also include the unique name or ID of the plan from Orb? E.g. billing_plan_id or billing_plan_name?
  3. Consider using NOT NULL DEFAULT '' to avoid handling pointers (null plan name and empty plan name is effectively the same, no need for additional states).

Copy link
Copy Markdown
Member Author

@pjain1 pjain1 Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. ok
  2. ok

null plan name and empty plan name is effectively the same, no need for additional states

I want to be able to distinguish between two as there are states when there will be no plan - for example when org is just created or when org subscription ended. In these cases plan name will be empty and thus we will know that there is no plan and avoid calling orb to fetch plan name again and again on each call to GetOrganization API. Otherwise if the plan name is null then this means that its not cached yet.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so it's to build a bridge for old rows, which won't initially have the plan stored? It's ugly but makes sense. It definitely needs a docstring about that difference though, it's not easy to guess that null means not loaded and "" means no plan.

PaymentCustomerID: org.PaymentCustomerID,
BillingEmail: org.BillingEmail,
CreatedByUserID: org.CreatedByUserID,
CachedPlanDisplayName: &sub.Plan.DisplayName,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it guaranteed to have a display name? If it may be empty (e.g. in case we forgot to set one in Orb), it should perhaps fall back to the unique name or ID instead (so it doesn't look like no plan is assigned).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plan display name is not set in orb at all, its generated logically as per product requirement here -https://github.com/rilldata/rill/blob/main/admin/billing/orb.go#L636

@pjain1
Copy link
Copy Markdown
Member Author

pjain1 commented Feb 16, 2025

lint job failing with

Failed executing command with error: the configuration contains invalid elements

looks like nlreturn is causing issues for govet its not valid anymore, as per schema here - https://golangci-lint.run/jsonschema/golangci.jsonschema.json

Copy link
Copy Markdown
Contributor

@begelundmuller begelundmuller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pjain1 pjain1 merged commit d1e8771 into main Feb 18, 2025
11 of 12 checks passed
@pjain1 pjain1 deleted the cache_billing_plan branch February 18, 2025 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants