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

Implementing a setting cache #19

Merged
merged 1 commit into from
Aug 25, 2023
Merged

Conversation

MohamedBassem
Copy link
Contributor

@MohamedBassem MohamedBassem commented Jul 26, 2023

Implementing a setting cache

ghstack-source-id: c906f41fd517a0021667280bf1bed438bf93e9e2
Pull Request resolved: #19

MohamedBassem added a commit that referenced this pull request Jul 26, 2023
ghstack-source-id: fdf297412b4269faa6477276e29681731bbb730b
Pull Request resolved: #19
MohamedBassem added a commit that referenced this pull request Jul 26, 2023
ghstack-source-id: 6eb38c1dc3680979c2b0d194bc98d999987d9d8d
Pull Request resolved: #19
@MohamedBassem MohamedBassem marked this pull request as ready for review July 26, 2023 17:07
MohamedBassem added a commit that referenced this pull request Jul 26, 2023
ghstack-source-id: 8ebbe2102e6a91674f9c3efa40592b84eff5d246
Pull Request resolved: #19
MohamedBassem added a commit that referenced this pull request Aug 6, 2023
ghstack-source-id: 763034e39c6dde58d8a6f64e5d8559072d1ebda7
Pull Request resolved: #19
Copy link
Contributor

@AhmedSoliman AhmedSoliman left a comment

Choose a reason for hiding this comment

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

A few comments and thoughts but generally looks peaceful.

cronback-lib/project_settings.rs Show resolved Hide resolved
cronback-lib/project_settings.rs Outdated Show resolved Hide resolved
cronback-lib/project_settings.rs Outdated Show resolved Hide resolved
/// the failed Result. However, the value will get refetched on the next read
/// attempt (even if it didn't exceed the TTL).
/// 4. Cache evictions happen in the background in a separate thread pool.
pub struct ProjectSetting<V, F> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be plural instead, i.e. ProjectSettings? The current name implies that users can get an individual setting for a given project.

Additionally, I wonder why is this generic over the value type if this has a single purpose of caching project settings?

Copy link
Contributor Author

@MohamedBassem MohamedBassem Aug 12, 2023

Choose a reason for hiding this comment

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

The current name implies that users can get an individual setting for a given project.

It is actually exactly that. As of now, there's no entity called "project settings". My idea was to keep them disaggregated such that if you want to get only the project status and not the project notification settings you can do that. That's why it's generic over the value. The value here will be the piece of the setting that you'll fetch from the project, and the fetcher will be the lambda that gets a metadata client and calls the corresponding metadata API on it.

type NotificationSettings = ProjectSetting<ProjectNotificationSetting, Fn(&ProjectId, &MetadataClient) -> ProjectNotificationSetting>;

type ProjectStatus = ProjectSetting<ProjectStatus, Fn(&ProjectId, &MetadataClient) -> ProjectStatus>;

Hope that makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting approach.

cronback-lib/Cargo.toml Show resolved Hide resolved
cronback-lib/project_settings.rs Outdated Show resolved Hide resolved
MohamedBassem added a commit that referenced this pull request Aug 12, 2023
ghstack-source-id: c906f41fd517a0021667280bf1bed438bf93e9e2
Pull Request resolved: #19
AhmedSoliman
AhmedSoliman previously approved these changes Aug 13, 2023
MohamedBassem added a commit that referenced this pull request Aug 25, 2023
ghstack-source-id: c906f41fd517a0021667280bf1bed438bf93e9e2
Pull Request resolved: #19
@MohamedBassem MohamedBassem changed the base branch from gh/MohamedBassem/1/base to main August 25, 2023 23:01
@MohamedBassem MohamedBassem dismissed AhmedSoliman’s stale review August 25, 2023 23:01

The base branch was changed.

ghstack-source-id: c906f41fd517a0021667280bf1bed438bf93e9e2
Pull Request resolved: #19
@MohamedBassem MohamedBassem merged commit ec779f8 into main Aug 25, 2023
3 checks passed
@MohamedBassem MohamedBassem deleted the gh/MohamedBassem/1/head branch August 25, 2023 23:10
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