HIVE-28848:Remove DFS_URI auth from ALTER_PARTITION if there is no ch… #5766
Conversation
|
| Exception ex = null; | ||
| try { | ||
| Table table = getMS().getTable(catName, db_name, tbl_name, null); | ||
| Partition oldPartition = getMS().getPartition(catName, db_name, tbl_name, part_vals); |
There was a problem hiding this comment.
nit: we fetch oldPart at L6062, can you see if you can eliminate this redundant call?
There was a problem hiding this comment.
I removed this redundant call from HMSHandler.
| if (StringUtils.isNotEmpty(newUri)) { | ||
| ret.add(getHivePrivilegeObjectDfsUri(newUri)); | ||
| // Skip DFS_URI auth if old and new partition uri are empty or same | ||
| if (StringUtils.isNotEmpty(newUri) && StringUtils.isNotEmpty(oldUri) && !StringUtils.equalsIgnoreCase(oldUri, |
There was a problem hiding this comment.
Do we need DFS_URI auth when newUri is not empty but oldUri is empty? Also consider add unit test for this change.
There was a problem hiding this comment.
change the condition to StringUtils.isNotEmpty(newUri) & !StringUtils.equalsIgnoreCase(oldUri, newUri) ?
There was a problem hiding this comment.
@wecharyu @dengzhhu653 We want to skip DFS_URI auth when newUri is not empty but oldUri is empty like in case of stats update during LOAD PARTITION, it would have empty old uri and non-empty new URI.
But I see your point this condition would be anyways covered by !StringUtils.equalsIgnoreCase(oldUri, newUri).
There was a problem hiding this comment.
Not quite understand why not need check the newUri auth when oldUri is empty but newUri is not.
But I see your point this condition would be anyways covered by !StringUtils.equalsIgnoreCase(oldUri, newUri).
But the check would fail-fast by StringUtils.isNotEmpty(oldUri) in your current implementation.
There was a problem hiding this comment.
@wecharyu I have updated the condition to StringUtils.isNotEmpty(newUri) & !StringUtils.equalsIgnoreCase(oldUri, newUri) now. And this patch proposes skipping DFS_URI authorization if there is no change in partition location. So, if new URI is non-empty and old uri is empty , ALTER PARTITION might be triggered for other HiveOperationType like ALTERPARTITION_FILEFORMAT, ALTERPARTITION_SERDEPROPERTIES which does not require DFS_URI auth.
...pache/hadoop/hive/ql/security/authorization/plugin/metastore/events/AlterPartitionEvent.java
Outdated
Show resolved
Hide resolved
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
asf-ci-hive
added
tests pending
tests unstable
and removed
tests unstable
tests pending
labels
|
|
||
| if (StringUtils.isNotEmpty(newUri)) { | ||
| ret.add(getHivePrivilegeObjectDfsUri(newUri)); | ||
| if (event.getOldPartVals() != null && event.getOldPartVals().size() > 0) { |
There was a problem hiding this comment.
should change null to tmpPart.getValues in this line: https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java#L6184?
There was a problem hiding this comment.
We don't have any changes in HMSHandler as part of this change. IIUC null is sent for oldPartition values and tmpPart is new partition here https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java#L6184
There was a problem hiding this comment.
The oldPartition values are the same as the tmpPart values in this method
...ache/hadoop/hive/ql/security/authorization/plugin/metastore/TestHiveMetaStoreAuthorizer.java
Outdated
Show resolved
Hide resolved
...ore-server/src/main/java/org/apache/hadoop/hive/metastore/events/PreAlterPartitionEvent.java
Show resolved
Hide resolved
asf-ci-hive
added
tests unstable
tests pending
and removed
tests pending
tests unstable
labels
|
@dengzhhu653 Thanks for the review! I have addressed review comments. |
asf-ci-hive
added
tests pending
tests failed
and removed
tests pending
tests failed
labels
…ange in partition location
asf-ci-hive
added
tests failed
tests pending
and removed
tests pending
tests failed
labels
|
5 participants
…ange in partition location
What changes were proposed in this pull request?
This change skips the DFS_URI authorization if there is no change in the PARTITION location in the ALTER_PARTITION event.
If the old partition and new partition Uri are the same or the old partition location is empty, then skip adding DFS_URI for authorization.
Why are the changes needed?
The LOAD DATA command invokes the ALTER_PARTITION event to update stats for the newly loaded partition. ALTER_PARTITION authorizes DFS_URI for the partition location even when the partition locations do not change. This requires the Hadoop SQL policy for DFS_URI to be defined for the partition location for LOAD DATA commands.
Does this PR introduce any user-facing change?
Yes, after this change, users will no longer need the DFS_URI policy for target table partition location.
Is the change a dependency upgrade?
No
How was this patch tested?
Manual testing