Skip to content

fix(query_planner): put the USAGE_REPORTING back in the context if the query plan has been cached#1559

Merged
Geal merged 2 commits intomainfrom
bnjjj/fix_usage_reporting_cache
Aug 22, 2022
Merged

fix(query_planner): put the USAGE_REPORTING back in the context if the query plan has been cached#1559
Geal merged 2 commits intomainfrom
bnjjj/fix_usage_reporting_cache

Conversation

@bnjjj
Copy link
Contributor

@bnjjj bnjjj commented Aug 19, 2022

This bug was introduced with the cache on the query planner and then we didn't have any correct data for usage reporting.

…e query plan has been cached

Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj bnjjj requested review from BrynCooke, Geal, SimonSapin, garypen and o0Ignition0o and removed request for garypen August 19, 2022 15:56
@bnjjj bnjjj self-assigned this Aug 19, 2022
@github-actions

This comment has been minimized.

Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

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

Ah! nice find!

Could we add some tests to catch this?

Copy link
Contributor

@Geal Geal left a comment

Choose a reason for hiding this comment

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

damn, I did it for the errors but forgot for the main case 😢

@o0Ignition0o
Copy link
Contributor

++ for a test, I've added one to the query plan exposition by invoking the service twice, this would probably work

@abernix abernix added this to the vNEXT milestone Aug 22, 2022
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj bnjjj requested a review from abernix August 22, 2022 08:07
@Geal Geal dismissed abernix’s stale review August 22, 2022 09:21

the test was added in 8dc8150

@Geal Geal merged commit 1022309 into main Aug 22, 2022
@Geal Geal deleted the bnjjj/fix_usage_reporting_cache branch August 22, 2022 09:21
@Geal Geal mentioned this pull request Aug 22, 2022
@abernix
Copy link
Member

abernix commented Aug 23, 2022

the test was added in 8dc8150

thanks! 🙏

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.

5 participants