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

Add sortable notification cache variants #39616

Merged
merged 1 commit into from
Mar 27, 2024
Merged

Conversation

rudream
Copy link
Contributor

@rudream rudream commented Mar 20, 2024

Purpose

Part of #37704

Adds notification sortcaches in order to be able to sort notifications by date from newest to oldest in a performant way.

I recommend reviewing commit by commit.

Implementation

Two new sortcaches were added, a UserNotificationCache for user-specific notifications (this contains all user-specific notifications for all users), and a GlobalNotificationCache which contains all global notifications. I considered the option of having only one cache which contains all notifications (both user-specific and global) mixed together, however the drawback is that this wouldn't scale well with many users as it would mean that to fetch notifications for a user, we would need to iterate through possibly thousands of user-specific notifications for other users that are in the way.

The cache implementation follows a similar pattern to access_request_cache.go, leveraging the recently added sortcache helper.

The caches are sorted using their UUID's which means they will be already be lexicographically sorted by date. For user-specific notifications, the key is <username>/<notification uuid>, this is so that the notifications for a user will be grouped together when sorted.

We return a stream as opposed to a list when fetching notifications because it's a convenient way for us to construct a page one item at a time while pulling from both user-specific notifications and global notifications. The StreamXXXXNotifications() methods work by fetching the entire relevant dataset from the cache upfront (sorted from newest to oldest) and then returning a stream which iterates through it.

@rudream rudream added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v15 labels Mar 20, 2024
@rudream rudream requested review from fspmarshall and avatus March 20, 2024 11:34
@github-actions github-actions bot requested review from flyinghermit and r0mant March 20, 2024 11:35
@rudream rudream force-pushed the yassine/notifications/cache branch 3 times, most recently from 644bb1b to ce085aa Compare March 20, 2024 19:35
@rudream rudream requested a review from rosstimothy March 20, 2024 19:38
Copy link
Contributor

@rosstimothy rosstimothy left a comment

Choose a reason for hiding this comment

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

I'm not seeing anything in this PR that actually adds notifications to the cache. If I'm not mistaken this is only adding two new notification specific cache variants that will later consume notifications from the actual cache. Is that correct?

lib/services/notifications_cache.go Outdated Show resolved Hide resolved
api/proto/teleport/notifications/v1/notifications.proto Outdated Show resolved Hide resolved
lib/service/service.go Show resolved Hide resolved
lib/service/service.go Show resolved Hide resolved
lib/services/local/notifications.go Outdated Show resolved Hide resolved
lib/services/local/notifications.go Outdated Show resolved Hide resolved
lib/services/local/notifications.go Outdated Show resolved Hide resolved
lib/services/local/notifications.go Outdated Show resolved Hide resolved
lib/services/local/notifications.go Outdated Show resolved Hide resolved
lib/services/notifications_cache.go Outdated Show resolved Hide resolved
@rudream
Copy link
Contributor Author

rudream commented Mar 22, 2024

If I'm not mistaken this is only adding two new notification specific cache variants that will later consume notifications from the actual cache. Is that correct?

@rosstimothy Correct, notifications are added to the actual cache in this PR. I'll rename this PR to make it more clear.

@rudream rudream force-pushed the yassine/notifications/cache branch from ce085aa to 9bf270f Compare March 22, 2024 01:38
@flyinghermit flyinghermit removed their request for review March 22, 2024 01:45
@rudream rudream force-pushed the yassine/notifications/cache branch from 9bf270f to ac7d17a Compare March 22, 2024 02:46
@rudream rudream changed the title Add notifications to cache Add sortable notification cache variants Mar 22, 2024
@rudream rudream requested a review from rosstimothy March 22, 2024 03:15
Copy link
Contributor

@fspmarshall fspmarshall left a comment

Choose a reason for hiding this comment

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

LGTM with some minor efficiency/scalability nits.

lib/services/notifications_cache.go Outdated Show resolved Hide resolved
lib/services/notifications_cache.go Outdated Show resolved Hide resolved
lib/services/notifications.go Outdated Show resolved Hide resolved
lib/services/local/notifications.go Outdated Show resolved Hide resolved
lib/services/notifications_cache.go Outdated Show resolved Hide resolved
lib/services/notifications_cache.go Outdated Show resolved Hide resolved
lib/services/notifications_cache.go Outdated Show resolved Hide resolved
lib/services/notifications_cache_test.go Outdated Show resolved Hide resolved
lib/services/notifications_cache_test.go Outdated Show resolved Hide resolved
lib/services/notifications_cache_test.go Outdated Show resolved Hide resolved
lib/services/notifications_cache_test.go Outdated Show resolved Hide resolved
lib/services/notifications_cache_test.go Outdated Show resolved Hide resolved
@rudream rudream requested a review from rosstimothy March 27, 2024 11:50
lib/services/notifications_cache.go Outdated Show resolved Hide resolved
lib/services/notifications_cache.go Outdated Show resolved Hide resolved
lib/services/notifications_cache.go Outdated Show resolved Hide resolved
@rudream rudream force-pushed the yassine/notifications/cache branch from 7f9634e to cf73121 Compare March 27, 2024 13:23
@rudream rudream enabled auto-merge March 27, 2024 13:24
@rudream rudream added this pull request to the merge queue Mar 27, 2024
Merged via the queue into master with commit 4b4fa81 Mar 27, 2024
37 checks passed
@rudream rudream deleted the yassine/notifications/cache branch March 27, 2024 13:58
@public-teleport-github-review-bot

@rudream See the table below for backport results.

Branch Result
branch/v15 Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v15 no-changelog Indicates that a PR does not require a changelog entry size/lg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants