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.

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

Cleanups of the storage definitions. #2979

Merged
merged 12 commits into from
Nov 30, 2024

Conversation

MathieuDutSik
Copy link
Contributor

@MathieuDutSik MathieuDutSik commented Nov 28, 2024

Motivation

In PR #2896 it was suggested to move the definition of the stores to mod.rs. Ultimately, this is not what is done in this PR as there was disagreement.

Proposal

The following was done:

  • Simplify out the relevant code from the stores, in particular the LocalStackTestContext that is no longer used.
  • Remove unused enum in the error types, that is MissingDatabase and AlreadyExistingDatabase.
  • Introduce constructor for the RocksDbStoreConfig and similar.
  • Remove the pub attribute when not needed.
  • Correct the mis-attribution "lru splitting" into "lru caching".
  • Add a PathWithStorage function and remove another function.
  • Put the constructor definition of the VISIBLE_MAX_VALUE_SIZE for DynbamoDb and remove the test that simply reproduce the formula.
  • The InvalidTableName was renamed as InvalidNamespace.

Test Plan

CI as the functionality should stay exactly the same.

Release Plan

Not relevant.

Links

None.

@MathieuDutSik MathieuDutSik marked this pull request as ready for review November 28, 2024 11:41
Copy link
Contributor

@afck afck left a comment

Choose a reason for hiding this comment

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

I like the simplifications using the constructors (like DynamoDbStoreConfig::new). 👍

But I'd prefer to keep types specific to different backends in different modules, personally.

#[cfg(with_scylladb)]
pub type ScyllaDbStoreError = crate::value_splitting::ValueSplittingError<
crate::backends::scylla_db::ScyllaDbStoreInternalError,
>;
Copy link
Contributor

Choose a reason for hiding this comment

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

We could still have these in separate submodules and re-export them, to reduce the number of cfg statements.

In general I'm not sure if moving these directly into backends is an improvement.

@MathieuDutSik
Copy link
Contributor Author

I like the simplifications using the constructors (like DynamoDbStoreConfig::new). 👍

But I'd prefer to keep types specific to different backends in different modules, personally.

Ok, we will have to see what @ma2bd thinks as it was his initial suggestion. I like the separation.

@MathieuDutSik MathieuDutSik changed the title Move the statements of the store into mod.rs Cleanups of the storage definitions. Nov 29, 2024
@MathieuDutSik
Copy link
Contributor Author

I like the simplifications using the constructors (like DynamoDbStoreConfig::new). 👍

But I'd prefer to keep types specific to different backends in different modules, personally.

That is indeed a little problematic. So, I revamped the PR by focusing on less controversial issues that hopefully could be accepted more easily.

@MathieuDutSik MathieuDutSik merged commit 54a0e82 into linera-io:main Nov 30, 2024
23 checks passed
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