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

Runtime: metrics view refer model connector #5390

Merged
merged 9 commits into from
Aug 15, 2024
Merged

Conversation

k-anshul
Copy link
Member

@k-anshul k-anshul commented Aug 5, 2024

closes #5085

@k-anshul k-anshul self-assigned this Aug 5, 2024
Copy link
Contributor

@begelundmuller begelundmuller left a comment

Choose a reason for hiding this comment

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

The change works but conceptually speaking I am not sure of it.
We are changing the spec during reconcile which is something we haven't done before.
Also considered storing this in mv's state but technically speaking the model connector is a spec attribute.

Yeah editing the current resource's spec is something we want to be careful with. In this particular case, I think one of these options would be preferable:

  1. (Simple and mostly correct option) Apply this PR to the mv.State.ValidSpec instead. It's used everywhere to actually render/query the metrics view.
  2. (Complex but maybe more correct option) If model: is set in the YAML, capture it as a separate property in the spec (e.g. Spec.Model instead of Spec.Table). Then at query time, if the metrics view is based on a model, look it up to find the underlying connector+table to query.

@k-anshul
Copy link
Member Author

(Simple and mostly correct option) Apply this PR to the mv.State.ValidSpec instead. It's used everywhere to actually render/query the metrics view.

So mv.Spec is what is being set to mv.State.ValidSpec so setting one or other should be equivalent?

(Complex but maybe more correct option) If model: is set in the YAML, capture it as a separate property in the spec (e.g. Spec.Model instead of Spec.Table). Then at query time, if the metrics view is based on a model, look it up to find the underlying connector+table to query.

I thought about it but not sure if we should worry about the underlying model being changed and metricsview not reconciled yet leading to inconsistencies.

@begelundmuller
Copy link
Contributor

So mv.Spec is what is being set to mv.State.ValidSpec so setting one or other should be equivalent?

The ValidSpec is only updated if the Spec passes validation, otherwise it stays as the previous correct spec (only when StageChanges is true). In general, the idea is that a resource's "spec" is its desired state, and the "state" represents its actual state. So a creator (usually the YAML parser) will emit a spec, then the reconciler will update the state based on the spec, and then consumers should only consume from the state.

So I think for this PR we could extend this idea and say that the ValidSpec is not just a validated version of the spec, but a validated and enriched version of the spec.

@k-anshul
Copy link
Member Author

The ValidSpec is only updated if the Spec passes validation, otherwise it stays as the previous correct spec (only when StageChanges is true). In general, the idea is that a resource's "spec" is its desired state, and the "state" represents its actual state. So a creator (usually the YAML parser) will emit a spec, then the reconciler will update the state based on the spec, and then consumers should only consume from the state.

So I think for this PR we could extend this idea and say that the ValidSpec is not just a validated version of the spec, but a validated and enriched version of the spec.

Sure. Sounds good.

runtime/reconcilers/metrics_view.go Outdated Show resolved Hide resolved
runtime/reconcilers/metrics_view.go Outdated Show resolved Hide resolved
runtime/compilers/rillv1/parse_metrics_view.go Outdated Show resolved Hide resolved
runtime/compilers/rillv1/parse_metrics_view.go Outdated Show resolved Hide resolved
runtime/compilers/rillv1/parse_metrics_view.go Outdated Show resolved Hide resolved
runtime/reconcilers/metrics_view.go Outdated Show resolved Hide resolved
runtime/reconcilers/metrics_view.go Outdated Show resolved Hide resolved
k-anshul and others added 2 commits August 15, 2024 14:44
Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com>
@k-anshul k-anshul merged commit 01a5878 into main Aug 15, 2024
7 checks passed
@k-anshul k-anshul deleted the mv_refer_model_connector branch August 15, 2024 09:53
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.

Runtime: Use connector of underlying model for metrics views
2 participants