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

refactor: use serde for TenantConf deserialization Fixes: #5300 #5310

Merged
merged 12 commits into from
Nov 30, 2023
Merged

refactor: use serde for TenantConf deserialization Fixes: #5300 #5310

merged 12 commits into from
Nov 30, 2023

Conversation

rmodpur
Copy link
Contributor

@rmodpur rmodpur commented Sep 14, 2023 •
edited by koivunej
Loading

Remove handcrafted TenantConf deserialization code. Use serde_path_to_error to include the field which failed parsing. Leaves the duplicated TenantConf in pageserver and models, does not touch PageserverConf handcrafted deserialization.

Error change:

  • before change: "configure option checkpoint_distance cannot be negative"
  • after change: "checkpoint_distance: invalid value: integer -1, expected u64"

Fixes: #5300
Cc: #3682

koivunej
koivunej reviewed Sep 14, 2023
View reviewed changes
pageserver/src/config.rs Outdated Show resolved Hide resolved
koivunej
koivunej reviewed Sep 16, 2023
View reviewed changes
pageserver/src/config.rs Outdated Show resolved Hide resolved
@rmodpur rmodpur force-pushed the tenant-conf-validation branch from bc5e8b5 to 5ba1c13 Compare September 18, 2023 04:21
@rmodpur rmodpur marked this pull request as ready for review September 18, 2023 04:23
@rmodpur rmodpur requested review from a team as code owners September 18, 2023 04:23
@rmodpur rmodpur requested review from conradludgate and koivunej and removed request for a team September 18, 2023 04:23
@rmodpur rmodpur force-pushed the tenant-conf-validation branch from 5ba1c13 to 74fda81 Compare September 19, 2023 02:53
@rmodpur rmodpur force-pushed the tenant-conf-validation branch from 74fda81 to 006a358 Compare September 26, 2023 09:26
@rmodpur
Copy link
Contributor Author

rmodpur commented Sep 26, 2023

@koivunej could you pls review this ?

@rmodpur rmodpur force-pushed the tenant-conf-validation branch from 006a358 to 45061a8 Compare September 26, 2023 09:31
@koivunej koivunej self-assigned this Nov 6, 2023
@koivunej koivunej mentioned this pull request Nov 6, 2023
Closed
@shanyp shanyp added the approved-for-ci-run Changes are safe to trigger CI for the PR label Nov 6, 2023
@github-actions github-actions bot removed the approved-for-ci-run Changes are safe to trigger CI for the PR label Nov 6, 2023
@vipvap vipvap mentioned this pull request Nov 6, 2023
Closed
@shanyp shanyp added the approved-for-ci-run Changes are safe to trigger CI for the PR label Nov 28, 2023
@github-actions github-actions bot removed the approved-for-ci-run Changes are safe to trigger CI for the PR label Nov 28, 2023
@shanyp shanyp added the approved-for-ci-run Changes are safe to trigger CI for the PR label Nov 28, 2023
@github-actions github-actions bot removed the approved-for-ci-run Changes are safe to trigger CI for the PR label Nov 28, 2023
@shanyp shanyp self-assigned this Nov 28, 2023
@shanyp shanyp added the approved-for-ci-run Changes are safe to trigger CI for the PR label Nov 29, 2023
@github-actions github-actions bot removed the approved-for-ci-run Changes are safe to trigger CI for the PR label Nov 29, 2023
@shanyp shanyp added the approved-for-ci-run Changes are safe to trigger CI for the PR label Nov 29, 2023
@github-actions github-actions bot removed the approved-for-ci-run Changes are safe to trigger CI for the PR label Nov 29, 2023
@shanyp shanyp added the approved-for-ci-run Changes are safe to trigger CI for the PR label Nov 29, 2023
@github-actions github-actions bot removed the approved-for-ci-run Changes are safe to trigger CI for the PR label Nov 29, 2023
@neondatabase neondatabase deleted a comment from github-actions bot Nov 29, 2023
@koivunej
refactor: use models::TenantConfig::default()
ad1dad1
@koivunej
refactor: just unwrap
aae5190 
unwrapping in tests is always preferable to anything else; the panic
message will be much better than what assert! of a bool could be.
@shanyp shanyp added the approved-for-ci-run Changes are safe to trigger CI for the PR label Nov 29, 2023
@github-actions github-actions bot removed the approved-for-ci-run Changes are safe to trigger CI for the PR label Nov 29, 2023
@shanyp shanyp added the approved-for-ci-run Changes are safe to trigger CI for the PR label Nov 29, 2023
@github-actions github-actions bot removed the approved-for-ci-run Changes are safe to trigger CI for the PR label Nov 29, 2023
@neondatabase neondatabase deleted a comment from github-actions bot Nov 29, 2023
@shanyp shanyp changed the title use serde_path_to_error for deserializing Remove handcrafted deserialization code and use serde_path_to_error for detecting misconfiguration fields Nov 29, 2023
@shanyp shanyp requested a review from jcsp November 29, 2023 18:26
@shanyp shanyp changed the title Remove handcrafted deserialization code and use serde_path_to_error for detecting misconfiguration fields Remove handcrafted deserialization code and use serde_path_to_error for detecting misconfiguration fields Fix: #5300 Nov 29, 2023
@github-actions GitHub Actions
Copy link

2406 tests run: 2310 passed, 0 failed, 96 skipped (full report)

Flaky tests (5)

Postgres 16

  • test_ondemand_download_timetravel[mock_s3]: debug

Postgres 15

  • test_crafted_wal_end[last_wal_record_crossing_segment]: release
  • test_timeline_deletion_with_files_stuck_in_upload_queue: debug

Postgres 14

  • test_branching_with_pgbench[cascade-1-10]: debug
  • test_timeline_deletion_with_files_stuck_in_upload_queue: debug

Code coverage (full report)

  • functions: 54.4% (9189 of 16879 functions)
  • lines: 82.1% (53410 of 65071 lines)
The comment gets automatically updated with the latest test results
b26f4c8 at 2023-11-29T18:51:03.490Z :recycle:

@koivunej koivunej changed the title Remove handcrafted deserialization code and use serde_path_to_error for detecting misconfiguration fields Fix: #5300 refactor: use serde for TenantConf deserialization Nov 30, 2023
koivunej
koivunej approved these changes Nov 30, 2023
View reviewed changes
Copy link
Member

@koivunej koivunej left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We do have quite the few tenant conf deserialization related tests, so I think the code must work. I mentioned what is being left undone in the PR description.

@shanyp shanyp changed the title refactor: use serde for TenantConf deserialization refactor: use serde for TenantConf deserialization Fixes: #5300 Nov 30, 2023
@shanyp shanyp merged commit 50d959f into neondatabase:main Nov 30, 2023
68 of 70 checks passed
@shanyp
Copy link
Contributor

shanyp commented Nov 30, 2023

@rmodpur thank you for this contribution!

@rmodpur rmodpur deleted the tenant-conf-validation branch December 4, 2023 12:53
@koivunej koivunej mentioned this pull request Dec 13, 2023
Merged
koivunej added a commit that referenced this pull request Dec 15, 2023
@koivunej
fix: better Json parsing errors (#6135)
07508fb 
Before any json parsing from the http api only returned errors were per
field errors. Now they are done using `serde_path_to_error`, which at
least helped greatly with the `disk_usage_eviction_run` used for
testing. I don't think this can conflict with anything added in #5310.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@koivunej koivunej koivunej approved these changes

@conradludgate conradludgate Awaiting requested review from conradludgate conradludgate was automatically assigned from neondatabase/compute

@jcsp jcsp Awaiting requested review from jcsp

Assignees

@shanyp shanyp

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

pageserver: consistent validation of tenant conf
3 participants
@rmodpur @shanyp @koivunej