Skip to content
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.

Sign up for GitHub

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

Jump to bottom

storcon: avoid storing pageserver_api::models::TenantConf's null fields in db & API output #9983

Open
problame opened this issue Dec 3, 2024 · 0 comments · May be fixed by #9987
Open

storcon: avoid storing pageserver_api::models::TenantConf's null fields in db & API output #9983

problame opened this issue Dec 3, 2024 · 0 comments · May be fixed by #9987
Labels
a/tech_debt Area: related to tech debt c/storage/controller Component: Storage Controller c/storage/pageserver Component: storage: pageserver

Comments

@problame
Copy link
Contributor

problame commented Dec 3, 2024 •
edited
Loading

Problem

Storcon currently uses the serde_json-serialized representation of pageserver_api::models::TenantConf to represent desired tenant config in storcon db.

The same type is also used in API responses.

Most often, all fields except pitr_interval are null, meaning fallback to the pagesever.toml's default tenant config value.

This is wasteful in terms of space usage but more importantly it's annoying for humans when they inspect a specific tenant, e.g. via storcon_cli tenant-describe because all these null values make it hard to spot the actual overrides.

Solution

Use skip_serializing_if(Option::is_none).

That would make the output a lot smaller, actually {} for most tenants.

Before doing this, we need to investigate whether this poses a compatibility problem.

Refs
@problame problame added c/storage/pageserver Component: storage: pageserver a/tech_debt Area: related to tech debt c/storage/controller Component: Storage Controller labels Dec 3, 2024
@problame problame mentioned this issue Dec 3, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Dec 3, 2024
@problame
storcon_cli tenant-describe: include tenant-wide information in output (
15d01b2 
#9899)

Before this PR, the storcon_cli didn't have a way to show the
tenant-wide information of the TenantDescribeResponse.

Sadly, the `Serialize` impl for the tenant config doesn't skip on
`None`, so, the output becomes a bit bloated.
Maybe we can use `skip_serializing_if(Option::is_none)` in the future.
=> #9983
@arpad-m arpad-m linked a pull request Dec 3, 2024 that will close this issue
Open
awarus pushed a commit that referenced this issue Dec 5, 2024
@problame @awarus
storcon_cli tenant-describe: include tenant-wide information in output (
8963ac8 
#9899)

Before this PR, the storcon_cli didn't have a way to show the
tenant-wide information of the TenantDescribeResponse.

Sadly, the `Serialize` impl for the tenant config doesn't skip on
`None`, so, the output becomes a bit bloated.
Maybe we can use `skip_serializing_if(Option::is_none)` in the future.
=> #9983
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
a/tech_debt Area: related to tech debt c/storage/controller Component: Storage Controller c/storage/pageserver Component: storage: pageserver
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

pageserver_api: add skip_serializing_none to TenantConfig neondatabase/neon
1 participant
@problame