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

Make MetricsFeature's dependencies explicit #20945

Merged
merged 6 commits into from
Jun 4, 2024

Conversation

goedderz
Copy link
Member

@goedderz goedderz commented May 16, 2024

Scope & Purpose

Make the dependencies of MetricsFeature explicit, so it can get by without getFeature calls, so it can be an ApplicationFeature instead of an ArangodFeature, so it can be used separately from it.

In order to achieve that, this PR introduces LazyApplicationFeatureReference to handle potential circular references in the feature dependencies (which we should try to get rid of later, whenever possible).

  • 🔨 Refactoring/simplification

Related Information

@goedderz goedderz added this to the devel milestone May 16, 2024
@goedderz goedderz self-assigned this May 16, 2024
@cla-bot cla-bot bot added the cla-signed label May 16, 2024
@goedderz goedderz force-pushed the chore/dependency-cleanup-p2 branch from 33fcbb4 to 0799185 Compare June 3, 2024 09:14
@goedderz goedderz changed the title Chore/dependency cleanup p2 Make MetricsFeature's dependencies explicit Jun 3, 2024
@goedderz goedderz marked this pull request as ready for review June 3, 2024 09:26
@goedderz goedderz requested review from a team as code owners June 3, 2024 09:26
Comment on lines +391 to +398
void MetricsFeature::prepare() {
_queryRegistryFeature = std::move(_lazyQueryRegistryFeatureRef).get();
_statisticsFeature = std::move(_lazyStatisticsFeatureRef).get();
_engineSelectorFeature = std::move(_lazyEngineSelectorFeatureRef).get();
_clusterMetricsFeature = std::move(_lazyClusterMetricsFeatureRef).get();
_clusterFeature = std::move(_lazyClusterFeatureRef).get();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we assert somehow that prepare() and therefore get() methods are really called only once?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@jsteemann jsteemann left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@alexbakharew alexbakharew left a comment

Choose a reason for hiding this comment

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

LGTM

@neunhoef neunhoef merged commit 89228e2 into devel Jun 4, 2024
6 checks passed
@neunhoef neunhoef deleted the chore/dependency-cleanup-p2 branch June 4, 2024 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants