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

ref(project-cache): Check revision before loading from Redis #3892

Merged
merged 4 commits into from
Aug 6, 2024

Conversation

Dav1dde
Copy link
Member

@Dav1dde Dav1dde commented Aug 2, 2024

Checks the 'new' revision key before actually loading and parsing the project config.

Revision introduced here: getsentry/sentry#75523
Change is backwards compatible, if the key is missing the check is simply skipped.

I was considering propagating the information whether something was refreshed or a fresh fetch through the state channel, but after review and consideration, I don't think that is useful information, we still have to reset a bunch of internal state and move the project out of the in flight status. All if which we already do, the only thing we could change is skipping the assignment of the project info contained in the project fetch state (because that's the same state as the old), but this is already only an Arc.

Still needs:

  • Disabled states also need to remember their revision. Maybe we skip this, since disabled states are generally very small and quick, but for consistency sake it should be there. Disabled states don't have a revision currently, they currently have no metadata and are just {"disabled": true}. In a followup we can think about introducing a revision for disabled states as well (or checking before & after for disabled).
  • Integration tests!

#skip-changelog

@Dav1dde Dav1dde self-assigned this Aug 2, 2024
@Dav1dde Dav1dde force-pushed the dav1d/project-config-redis-rev branch 2 times, most recently from 382d427 to c97c47d Compare August 2, 2024 12:30
@Dav1dde Dav1dde force-pushed the dav1d/project-config-redis-rev branch from c97c47d to 8c929dc Compare August 5, 2024 11:18
@Dav1dde Dav1dde marked this pull request as ready for review August 5, 2024 13:56
@Dav1dde Dav1dde requested a review from a team as a code owner August 5, 2024 13:56
Copy link
Contributor

@olksdr olksdr left a comment

Choose a reason for hiding this comment

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

lgtm

see the comments

relay-server/src/services/project_cache.rs Outdated Show resolved Hide resolved
relay-server/src/services/project_cache.rs Outdated Show resolved Hide resolved
relay-server/src/statsd.rs Outdated Show resolved Hide resolved
relay-server/src/services/project_redis.rs Show resolved Hide resolved
@Dav1dde Dav1dde force-pushed the dav1d/project-config-redis-rev branch from 9d81609 to 77fcb34 Compare August 6, 2024 07:51
@Dav1dde Dav1dde enabled auto-merge (squash) August 6, 2024 07:54
@Dav1dde Dav1dde merged commit be744a1 into master Aug 6, 2024
24 checks passed
@Dav1dde Dav1dde deleted the dav1d/project-config-redis-rev branch August 6, 2024 08:04
Dav1dde added a commit that referenced this pull request Aug 6, 2024
Dav1dde added a commit that referenced this pull request Aug 6, 2024
…#3898)

Reverts #3892

Noticing some problems with this commit using too much memory, almost
looks like a memory leak.
Dav1dde added a commit that referenced this pull request Aug 14, 2024
Switches the regex implementation for globs from `regex` to regex-lite`,
this should bring down memory consumption for all globs.

This is just a temporary stop-gap to unblock the project cache
improvements started in #3892, eventually the glob implementations
should be unified and ideally be as much as possible regex free.
Dav1dde added a commit that referenced this pull request Aug 14, 2024
Dav1dde added a commit that referenced this pull request Aug 28, 2024
Follow-up to to #3892, now also implements the revision fetching in the
project config API.

Sentry does not need to be updated and this is not a breaking change and
works with any combinations of Relays.

Extends the project config API with another field `revisions` which
optionally contains the revisions for all requested public keys. If
there are are revisions, the response is extended with a field
`unchanged` which contains all project keys which were unchanged (have
the same revision), similar to how `pending` is handled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants