Skip to content

Conversation

@dengzhhu653
Copy link
Member

…increases overhead

What changes were proposed in this pull request?

Why are the changes needed?

HiveMetaStoreAuthorizer can act as the pre event listener inside the HMS, or as the metadata filter hook on the client. If it's insides the HMS, getConf() should have loaded all properties in hive-site.xml, otherwise it contains the information to talk with the HMS at least, as the call is from client as a filter hook.

We don't need to create a new HiveConf per thread, it's wasteful and might spend dozens of milliseconds to load the properties, log the warn message "HiveConf of name xx does not exist" continuously if the property is only specified in MetastoreConf.

Does this PR introduce any user-facing change?

No

How was this patch tested?

The warn message of HiveConf doesn't exist is gone in an affected HMS

@sonarqubecloud
Copy link

Copy link
Contributor

@wecharyu wecharyu left a comment

Choose a reason for hiding this comment

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

There are other places where creating new HiveConf may not be necessary. For example:

private static AlterTableExecuteDesc getExpireSnapshotDesc(TableName tableName, Map<String, String> partitionSpec,
List<Node> children, HiveConf conf) throws SemanticException {
AlterTableExecuteSpec<ExpireSnapshotsSpec> spec;
if (children.size() == 1) {
spec = new AlterTableExecuteSpec(EXPIRE_SNAPSHOT, null);
return new AlterTableExecuteDesc(tableName, partitionSpec, spec);
}
ZoneId timeZone = SessionState.get() == null ?
new HiveConf().getLocalTimeZone() :

return ret;
}


Copy link
Contributor

Choose a reason for hiding this comment

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

nit: unnecessary empty line.

initialize(cls);
}

private HiveConf(Configuration other) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about making it public and use it for a clone?

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.

3 participants