-
Notifications
You must be signed in to change notification settings - Fork 4.8k
HIVE-28993: use lower case datanucleus.plugin.pluginRegistryBundleCheck #5847
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a link to the datanucleus change for reference: datanucleus/datanucleus-core@20a99c1#diff-b9ebd71adfcb98d965225c373e44910dc01d1dd8c37a3e4d6d906d58f108e120R30
LGTM pending green test run.
Thanks for the PR!
DATANUCLEUS_PLUGIN_REGISTRY_BUNDLE_CHECK("datanucleus.plugin.pluginRegistryBundleCheck", | ||
"datanucleus.plugin.pluginRegistryBundleCheck", "LOG", | ||
"Defines what happens when plugin bundles are found and are duplicated [EXCEPTION|LOG|NONE]"), | ||
DATANUCLEUS_PLUGIN_REGISTRY_BUNDLE_CHECK("datanucleus.plugin.pluginRegistryBundleCheck".toLowerCase(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- why use
toLowerCase()
instead of just lowercasing the string? - hiveName remained the same, why mark it as deprecated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Same as datanucleus. Please see https://github.com/datanucleus/datanucleus-core/blob/6dc3ad2e63b74efdd3a3c4d3f318a983048e13fc/src/main/java/org/datanucleus/PropertyNames.java#L25.
- The old key whether it is
hiveName
or metastorevarname
has no effect, so both are deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:) interesting style.
Hive already creates plenty of HiveConf obj instances that take a significant part of Heap, so i am not sure it's a good idea to add an overhead by creating unnecessary objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HiveConf holds over 1K entries and avoiding one extra mapping is not going to buy much, IMO. Avoiding just one extra HiveConf object is going to save much more :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, but in any case, why add more redundancy?
...tore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
Show resolved
Hide resolved
@deniskuzZ IMO, it will be good to revisit of how much testing was done during datanucleus major version upgrade and review https://www.datanucleus.org/products/accessplatform_6_0/migration.html. |
|
What changes were proposed in this pull request?
use lower case
datanucleus.plugin.pluginregistrybundlecheck
and deprecate priordatanucleus.plugin.pluginRegistryBundleCheck
.Why are the changes needed?
After upgrade to 6.x datanucleus does not recognize old settings as it was changed to all lower case.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Using existing tests