Skip to content

HIVE-29046: Avoid unecessary perf overhead from handling deprecated properties in MetastoreConf #5897

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+15 −57
Open

HIVE-29046: Avoid unecessary perf overhead from handling deprecated properties in MetastoreConf #5897

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 9 additions & 51 deletions ...e/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,13 @@ public class MetastoreConf {
keyToVars.put(var.varname, var);
keyToVars.put(var.hiveName, var);
}
Configuration.addDeprecations(new Configuration.DeprecationDelta[] {
Copy link
Member

Choose a reason for hiding this comment

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

can we just drop them in Hive-4.1?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have a policy in place about when and how to remove deprecated props so its our call. Personally, my goal here is to avoid the unnecessary perf hit so I don't mind removing them completely if you prefer this path.

Most often, when projects follow semantic versioning breaking changes only happen during major version bumps so ideally the removal should happen in 5.x. In Hive, we are not so dogmatic about it so I think we can afford such small breaking changes even in minor version bumps.

new Configuration.DeprecationDelta("metastore.compactor.history.retention.attempted",
ConfVars.COMPACTOR_HISTORY_RETENTION_DID_NOT_INITIATE.varname),
new Configuration.DeprecationDelta("hive.compactor.history.retention.attempted",
ConfVars.COMPACTOR_HISTORY_RETENTION_DID_NOT_INITIATE.varname),
new Configuration.DeprecationDelta("datanucleus.plugin.pluginRegistryBundleCheck",
ConfVars.DATANUCLEUS_PLUGIN_REGISTRY_BUNDLE_CHECK.varname)});
}

