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

engine: remove implicit fallback #518

Merged
merged 1 commit into from
Feb 21, 2025

Conversation

MichaHoffmann
Copy link
Contributor

@MichaHoffmann MichaHoffmann commented Feb 19, 2025

What to do in cases where this engine does not understand the query should be decided in the calling code. Taking this responsibility of the engine also forces us to deal with queries that we do not understand explicitly in acceptance tests ( by skipping them ) without accidentally falling back.
It also makes the code and test surface a bit smaller for the engine.

@MichaHoffmann MichaHoffmann force-pushed the mhoffmann/remove-implicit-fallback branch 2 times, most recently from df6e098 to f13566a Compare February 19, 2025 16:17
@MichaHoffmann MichaHoffmann force-pushed the mhoffmann/remove-implicit-fallback branch from f13566a to d7e66d4 Compare February 19, 2025 16:22
Signed-off-by: Michael Hoffmann <[email protected]>
@MichaHoffmann MichaHoffmann force-pushed the mhoffmann/remove-implicit-fallback branch from d7e66d4 to 479ebed Compare February 20, 2025 08:00
Copy link
Member

@saswatamcode saswatamcode left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good, just small doc comment

@@ -6,7 +6,7 @@ The project is currently under active development.

## Roadmap

The engine intends to have full compatibility with the original engine used in Prometheus. Since implementing the full specification will take time, we aim to add support for most commonly used expressions while falling back to the original engine for operations that are not yet supported. This will allow us to have smaller and faster releases, and gather feedback on a regular basis. Instructions on using the engine will be added after we have enough confidence in its correctness.
The engine intends to have full compatibility with the original engine used in Prometheus. Since implementing the full specification will take time, we aim to add support for most commonly used expressions. Instructions on using the engine will be added after we have enough confidence in its correctness. If the engine encounters an expression it does not support it will return an error that can be tested with `engine.IsUnimplemented(err)`, the calling code is expected to handle this fallback.
Copy link
Member

Choose a reason for hiding this comment

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

Hmm I wonder if we can use some other term than "confidence in its correctness". I think the tests speak to that :)

Copy link
Collaborator

@fpetkovski fpetkovski left a comment

Choose a reason for hiding this comment

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

Thanks, lgtm. We should not forget to update Thanos and move the fallback there.

@MichaHoffmann MichaHoffmann merged commit 4230034 into main Feb 21, 2025
7 checks passed
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.

3 participants