Don't use explicit Guice binding for config class#14039
Closed
ksobolew wants to merge 1 commit intotrinodb:masterfrom
Closed
Don't use explicit Guice binding for config class#14039ksobolew wants to merge 1 commit intotrinodb:masterfrom
ksobolew wants to merge 1 commit intotrinodb:masterfrom
Conversation
The binding is necessary because `HiveMetadataFactory` has an `@Inject`ed parameter `MetastoreTypeConfig` now. This means that it needs to be bound now in all code paths, even when the metastore is provided externally (which only happens when using `HiveQueryRunner`). But explicit binding is bad, because it is incompatible with bindings managed by io.airlift:configuration, specifically with `buildConfigObject` and anything that calls it. This commit changes the binding to use a regular `configBinder()`; in order to prevent unexpected configuration "leaking" from the environment, the `HiveQueryRunner` now explicitly forbids setting the `hive.metastore` property.
findepi
requested changes
Sep 7, 2022
| binder.bind(HiveMetastoreFactory.class).annotatedWith(RawHiveMetastoreFactory.class).toInstance(HiveMetastoreFactory.ofInstance(metastore.get())); | ||
| MetastoreTypeConfig metastoreTypeConfig = new MetastoreTypeConfig(); | ||
| metastoreTypeConfig.setMetastoreType("provided"); | ||
| binder.bind(MetastoreTypeConfig.class).toInstance(metastoreTypeConfig); |
findepi
reviewed
Sep 7, 2022
| metastoreTypeConfig.setMetastoreType("provided"); | ||
| binder.bind(MetastoreTypeConfig.class).toInstance(metastoreTypeConfig); | ||
| configBinder(binder).bindConfigDefaults(MetastoreTypeConfig.class, config -> config.setMetastoreType("provided")); | ||
| configBinder(binder).bindConfig(MetastoreTypeConfig.class); |
Member
There was a problem hiding this comment.
It is wrong. Here the code intention is not to expose any configurable properties
Contributor
Author
There was a problem hiding this comment.
I talked with @homar about this and was under the impression that this was done this way because there was no better way. I kinda understand why it's like this, but the current way is wrong too :)
Here the code intention is not to expose any configurable properties
I get that. But this code path is only used when HiveQueryRunner is used, so I added an assertion there to prevent setting this property. I was hoping this would have the same effect 🤷
Member
|
I posted #14059 as an alternative approach |
Contributor
Author
|
Closing in favor of #14059 |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
The binding is necessary because
HiveMetadataFactoryhas an@Injected parameterMetastoreTypeConfignow. This means that it needs to be bound now in all code paths, even when the metastore is provided externally (which only happens when usingHiveQueryRunner). But explicit binding is bad, because it is incompatible with bindings managed by io.airlift:configuration, specifically withbuildConfigObjectand anything that calls it.This commit changes the binding to use a regular
configBinder(); in order to prevent unexpected configuration "leaking" from the environment, theHiveQueryRunnernow explicitly forbids setting thehive.metastoreproperty.Non-technical explanation
Fixes incorrect internal use of configuration management interfaces.
Release notes
(x) This is not user-visible and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: