You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2021/03/19 21:10:11 UTC
[GitHub] [hive] nrg4878 opened a new pull request #2094: HIVE-24175: getDatabase() should only return managedlocation if set (…
nrg4878 opened a new pull request #2094:
URL: https://github.com/apache/hive/pull/2094
…Naveen Gangam)
### What changes were proposed in this pull request?
With the introduction of a managedLocation and HMS translation layer, getDatabase() call always returns a managedlocation as well even if not set (defaults to managed warehouse dir). This might have been a bit ambitious as the preevent and postevents sent out as part of this create database DDL does not include this managed location. This path is added by the translation layer on a getDatabase() call. HMS DB store does not have a value either. So we are a bit inconsistent on this matter.
### Why are the changes needed?
Address a bug fix.
### Does this PR introduce _any_ user-facing change?
no
### How was this patch tested?
Manually
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] vihangk1 commented on a change in pull request #2094: HIVE-24175: getDatabase() should only return managedlocation if set (…
Posted by GitBox <gi...@apache.org>.
vihangk1 commented on a change in pull request #2094:
URL: https://github.com/apache/hive/pull/2094#discussion_r600862728
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetastoreDefaultTransformer.java
##########
@@ -695,7 +695,8 @@ public Database transformDatabase(Database db, List<String> processorCapabilitie
if (!isTenantBasedStorage) {
Path locationPath = Path.getPathWithoutSchemeAndAuthority(new Path(db.getLocationUri()));
Path whRootPath = Path.getPathWithoutSchemeAndAuthority(hmsHandler.getWh().getWhRoot());
- if (FileUtils.isSubdirectory(whRootPath.toString(), locationPath.toString())) { // legacy path
+ LOG.debug("Comparing DB and warehouse paths warehouse={} db.getLocationUri={}", whRootPath.toString(), locationPath.toString());
+ if (FileUtils.isSubdirectory(whRootPath.toString(), locationPath.toString()) || locationPath.equals(whRootPath)) { // legacy path
Review comment:
Synced up with Naveen offline. I think it would be great to add a comment here explaining when the managed Location is set and when it is not. The patch itself looks good to me. Once we have a comment, I think we can merge it without the need for precommit (since there is only a comment which is added).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] vihangk1 commented on a change in pull request #2094: HIVE-24175: getDatabase() should only return managedlocation if set (…
Posted by GitBox <gi...@apache.org>.
vihangk1 commented on a change in pull request #2094:
URL: https://github.com/apache/hive/pull/2094#discussion_r599792900
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetastoreDefaultTransformer.java
##########
@@ -695,7 +695,8 @@ public Database transformDatabase(Database db, List<String> processorCapabilitie
if (!isTenantBasedStorage) {
Path locationPath = Path.getPathWithoutSchemeAndAuthority(new Path(db.getLocationUri()));
Path whRootPath = Path.getPathWithoutSchemeAndAuthority(hmsHandler.getWh().getWhRoot());
- if (FileUtils.isSubdirectory(whRootPath.toString(), locationPath.toString())) { // legacy path
+ LOG.debug("Comparing DB and warehouse paths warehouse={} db.getLocationUri={}", whRootPath.toString(), locationPath.toString());
+ if (FileUtils.isSubdirectory(whRootPath.toString(), locationPath.toString()) || locationPath.equals(whRootPath)) { // legacy path
Review comment:
It is unclear to me why we set the managedLocationUri in this if block. It looks like if the db location is under warehouse location we will be setting managed location same as the db location. Why is that needed? A comment here about the reasoning for this would be very helpful.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] nrg4878 commented on pull request #2094: HIVE-24175: getDatabase() should only return managedlocation if set (…
Posted by GitBox <gi...@apache.org>.
nrg4878 commented on pull request #2094:
URL: https://github.com/apache/hive/pull/2094#issuecomment-806199611
Closing the PR as it has been merged. Thanks for the review Vihang.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] nrg4878 closed pull request #2094: HIVE-24175: getDatabase() should only return managedlocation if set (…
Posted by GitBox <gi...@apache.org>.
nrg4878 closed pull request #2094:
URL: https://github.com/apache/hive/pull/2094
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] nrg4878 commented on a change in pull request #2094: HIVE-24175: getDatabase() should only return managedlocation if set (…
Posted by GitBox <gi...@apache.org>.
nrg4878 commented on a change in pull request #2094:
URL: https://github.com/apache/hive/pull/2094#discussion_r600816065
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetastoreDefaultTransformer.java
##########
@@ -695,7 +695,8 @@ public Database transformDatabase(Database db, List<String> processorCapabilitie
if (!isTenantBasedStorage) {
Path locationPath = Path.getPathWithoutSchemeAndAuthority(new Path(db.getLocationUri()));
Path whRootPath = Path.getPathWithoutSchemeAndAuthority(hmsHandler.getWh().getWhRoot());
- if (FileUtils.isSubdirectory(whRootPath.toString(), locationPath.toString())) { // legacy path
+ LOG.debug("Comparing DB and warehouse paths warehouse={} db.getLocationUri={}", whRootPath.toString(), locationPath.toString());
+ if (FileUtils.isSubdirectory(whRootPath.toString(), locationPath.toString()) || locationPath.equals(whRootPath)) { // legacy path
Review comment:
so prior to the introduction of managedlocation for db, while the exact semantics are unclear, the location was meant to be for managed tables. Although there is no code that enforces this strictly. So a legacy db object could have a location that could point anywhere.
This fix it for backward compatibility. When a location is set to a managed location, we are ensuring that that location now becomes the managedlocation.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] nrg4878 commented on pull request #2094: HIVE-24175: getDatabase() should only return managedlocation if set (…
Posted by GitBox <gi...@apache.org>.
nrg4878 commented on pull request #2094:
URL: https://github.com/apache/hive/pull/2094#issuecomment-803214576
recheck
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org