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

Query: Fixing extended functions in distributed querier #7331

Merged

Conversation

pedro-stanaka
Copy link
Contributor

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

  • Adding repro case for broken query with distributed engine
  • Fixing problem with distributed queries and xfunctions
  • Adding support for extended functions in tenancy enforcement
  • Moving custom parser to new package

Verification

Added new acceptance tests and tested locally with two queriers.

@MichaHoffmann
Copy link
Contributor

LGTM, can you separate the prometheus import from stdlib imports though please?

pkg/api/query/v1.go Outdated Show resolved Hide resolved
@pedro-stanaka
Copy link
Contributor Author

LGTM, can you separate the prometheus import from stdlib imports though please?

@MichaHoffmann can you recheck?

fpetkovski
fpetkovski previously approved these changes May 3, 2024
Copy link
Contributor

@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.

Nice work

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.

Should we forbid using parser.ParseExpr directly through faillint? What about other places where parser.ParseExpr is used? 🤔

@pedro-stanaka
Copy link
Contributor Author

Should we forbid using parser.ParseExpr directly through faillint? What about other places where parser.ParseExpr is used? 🤔

I have a follow up PR where I will clean up all other places where we use parser.ParserExpr did not want to include on this one because I will touch more than 30 files. I can also create this fail lint rule if it is the case.

@@ -196,3 +195,54 @@ func TestTenantProxyPassing(t *testing.T) {
_ = q.Series(&storepb.SeriesRequest{Matchers: seriesMatchers}, &storeSeriesServer{ctx: ctx})
})
}

func TestGetTenantFromHTTP(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

This test seems unrelated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed.

Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>
Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>
Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>
Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>
Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>
Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>
Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>
Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>
Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>
Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, this has been flaky for a while I think!

Copy link
Contributor

@MichaHoffmann MichaHoffmann 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

Choose a reason for hiding this comment

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

That one is on me, thanks for fixing

@MichaHoffmann MichaHoffmann merged commit 70e3bd9 into thanos-io:main May 4, 2024
20 checks passed
Nashluffy pushed a commit to Nashluffy/thanos that referenced this pull request May 14, 2024
* Adding repro case for broken query with distributed engine

Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>

* Fixing problem with distributed queries and xfunctios

Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>

* Adding support for extended functions in tenancy enforcement

Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>

* Moving custom parser to new package

Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>

* fixing go-lint

Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>

* Using same opts and reorganize imports

Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>

* fixing problem with query format

Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>

* fixing flaky tests

Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>

* removing extra test

Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>

* yet another flaky test

Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>

---------

Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>
Signed-off-by: mluffman <nashluffman@gmail.com>
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.

None yet

4 participants