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 sortcache helper #38391

Merged
merged 1 commit into from
Feb 23, 2024
Merged

add sortcache helper #38391

merged 1 commit into from
Feb 23, 2024

Conversation

fspmarshall
Copy link
Contributor

@fspmarshall fspmarshall commented Feb 19, 2024

Adds a new generic helper type sortcache, a tree-like cache that can store resources under an arbitrary number of sortable indexes that support direct lookups, as well as ascending and descending iteration of ranges. This helper is heavily based on the logic of the unified resource cache, and is intended to make it easier to build things like the unified resource cache in the future.

This helper is intended to support ongoing work in moving existing resources to paginated APIs. Some resources (namely access requests), require some forms of sort that are incompatible with how our backends currently work, but are also not really appropriate for inclusion in the existing unified resource cache.

@fspmarshall fspmarshall force-pushed the fspmarshall/sort-cache branch 4 times, most recently from 36d2a5f to 882db67 Compare February 20, 2024 17:07
@fspmarshall fspmarshall requested a review from avatus February 20, 2024 17:16
@fspmarshall fspmarshall marked this pull request as ready for review February 20, 2024 17:16
Copy link

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@fspmarshall fspmarshall added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v13 backport/branch/v14 backport/branch/v15 labels Feb 20, 2024
Copy link
Contributor

@avatus avatus left a comment

Choose a reason for hiding this comment

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

looks good to me with the context I have from unified resources. Awesome!

lib/utils/sortcache/sortcache.go Show resolved Hide resolved
c.values[c.counter] = value

for index, fn := range c.indexes {
key := fn(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

would it make sense to add some sort of validation to the key here? maybe something as simple as "make sure it isn't empty string"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried out some variations of this, but ended up reverting them. The sortcache itself doesn't care if you use empty keys (or any particular kind of key), and its actually pretty tricky to do this logic in such a way where it can fail mid insertion without leaving the cache in a weird state (at least not without pre-allocating all the keys in a temporary mapping, which would be unfortunate for performance).

I think instead, users of this helper that have specific requirements for their keys (e.g. no empty keys), should use the SortCache.KeyOf(...) method to see what keys their values map to and decide if they are acceptable before insertion. For the access-request usecase I'm not really worried about that (all indexes use the uuid as a suffix, and uuids are required to be non-empty already), and I'd therefore prefer not to pay the cost in extra allocations.

@fspmarshall fspmarshall force-pushed the fspmarshall/sort-cache branch from 2c3a246 to c29dd60 Compare February 23, 2024 19:04
@fspmarshall fspmarshall added this pull request to the merge queue Feb 23, 2024
Merged via the queue into master with commit 44ef266 Feb 23, 2024
34 checks passed
@fspmarshall fspmarshall deleted the fspmarshall/sort-cache branch February 23, 2024 19:39
@public-teleport-github-review-bot

@fspmarshall See the table below for backport results.

Branch Result
branch/v13 Create PR
branch/v14 Create PR
branch/v15 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants