Make default query context show up in sql query logs #12613
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…up in query logs also has a unit test
TSFenwick
commented
Jun 6, 2022
aggarwalakshay
approved these changes
Jun 6, 2022
kfaraz
reviewed
Jun 7, 2022
There was a problem hiding this comment.
Thanks for the changes, @TSFenwick !
If I understand the problem correctly,
- default query context params can be set as runtime properties
- these are read by
QueryLifecyclebut notSqlLifecycle - SQL execution still honors these default params as it is converted to a native query at which point a
QueryLifecyclegets involved - these default query params don't show up in logs or metrics emitted from the
SqlLifecycle
Please correct me if I have missed anything. If this is the current behaviour, then your changes look good.
I would be nice if you could also add tests (or confirm that these tests already exist) to:
- ensure that SQL queries do indeed honor the default query params
- double writing of the default query params (once in
SqlLifecycleand once inQueryLifecycle) does not affect behaviour
sql/src/main/java/org/apache/druid/sql/SqlLifecycleFactory.java
Outdated
Show resolved
Hide resolved
|
@kfaraz you understood it completely. I'll see what I can do about implementing those two tests you want |
kfaraz
approved these changes
Jun 8, 2022
| @@ -237,6 +242,8 @@ public void configure(Binder binder) | |||
| clientLosAngeles = DriverManager.getConnection(url, propertiesLosAngeles); | |||
| } | |||
|
|
|||
|
|
|||
| @@ -214,6 +218,7 @@ public void configure(Binder binder) | |||
| .toProvider(QuerySchedulerProvider.class) | |||
| .in(LazySingleton.class); | |||
| binder.bind(QueryMakerFactory.class).to(NativeQueryMakerFactory.class); | |||
| binder.bind(new TypeLiteral<Supplier<DefaultQueryConfig>>(){}).toInstance(Suppliers.ofInstance(new DefaultQueryConfig(ImmutableMap.of()))); | |||
There was a problem hiding this comment.
Nit: style
Maybe break up this line so that it's easier to read.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes an issue where when you look at the sql query request logs emitted via an emitter or logged would be missing default query context values set in runtime.properties
Description
Added DefaultQueryConfig to SqlLifecycle ...
This was chosen because while the default query context is added as part of QueryLifecycle it was never added as part of the sql lifecycle meaning it wouldn't show up in the logs or when emitted via an emitter. Since this is a duplicate value with the values being added at QueryLifecycle there should no worries about the eventual double write of default query context values
Key changed/added classes in this PR
SqlLifecycleThis PR has: