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

Cache registry module errors #1258

Merged
merged 7 commits into from
Apr 27, 2023
Merged

Cache registry module errors #1258

merged 7 commits into from
Apr 27, 2023

Conversation

dbanck
Copy link
Member

@dbanck dbanck commented Apr 26, 2023

This PR introduces caching for registry module HTTP request errors. We're currently only caching 404 errors, but it might also be sensible to cache other errors.

The change dramatically improves the performance when many jobs run in the background after a more significant text change.

UX

Before
2023-04-26 15 52 48

After
2023-04-26 15 51 31

dbanck added 3 commits April 26, 2023 15:09
Errors don't have a version, so we update the compound index
to allow missing indices
@dbanck dbanck added the enhancement New feature or request label Apr 26, 2023
@dbanck dbanck self-assigned this Apr 26, 2023
@dbanck dbanck marked this pull request as ready for review April 26, 2023 13:56
@dbanck dbanck requested a review from a team as a code owner April 26, 2023 13:56
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

LGTM, aside from the minor comments about naming. 👍🏻

internal/registry/module.go Outdated Show resolved Hide resolved
internal/registry/module.go Outdated Show resolved Hide resolved
@@ -14,6 +14,7 @@ import (
type RegistryModuleData struct {
Source tfaddr.Module
Version *version.Version
Error bool
Copy link
Member

Choose a reason for hiding this comment

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

Given that we only cache 404 errors, is it worth calling this e.g. NotFound or something similar instead of just Error?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we now cache most 4XX errors, I'll keep it as is?

@@ -67,3 +75,31 @@ func (s *RegistryModuleStore) Cache(sourceAddr tfaddr.Module, modVer *version.Ve
txn.Commit()
return nil
}

func (s *RegistryModuleStore) CacheError(sourceAddr tfaddr.Module) error {
Copy link
Member

Choose a reason for hiding this comment

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

Similar question to above - should we call this e.g. CacheNotFound()?

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants