neondatabase / neon

Neon: Serverless Postgres. We separated storage and compute to offer autoscaling, code-like database branching, and scale to zero.
https://neon.tech
Apache License 2.0
14.48k stars 420 forks source link

Use serde for RemoteStorageConfig parsing #8126

Closed arpad-m closed 3 months ago

arpad-m commented 3 months ago

Adds a Deserialize impl to RemoteStorageConfig. We thus achieve the same as #7743 but with less repetitive code, by deriving Deserialize impls on S3Config, AzureConfig, and RemoteStorageConfig. The disadvantage is less useful error messages.

The git history of this PR contains a state where we go via an intermediate representation, leveraging the serde_json crate.

Also, the PR adds tests.

Alternative to #7743 .

github-actions[bot] commented 3 months ago

2910 tests run: 2793 passed, 0 failed, 117 skipped (full report)


Flaky tests (1) #### Postgres 14 - `test_subscriber_restart`: [release](https://neon-github-public-dev.s3.amazonaws.com/reports/pr-8126/9627108790/index.html#suites/8be0c222d5601535470e7e5978bbfb03/bfa456aa02764979/retries)

Code coverage* (full report)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
ecfc030f84033ff58b5e812b42d626b5e7b16bf0 at 2024-06-22T18:05:31.382Z :recycle:
arpad-m commented 3 months ago

I decided against derive tricks because the custom logic in RemoteStorageConfig::from_toml seemed daunting.

yeah tbh serialization derives are poorly documented and it's not very easy to inspect the resulting format/schema. This PR for example (probably) regresses error messages, but I think it's okay as we use derives everywhere so it's a sunken cost.

I filed this PR because I didn't like the additional code that #7743 meant. serde is really not good for writing manual (de)serializations, the code is less maintainable. Now it's way easier to add additional fields.

Does Serialize also work? I need that in https://github.com/neondatabase/neon/pull/7656

Oh that's interesting. Why do you need Serialize there? I think it's easy to add it.

problame commented 3 months ago

Oh that's interesting. Why do you need Serialize there? I think it's easy to add it.

Ok, I'll add it in https://github.com/neondatabase/neon/pull/7656 then