@VisibleForTesting
Expand Down Expand Up @@ -421,9 +428,7 @@ public enum ConfVars {
COMPACTOR_HISTORY_RETENTION_DID_NOT_INITIATE("metastore.compactor.history.retention.did.not.initiate",
"hive.compactor.history.retention.did.not.initiate", 2,
new RangeValidator(0, 100), "Determines how many compaction records in state " +
"'did not initiate' will be retained in compaction history for a given table/partition.",
// deprecated keys:
"metastore.compactor.history.retention.attempted", "hive.compactor.history.retention.attempted"),
"'did not initiate' will be retained in compaction history for a given table/partition."),
COMPACTOR_HISTORY_RETENTION_FAILED("metastore.compactor.history.retention.failed",
"hive.compactor.history.retention.failed", 3,
new RangeValidator(0, 100), "Determines how many failed compaction records will be " +
Expand Down Expand Up @@ -743,8 +748,7 @@ public enum ConfVars {
"initializeColumnInfo setting for DataNucleus; set to NONE at least on Postgres."),
DATANUCLEUS_PLUGIN_REGISTRY_BUNDLE_CHECK("datanucleus.plugin.pluginRegistryBundleCheck".toLowerCase(),
"datanucleus.plugin.pluginRegistryBundleCheck", "LOG", true,
"Defines what happens when plugin bundles are found and are duplicated [EXCEPTION|LOG|NONE]",
"datanucleus.plugin.pluginRegistryBundleCheck", null),
"Defines what happens when plugin bundles are found and are duplicated [EXCEPTION|LOG|NONE]"),
DATANUCLEUS_TRANSACTION_ISOLATION("datanucleus.transactionIsolation",
"datanucleus.transactionIsolation", "read-committed",
"Default transaction isolation level for identity generation."),
Expand Down Expand Up @@ -1940,8 +1944,6 @@ public enum ConfVars {
LONG_TEST_ENTRY("test.long", "hive.test.long", 42, "comment"),
DOUBLE_TEST_ENTRY("test.double", "hive.test.double", Math.PI, "comment"),
TIME_TEST_ENTRY("test.time", "hive.test.time", 1, TimeUnit.SECONDS, "comment"),
DEPRECATED_TEST_ENTRY("test.deprecated", "hive.test.deprecated", 0, new RangeValidator(0, 3), "comment",
"this.is.the.metastore.deprecated.name", "this.is.the.hive.deprecated.name"),
TIME_VALIDATOR_ENTRY_INCLUSIVE("test.time.validator.inclusive", "hive.test.time.validator.inclusive", 1,
TimeUnit.SECONDS,
new TimeValidator(TimeUnit.MILLISECONDS, 500L, true, 1500L, true), "comment"),
Expand All @@ -1957,8 +1959,6 @@ public enum ConfVars {
private final Validator validator;
private final boolean caseSensitive;
private final String description;
private String deprecatedName = null;
private String hiveDeprecatedName = null;

ConfVars(String varname, String hiveName, String defaultVal, String description) {
this.varname = varname;
Expand Down Expand Up @@ -1989,18 +1989,6 @@ public enum ConfVars {
this.description = description;
}

ConfVars(String varname, String hiveName, String defaultVal, boolean caseSensitive,
String description, String deprecatedName, String hiveDeprecatedName) {
this.varname = varname;
this.hiveName = hiveName;
this.defaultVal = defaultVal;
validator = null;
this.caseSensitive = caseSensitive;
this.description = description;
this.deprecatedName = deprecatedName;
this.hiveDeprecatedName = hiveDeprecatedName;
}

ConfVars(String varname, String hiveName, long defaultVal, String description) {
this.varname = varname;
this.hiveName = hiveName;
Expand All @@ -2020,18 +2008,6 @@ public enum ConfVars {
this.description = description;
}

ConfVars(String varname, String hiveName, long defaultVal, Validator validator,
String description, String deprecatedName, String hiveDeprecatedName) {
this.varname = varname;
this.hiveName = hiveName;
this.defaultVal = defaultVal;
this.validator = validator;
caseSensitive = false;
this.description = description;
this.deprecatedName = deprecatedName;
this.hiveDeprecatedName = hiveDeprecatedName;
}

ConfVars(String varname, String hiveName, boolean defaultVal, String description) {
this.varname = varname;
this.hiveName = hiveName;
Expand Down Expand Up @@ -2237,24 +2213,6 @@ public static Configuration newMetastoreConf(Configuration conf) {
LOG.isDebugEnabled()) {
LOG.debug(dumpConfig(conf));
}

/*
Add deprecated config names to configuration.
The parameters for Configuration.addDeprecation are (oldKey, newKey) and it is assumed that the config is set via
newKey and the value is retrieved via oldKey.
However in this case we assume the value is set with the deprecated key (oldKey) in some config file and we
retrieve it in the code via the new key. So the parameter order we use here is: (newKey, deprecatedKey).
We do this with the HiveConf configs as well.
*/
for (ConfVars var : ConfVars.values()) {
if (var.deprecatedName != null) {
Configuration.addDeprecation(var.getVarname(), var.deprecatedName);
}
if (var.hiveDeprecatedName != null) {
Configuration.addDeprecation(var.getHiveName(), var.hiveDeprecatedName);
}
}

return conf;
}

Expand Down
12 changes: 6 additions & 6 deletions ...tastore-server/src/test/java/org/apache/hadoop/hive/metastore/conf/TestMetastoreConf.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,26 +321,26 @@ public void testDeprecatedConfigs() throws IOException {
// set with deprecated key
createConfFile("metastore-site.xml", false, "METASTORE_CONF_DIR", instaMap(
"hive.test.str", "hivedefault",
"this.is.the.metastore.deprecated.name", "1" // default is 0
"metastore.compactor.history.retention.attempted", "0" // default is 2
));
conf = MetastoreConf.newMetastoreConf();
Assert.assertEquals(1, MetastoreConf.getIntVar(conf, ConfVars.DEPRECATED_TEST_ENTRY));
Assert.assertEquals(0, MetastoreConf.getIntVar(conf, ConfVars.COMPACTOR_HISTORY_RETENTION_DID_NOT_INITIATE));

// set with hive (HiveConf) deprecated key
createConfFile("metastore-site.xml", false, "METASTORE_CONF_DIR", instaMap(
"hive.test.str", "hivedefault",
"this.is.the.hive.deprecated.name", "2" // default is 0
"hive.compactor.history.retention.attempted", "1" // default is 2
));
conf = MetastoreConf.newMetastoreConf();
Assert.assertEquals(2, MetastoreConf.getIntVar(conf, ConfVars.DEPRECATED_TEST_ENTRY));
Assert.assertEquals(1, MetastoreConf.getIntVar(conf, ConfVars.COMPACTOR_HISTORY_RETENTION_DID_NOT_INITIATE));

// set with normal key
createConfFile("metastore-site.xml", false, "METASTORE_CONF_DIR", instaMap(
"hive.test.str", "hivedefault",
"test.deprecated", "3" // default is 0
"metastore.compactor.history.retention.did.not.initiate", "3" // default is 2
));
conf = MetastoreConf.newMetastoreConf();
Assert.assertEquals(3, MetastoreConf.getIntVar(conf, ConfVars.DEPRECATED_TEST_ENTRY));
Assert.assertEquals(3, MetastoreConf.getIntVar(conf, ConfVars.COMPACTOR_HISTORY_RETENTION_DID_NOT_INITIATE));
}

@Test
Expand Down
Loading