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

Gzip content cache and bugfix cache time calculation #790

Merged
merged 4 commits into from Jul 22, 2022

Conversation

addreas
Copy link
Contributor

@addreas addreas commented Jul 15, 2022

Description

Three things happening here:

  • Compressing the content cache to put less pressure on the api-server
  • Adding a nil pointer check and fixing logic for contentCacheDuration <= 0
  • Remove old spec.json content before setting the status.contentGzip field to further down-size the CR. Not sure if this is a good idea. Might make some users unhappy if things disappear?

Have to create a new content field in the status since changing a type from string to []byte would prevent the operator from reading existing status content which wouldn't be valid base64. Perhaps should remove the old one?

Relevant issues/tickets

#789

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • This change requires a documentation update
  • I have added tests that prove my fix is effective or that my feature works
  • I have added a test case that will be used to verify my changes
  • Verified independently on a cluster by reviewer

Verification steps

@addreas addreas changed the title Gzip content cache and bugfix cache time ecalculation Gzip content cache and bugfix cache time calculation Jul 19, 2022
Copy link
Collaborator

@NissesSenap NissesSenap left a comment

Choose a reason for hiding this comment

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

In general I think it makes sense that we ignore spec.json if we use something else like gzipJson or url. At the same time it's a breaking change since it will change functionality even though that way of working is probably "wrong".

About "Have to create a new content field in the status since changing a type from string to []byte would prevent the operator from reading existing status content which wouldn't be valid base64. Perhaps should remove the old one?"

Once again this makes a breaking change, at the same time this status filed is new and there are probably not many people that rely on it. So from that point of view I would say it's probably better to remove it to minimize confusion and problem in the future.

I can't remember what we said during the meeting how to handle this PR in general.
Any way I added a few comments.

api/integreatly/v1alpha1/grafanadashboard_types.go Outdated Show resolved Hide resolved
@addreas
Copy link
Contributor Author

addreas commented Jul 21, 2022

From what I can tell it shouldn't be a problem to remove the old status.content, as long as the CRDs are updated along side the operator it self. It will be silently ignored when read from the api-server, and not submitted when setting the new values. The api-server doesn't seem to validate existing resources when changing a CRD, only when submitting the actual resource.

I changed the clearing of the old spec.json data to only happen when the freshly downloaded content is identical. Either way this only happens when fetching is going on, and the fetch has been successful. If the fetch fails the fallback behavior is the same as before.

Copy link
Collaborator

@HubertStefanski HubertStefanski left a comment

Choose a reason for hiding this comment

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

LGTM!

@NissesSenap NissesSenap merged commit a694bdf into grafana:master Jul 22, 2022
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.

None yet

3 participants