HIVE-29191: Gracefully Handle Removed Configuration Properties in Hiv… #6073
Conversation
…e 4 to Avoid Job Failures
| public static String generateRemovedWarning() { | ||
| return "This config does not exist in the current version of Hive. Consider removing this config."; | ||
| } | ||
|
|
There was a problem hiding this comment.
There are better ways to log warning about deprecated/removed properties. See: org.apache.hadoop.conf.Configuration#addDeprecation
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
asf-ci-hive
added
tests pending
tests unstable
and removed
tests passed
tests pending
labels
vikramahuja1001
force-pushed
the
RemovedConfigs
branch
from
699397f to
950a048
Compare
|
There was a problem hiding this comment.
Apologies for the delay but I missed the review notification. @vikramahuja1001 I appreciate your efforts in improving the migration experience for the end-users but let's discuss a bit more to see if we can find solution that will have less maintenance burden.
|
|
||
| private static final String prefix = "set: "; | ||
| private static final Set<String> removedConfigs = | ||
| private static final Set<String> removedHiveConfigs = |
There was a problem hiding this 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.
| throw new IllegalArgumentException(message.toString()); | ||
| } | ||
| } else if (!removedConfigs.contains(key) && key.startsWith("hive.")) { | ||
| } else if (!allowOrcConfigs.contains(key) && key.startsWith("hive.")) { |
There was a problem hiding this comment.
Going forward I see the following options (in personal preference order):
- 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
- 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 - We revert HIVE-7211 and never throw if a "hive." property is missing (potentially just log a warn message)
| public static String generateRemovedWarning() { | ||
| return "This config does not exist in the current version of Hive. Consider removing this config."; | ||
| } | ||
|
|
There was a problem hiding this 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.
4 participants
…e 4 to Avoid Job Failures
What changes were proposed in this pull request?
Why are the changes needed?
Does this PR introduce any user-facing change?
How was this patch tested?