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

Don't set cache-skip header if async caching enabled #17

Merged
merged 1 commit into from
Jul 18, 2023
Merged

Conversation

iwysiu
Copy link
Contributor

@iwysiu iwysiu commented Jul 14, 2023

Can be merged separately, but caching actually working is dependent on grafana/grafana#71682

Stop adding the X-Cache-Skip header if awsAsyncQueryCaching is enabled.

Part of #14

@iwysiu iwysiu requested review from a team, idastambuk and kevinwcyu and removed request for a team July 14, 2023 19:43
@@ -136,7 +136,8 @@ export class DatasourceWithAsyncBackend<
};

let headers = {};
if (isRunning(status)) {
const cachingDisabled = !config.featureToggles.useCachingService || !config.featureToggles.awsAsyncQueryCaching
Copy link
Member

Choose a reason for hiding this comment

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

How come we need to involve config.featureToggles.awsAsyncQueryCaching here? What is it about the original code that had anything to do with config.featureToggles.awsAsyncQueryCaching?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There might be a better way to do this, but basically I needed to add an additional check to the query service to get it to work with async aws queries (see the linked pr in the description), and without that additional check we'll end up caching "running" states as well. Technically a user could enable this flag when the fix isn't present and break their version of the plugin, but this is a bit of a guard for if they have the caching service enabled and a version of the plugin that has this check, but aren't on a version of main grafana with that check added.

@iwysiu iwysiu merged commit 0df7f91 into main Jul 18, 2023
2 checks passed
@iwysiu iwysiu deleted the iwysiu/14 branch July 18, 2023 13:11
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