Skip to content

HIVE-27120: Warn when Authorizer V2 is configured - #4349

Merged
ayushtkn merged 1 commit into
apache:masterfrom
okumin:HIVE-27120-warn
May 24, 2023
Merged

HIVE-27120: Warn when Authorizer V2 is configured#4349
ayushtkn merged 1 commit into
apache:masterfrom
okumin:HIVE-27120-warn

Conversation

@okumin

@okumin okumin commented May 22, 2023

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

This PR will change an internal param name defined in SessionState.

Why are the changes needed?

The param causes WARN unless we disable hive.conf.validation.

pod/hive-hiveserver2-7fc4df88b6-dmn8v: 2023-03-01T13:50:38,959  WARN [main] conf.HiveConf: HiveConf of name hive.internal.ss.authz.settings.applied.marker does not exist 

That's because it is not listed in HiveConf even though it starts with hive..
https://github.com/apache/hive/blob/rel/release-4.0.0-alpha-2/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java#L6429-L6440

So, this PR will change the prefix from hive.internal. into _hive.. This manner is inherited from some other private params.
https://github.com/apache/hive/blob/rel/release-4.0.0-alpha-2/ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java#L130-L132

Other options I considered.

  • To put hive.internal.ss.authz.settings.applied.marker in HiveConf
    • It is weird to me because users must not configure it
  • To add hive.internal.ss.authz.settings.applied.marker to the allowlist
    • It sounds too adhoc
  • To let HiveConf skip validation when a key starts with hive.internal.
    • I think the approach of this PR is more consistent with other private params

Does this PR introduce any user-facing change?

I assume we can say NO. Precisely, it might change a behavior when a user explicity configures hive.internal.ss.authz.settings.applied.marker. However, I assume this param must not be configured by users.

How was this patch tested?

I have confirmed I don't see the WARN log with this change on my local environment.

@aturoczy aturoczy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This change is sound legit to me. BUT I guess it has some historical part why it is started with _
The change itself OK.
@ayushtkn @deniskuzZ @abstractdog @zabetak @nrg4878 Do you have a historical knowledge about this config?
(The description are pretty well described)

@sonarqubecloud

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@zabetak zabetak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. I checked the git history and I don't see an obvious reason of why "hive.internal" prefix was chosen. Choosing "_hive" to be inline with other "internal" params in this class makes sense.

@ayushtkn ayushtkn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, couldn't find the reason why someone did like that, but it seems safe to me

@okumin

okumin commented May 23, 2023

Copy link
Copy Markdown
Contributor Author

Thanks. My environment will be a bit more silent thanks to your reviews!

@ayushtkn
ayushtkn merged commit 9a35866 into apache:master May 24, 2023
@okumin
okumin deleted the HIVE-27120-warn branch May 25, 2023 00:50
yeahyung pushed a commit to yeahyung/hive that referenced this pull request Jul 20, 2023
…min, reviewed by Stamatis Zampetakis, Ayush Saxena, Attila Turoczy)
tarak271 pushed a commit to tarak271/hive-1 that referenced this pull request Dec 19, 2023
…min, reviewed by Stamatis Zampetakis, Ayush Saxena, Attila Turoczy)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants