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

feat(core): Make cache configuration configurable at runtime #3346

Closed
wants to merge 4 commits into from

Conversation

tusharxoxoxo
Copy link

Type of Change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring
  • Dependency updates
  • Documentation
  • CI/CD

Description

Additional Changes

  • This PR modifies the API contract
  • This PR modifies the database schema
  • This PR modifies application configuration/environment variables

Motivation and Context

#3276

How did you test it?

Checklist

  • I formatted the code cargo +nightly fmt --all
  • I addressed lints thrown by cargo clippy
  • I reviewed the submitted code
  • I added unit tests for my changes where possible
  • I added a CHANGELOG entry if applicable

@tusharxoxoxo tusharxoxoxo requested a review from a team as a code owner January 13, 2024 21:26
@tusharxoxoxo
Copy link
Author

  1. I think I should also change the values in crates/storage_impl/src/reddis/cache.rs but it is throwing an error whenever I try to change it, can someone guide me on how this could be done

  2. My LSP is showing this error, even when I haven't made any changes to the code base
    image

@tusharxoxoxo
Copy link
Author

@dracarys18

@dracarys18
Copy link
Member

Can you run

cargo c 

and check the error

@dracarys18
Copy link
Member

Regarding change this in cache.rs, since this is a static we can't make it completely runtime configurable. Instead you can make consts like MAX_CAPACITY a static, You can refer

KMS_CLIENT.get_or_init(|| KmsClient::new(config)).await
. This is ideally how we should do it

@SanchithHegde SanchithHegde added A-framework Area: Framework S-waiting-on-author Status: This PR is incomplete or needs to address review comments C-refactor Category: Refactor labels Jan 21, 2024
@tusharxoxoxo
Copy link
Author

by doing cargo c, I do not see any error
image

@tusharxoxoxo
Copy link
Author

I will change my PR according to the way u mentioned above

@tusharxoxoxo
Copy link
Author

in issue #3276, max_capacity was taken u32 but when it's used in crates/storage_impl/src/reddis/cache.rs it is u64, should I leave it as it is or make changes to it, either make both of them u64?
image
image

const MAX_CAPACITY: u64 = 30;
is already declared static in cache.rs at the top

@dracarys18
Copy link
Member

in issue #3276, max_capacity was taken u32 but when it's used in crates/storage_impl/src/reddis/cache.rs it is u64, should I leave it as it is or make changes to it, either make both of them u64? image image

const MAX_CAPACITY: u64 = 30; is already declared static in cache.rs at the top

You can keep it u64 as moka internally want you to pass this type in max_capacity

@SanchithHegde SanchithHegde removed the S-waiting-on-author Status: This PR is incomplete or needs to address review comments label Feb 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-framework Area: Framework C-refactor Category: Refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants