zabetak commented on code in PR #6073:
URL: https://github.com/apache/hive/pull/6073#discussion_r2420763319
##########
ql/src/java/org/apache/hadoop/hive/ql/processors/SetProcessor.java:
##########
@@ -55,19 +56,30 @@ 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 =
Review Comment:
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.
##########
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java:
##########
@@ -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.";
+ }
+
Review Comment:
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.
##########
ql/src/java/org/apache/hadoop/hive/ql/processors/SetProcessor.java:
##########
@@ -251,7 +275,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.")) {
Review Comment:
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
