Skip to content

HIVE-29191: Gracefully Handle Removed Configuration Properties in Hiv… #6073

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 2 commits into
base: master
Choose a base branch
from
+48 −4
Open

HIVE-29191: Gracefully Handle Removed Configuration Properties in Hiv… #6073

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ee0d41
HIVE-29191: Gracefully Handle Removed Configuration Properties in Hiv…
vikramahuja1001 Sep 11, 2025
950a048
review comments handling
vikramahuja1001 Sep 23, 2025
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
9 changes: 9 additions & 0 deletions common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5607,11 +5607,16 @@ public static enum ConfVars {
HIVE_MM_AVOID_GLOBSTATUS_ON_S3("hive.mm.avoid.s3.globstatus", true,
"Whether to use listFiles (optimized on S3) instead of globStatus when on S3."),

HIVE_IGNORE_REMOVED_CONFIGS_LIST("hive.ignore.removed.configs.list",
"",
"Comma separated list of configuration options which are removed from hive code. Silently ignore if the user tries to set them"),

// If a parameter is added to the restricted list, add a test in TestRestrictedList.Java
HIVE_CONF_RESTRICTED_LIST("hive.conf.restricted.list",
"hive.security.authenticator.manager,hive.security.authorization.manager," +
"hive.security.metastore.authorization.manager,hive.security.metastore.authenticator.manager," +
"hive.users.in.admin.role,hive.server2.xsrf.filter.enabled,hive.server2.csrf.filter.enabled,hive.security.authorization.enabled," +
"hive.ignore.removed.configs.list," +
"hive.distcp.privileged.doAs," +
"hive.server2.authentication.ldap.baseDN," +
"hive.server2.authentication.ldap.url," +
Expand Down Expand Up @@ -7261,6 +7266,10 @@ public static String generateDeprecationWarning() {
+ "versions. Please adjust DDL towards the new semantics.";
}

public static String generateRemovedWarning() {
return "This config does not exist in the current version of Hive. Consider removing this config.";
}

Comment on lines +7264 to +7267
Copy link
Member

@zabetak zabetak Sep 11, 2025

Choose a reason for hiding this comment

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

There are better ways to log warning about deprecated/removed properties. See: org.apache.hadoop.conf.Configuration#addDeprecation

Copy link
Contributor Author

@vikramahuja1001 vikramahuja1001 Sep 29, 2025

Choose a reason for hiding this comment

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

I checked the method addDeprecation. This particular method takes oldKeys and newKeys(which are replacing the oldKeys) and it also has a null check to make sure newKeys are not null. Now since we have removed the properties there are no newKeys in our case making the method unusable for us.

Do let me know in case there is some other way to use the same functionality.

Copy link
Member

@zabetak zabetak Oct 10, 2025

Choose a reason for hiding this comment

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

I guess we could pass an empty string or some special placeholder to achieve somewhat the desired behavior. However, since we are talking about removed properties I guess the deprecation API that I suggested does not make much sense. One way or the other at some point also entries in the deprecation context will be removed.

private static final Object reverseMapLock = new Object();
private static HashMap<String, ConfVars> reverseMap = null;

Expand Down
1 change: 1 addition & 0 deletions itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestRestrictedList.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ public static void startServices() throws Exception {

// Add the parameter here if it cannot change at runtime
addToExpectedRestrictedMap("hive.conf.restricted.list");
addToExpectedRestrictedMap("hive.ignore.removed.configs.list");
addToExpectedRestrictedMap("hive.security.authenticator.manager");
addToExpectedRestrictedMap("hive.security.authorization.manager");
addToExpectedRestrictedMap("hive.security.metastore.authorization.manager");
Expand Down
36 changes: 32 additions & 4 deletions ql/src/java/org/apache/hadoop/hive/ql/processors/SetProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

import static org.apache.hadoop.hive.conf.SystemVariables.*;

import java.util.HashSet;
import java.util.Map;
import java.util.Properties;
import java.util.Set;
Expand Down Expand Up @@ -55,19 +56,26 @@ public class SetProcessor implements CommandProcessor {
private static final SessionState.LogHelper console = SessionState.getConsole();

private static final String prefix = "set: ";
private static final Set<String> removedConfigs =
private static final Set<String> removedHiveConfigs =
Copy link
Member

@zabetak zabetak Oct 10, 2025

Choose a reason for hiding this comment

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

I feel that maintaining all removed configs in the code base does not make much sense and beats the purpose of deprecating and removing a property.

If every time that we remove a property we had to add it here and continue maintaining to some extend then why remove it in the first place. We could keep the property and just remove all references to it which also doesn't make sense.

Moreover, as history has shown we don't enrich this removeConfigs set in a consistent manner so sooner or later we will have more tickets proposing to enrich this set with more removed stuff.

I would propose to drop this completely and stop adding stuff here.

Sets.newHashSet("hive.mapred.supports.subdirectories",
"hive.enforce.sorting","hive.enforce.bucketing",
"hive.outerjoin.supports.filters",
"hive.llap.zk.sm.principal",
"hive.llap.zk.sm.keytab.file"
"hive.llap.zk.sm.keytab.file",
"hive.stats.fetch.partition.stats",
"hive.optimize.sort.dynamic.partition",
"hive.metastore.initial.metadata.count.enabled",
"hive.cli.pretty.output.num.cols",
"hive.debug.localtask",
"hive.timedout.txn.reaper.start"
);
// Allow the user to set the ORC properties without getting an error.
private static final Set<String> allowOrcConfigs = new HashSet<>();
static {
for(OrcConf var: OrcConf.values()) {
String name = var.getHiveConfName();
if (name != null && name.startsWith("hive.")) {
removedConfigs.add(name);
allowOrcConfigs.add(name);
}
}
}
Expand Down Expand Up @@ -140,6 +148,10 @@ private boolean isHidden(String key) {
return false;
}

public Set<String> getRemovedHiveConfigs() {
return removedHiveConfigs;
}

private void dumpOption(String s) {
SessionState ss = SessionState.get();

Expand Down Expand Up @@ -228,6 +240,22 @@ static String setConf(SessionState ss, String varName, String key, String varVal
throws IllegalArgumentException {
String result = null;
HiveConf conf = ss.getConf();

String removedHiveConfigsList = conf.getVar(HiveConf.ConfVars.HIVE_IGNORE_REMOVED_CONFIGS_LIST);
if (removedHiveConfigsList != null && !removedHiveConfigsList.isEmpty()) {
for (String entry : removedHiveConfigsList.split(",")) {
if (!removedHiveConfigs.contains(entry.trim())) {
removedHiveConfigs.add(entry.trim());
}
}
}
if (removedHiveConfigs.contains(key)) {
// do not do anything. do not throw any error, just silently return
result = HiveConf.generateRemovedWarning();
LOG.warn(result);
return result;
}

String value = new VariableSubstitution(new HiveVariableSource() {
@Override
public Map<String, String> getHiveVariable() {
Expand All @@ -251,7 +279,7 @@ public Map<String, String> getHiveVariable() {
message.append("' FAILED in validation : ").append(fail).append('.');
throw new IllegalArgumentException(message.toString());
}
} else if (!removedConfigs.contains(key) && key.startsWith("hive.")) {
} else if (!allowOrcConfigs.contains(key) && key.startsWith("hive.")) {
Copy link
Member

@zabetak zabetak Oct 10, 2025

Choose a reason for hiding this comment

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

Going forward I see the following options (in personal preference order):

  1. Leave exception as is and have users turn off property validation till they migrate their scripts which is somewhat acceptable given that a proper deprecation path was followed before removing a property
  2. Add a very specific boolean property (e.g., hive.conf.validation.missing.key.throws) that fine tunes a bit if validation throws or not on missing keys
  3. We revert HIVE-7211 and never throw if a "hive." property is missing (potentially just log a warn message)

throw new IllegalArgumentException("hive configuration " + key + " does not exists.");
}
}
Expand Down
16 changes: 16 additions & 0 deletions ql/src/test/org/apache/hadoop/hive/ql/processors/TestSetProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,22 @@ public void testSystemPropertyIndividual() throws Exception {
Assert.assertTrue(output.contains("hidden"));
}

@Test
public void testRemovedConfigs() throws Exception {
Assert.assertEquals(baos.size(),0);
for (String entry : processor.getRemovedHiveConfigs()) {
// trying to set value
runSetProcessor(entry + "=Something");
// trying to get the value based on the previous set
runSetProcessor(entry);
Assert.assertTrue(baos.size() > 0);
String output = baos.toString();
Assert.assertTrue(output.contains("undefined"));
Assert.assertFalse(output.contains("Something"));
}
Assert.assertTrue(baos.size() > 0);
}

/*
* Simulates the set <command>;
*/
Expand Down
Loading