Skip to content

Pass QueryEngineFactory by reference#6331

Merged
fpetkovski merged 2 commits intothanos-io:mainfrom
rabenhorst:fix-engine-factory
May 3, 2023
Merged

Pass QueryEngineFactory by reference#6331
fpetkovski merged 2 commits intothanos-io:mainfrom
rabenhorst:fix-engine-factory

Conversation

@rabenhorst
Copy link
Contributor

Using both the GRPC and HTTP query API of one querier results in duplicate metrics collector registration attempted. This happens because the queryEngineFactory is currently passed by value, so there will be two instances of the factory resulting in two engines being created when using both the GRPC and HTTP query API of the same querier. Since both engines will register the same metrics we get the duplication error. To fix this we change to passing the engine factory by reference in this PR.

@rabenhorst rabenhorst changed the title Pass engine factory by reference Pass QueryEngineFactory by reference May 3, 2023
@pull-request-size pull-request-size bot added size/M and removed size/S labels May 3, 2023
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

DCO missing?

@rabenhorst rabenhorst force-pushed the fix-engine-factory branch from 1a58b18 to d1edd26 Compare May 3, 2023 13:36
@rabenhorst
Copy link
Contributor Author

DCO missing?

Fixed

rabenhorst added 2 commits May 3, 2023 15:38
Signed-off-by: Sebastian Rabenhorst <sebastian.rabenhorst@shopify.com>

Cleanup

Signed-off-by: Sebastian Rabenhorst <sebastian.rabenhorst@shopify.com>
Signed-off-by: Sebastian Rabenhorst <sebastian.rabenhorst@shopify.com>
@rabenhorst rabenhorst force-pushed the fix-engine-factory branch from d1edd26 to 9264b2d Compare May 3, 2023 13:38
@fpetkovski fpetkovski enabled auto-merge May 3, 2023 13:56
@fpetkovski fpetkovski merged commit 1d08ebf into thanos-io:main May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants