You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sentry.apache.org by sr...@apache.org on 2016/04/13 04:42:00 UTC
sentry git commit: SENTRY-1184: Clean up HMSPaths.renameAuthzObject (
Sravya Tirukkovalur, Reviewed by: Hao Hao)
Repository: sentry
Updated Branches:
refs/heads/master c4b057310 -> f4a9ad51f
SENTRY-1184: Clean up HMSPaths.renameAuthzObject ( Sravya Tirukkovalur, Reviewed by: Hao Hao)
Change-Id: I551a7310d077cfd7b338f7676bffeb89cf331aab
Project: http://git-wip-us.apache.org/repos/asf/sentry/repo
Commit: http://git-wip-us.apache.org/repos/asf/sentry/commit/f4a9ad51
Tree: http://git-wip-us.apache.org/repos/asf/sentry/tree/f4a9ad51
Diff: http://git-wip-us.apache.org/repos/asf/sentry/diff/f4a9ad51
Branch: refs/heads/master
Commit: f4a9ad51f5056696f7b14d821b30b3613a66af25
Parents: c4b0573
Author: Sravya Tirukkovalur <sr...@apache.org>
Authored: Wed Apr 6 12:06:46 2016 -0700
Committer: Sravya Tirukkovalur <sr...@apache.org>
Committed: Tue Apr 12 19:41:52 2016 -0700
----------------------------------------------------------------------
.../java/org/apache/sentry/hdfs/HMSPaths.java | 90 +++++++++++++-------
.../sentry/hdfs/UpdateableAuthzPaths.java | 13 +--
.../org/apache/sentry/hdfs/TestHMSPaths.java | 45 +++++++++-
.../sentry/hdfs/TestUpdateableAuthzPaths.java | 13 ++-
.../tests/e2e/hdfs/TestHDFSIntegration.java | 25 +++++-
5 files changed, 140 insertions(+), 46 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/sentry/blob/f4a9ad51/sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
----------------------------------------------------------------------
diff --git a/sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java b/sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
index ceb1da8..80a25aa 100644
--- a/sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
+++ b/sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
@@ -260,6 +260,9 @@ public class HMSPaths implements AuthzPaths {
if (prefix != null) {
// we only create the entry if is under a prefix, else we ignore it
entry = createChild(pathElements, EntryType.AUTHZ_OBJECT, authzObj);
+ } else {
+ LOG.info("Skipping to create authzObjPath as it is outside of prefix. authObj = " + authzObj
+ + " pathElements=" + pathElements);
}
return entry;
}
@@ -290,6 +293,7 @@ public class HMSPaths implements AuthzPaths {
}
}
+
public void delete() {
if (getParent() != null) {
if (getChildren().isEmpty()) {
@@ -469,15 +473,17 @@ public class HMSPaths implements AuthzPaths {
if (e != null) {
newEntries.add(e);
} else {
- LOG.warn("Ignoring path, no prefix");
+ LOG.info("Path outside prefix");
}
}
entries.addAll(newEntries);
} else {
if (createNew) {
addAuthzObject(authzObj, authzObjPathElements);
+ } else {
+ LOG.warn("Path was not added to AuthzObject, could not find key in authzObjToPath. authzObj = " + authzObj +
+ " authzObjPathElements=" + authzObjPathElements);
}
- LOG.warn("Object does not exist");
}
}
@@ -489,6 +495,11 @@ public class HMSPaths implements AuthzPaths {
addPathsToAuthzObject(authzObj, authzObjPaths, false);
}
+ /*
+ 1. Removes authzObj from all entries corresponding to the authzObjPathElements
+ ( which also deletes the entry if no more authObjs to that path and does it recursively upwards)
+ 2. Removes it from value of authzObjToPath Map for this authzObj key, does not reset entries to null even if entries is empty
+ */
void deletePathsFromAuthzObject(String authzObj,
List<List<String>> authzObjPathElements) {
Set<Entry> entries = authzObjToPath.get(authzObj);
@@ -501,12 +512,13 @@ public class HMSPaths implements AuthzPaths {
entry.deleteAuthzObject(authzObj);
toDelEntries.add(entry);
} else {
- LOG.warn("Ignoring path, it was not registered");
+ LOG.info("Path was not deleted from AuthzObject, path not registered. This is possible for implicit partition locations. authzObj = " + authzObj + " authzObjPathElements=" + authzObjPathElements);
}
}
entries.removeAll(toDelEntries);
} else {
- LOG.warn("Object does not exist");
+ LOG.info("Path was not deleted from AuthzObject, could not find key in authzObjToPath. authzObj = " + authzObj +
+ " authzObjPathElements=" + authzObjPathElements);
}
}
@@ -548,41 +560,59 @@ public class HMSPaths implements AuthzPaths {
return (entry != null) ? entry.getAuthzObjs() : null;
}
- boolean renameAuthzObject(String oldName, List<String> oldPathElems,
- String newName, List<String> newPathElems) {
- // Handle '/'
- if (oldPathElems == null || oldPathElems.size() == 0) {
- return false;
- }
- Entry entry =
- root.find(oldPathElems.toArray(new String[oldPathElems.size()]), false);
- if (entry != null && entry.getAuthzObjs().contains(oldName)) {
- // Update pathElements
- String[] newPath = newPathElems.toArray(new String[newPathElems.size()]);
- // Can't use Lists.newArrayList() because of whacky generics
- List<List<String>> pathElemsAsList = new LinkedList<List<String>>();
- pathElemsAsList.add(oldPathElems);
- deletePathsFromAuthzObject(oldName, pathElemsAsList);
- if (isUnderPrefix(newPath)) {
- // Can't use Lists.newArrayList() because of whacky generics
- pathElemsAsList = new LinkedList<List<String>>();
- pathElemsAsList.add(newPathElems);
- addPathsToAuthzObject(oldName, pathElemsAsList);
+ /*
+ Following condition should be true: oldName != newName
+ If oldPath == newPath, Example: rename external table (only HMS meta data is updated)
+ => new_table.add(new_path), new_table.add(old_table_partition_paths), old_table.dropAllPaths.
+ If oldPath != newPath, Example: rename managed table (HMS metadata is updated as well as physical files are moved to new location)
+ => new_table.add(new_path), old_table.dropAllPaths.
+ */
+ void renameAuthzObject(String oldName, List<List<String>> oldPathElems,
+ String newName, List<List<String>> newPathElems) {
+
+ if ( oldPathElems == null || oldPathElems.size() == 0 || newPathElems == null || newPathElems.size() == 0
+ || newName == null || oldName == null || oldName == newName) {
+ LOG.warn("Unexpected state in renameAuthzObject, inputs invalid: oldName=" + oldName + " newName=" + newName +
+ " oldPath=" + oldPathElems + " newPath=" + newPathElems);
+ return;
+ }
+
+ //new_table.add(new_path)
+ addPathsToAuthzObject(newName, newPathElems, true);
+
+ //if oldPath == newPath, that is path has not changed as part of rename and hence new table needs to have old paths
+ // => new_table.add(old_table_partition_paths)
+ List<String> oldPathElements = oldPathElems.get(0);
+ List<String> newPathElements = newPathElems.get(0);
+ boolean samePaths = false;
+ if(oldPathElements.size() == newPathElements.size()) {
+ samePaths = true;
+ for(int i=0; i<oldPathElements.size() ; i++) {
+ if (!newPathElements.get(i).equalsIgnoreCase(oldPathElements.get(i))) {
+ samePaths = false;
+ }
}
- // This would be true only for table rename
- if (!oldName.equals(newName)) {
- Set<Entry> eSet = authzObjToPath.get(oldName);
+ }
+ if(samePaths) {
+ Set<Entry> eSet = authzObjToPath.get(oldName);
+ if (eSet == null) {
+ LOG.warn("Unexpected state in renameAuthzObject, cannot find oldName in authzObjToPath: oldName=" + oldName + " newName=" + newName +
+ " oldPath=" + oldPathElems + " newPath=" + newPathElems);
+ } else {
authzObjToPath.put(newName, eSet);
for (Entry e : eSet) {
if (e.getAuthzObjs().contains(oldName)) {
- e.removeAuthzObj(oldName);
e.addAuthzObj(newName);
+ } else {
+ LOG.warn("Unexpected state in renameAuthzObject, authzObjToPath has an entry <oldName,entries> where one of the entry does not have oldName : oldName=" + oldName + " newName=" + newName +
+ " oldPath=" + oldPathElems + " newPath=" + newPathElems);
}
}
- authzObjToPath.remove(oldName);
}
}
- return true;
+
+ //old_table.dropAllPaths
+ deleteAuthzObject(oldName);
}
@Override
http://git-wip-us.apache.org/repos/asf/sentry/blob/f4a9ad51/sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
----------------------------------------------------------------------
diff --git a/sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java b/sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
index 8fc5470..5ff7294 100644
--- a/sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
+++ b/sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java
@@ -61,7 +61,7 @@ public class UpdateableAuthzPaths implements AuthzPaths, Updateable<PathsUpdate>
@Override
public UpdateableAuthzPaths updateFull(PathsUpdate update) {
UpdateableAuthzPaths other = getPathsDump().initializeFromDump(
- update.toThrift().getPathsDump());
+ update.toThrift().getPathsDump());
other.seqNum.set(update.getSeqNum());
return other;
}
@@ -87,8 +87,8 @@ public class UpdateableAuthzPaths implements AuthzPaths, Updateable<PathsUpdate>
}
private void applyPartialUpdate(PathsUpdate update) {
- // Handle alter table rename : will have exactly 2 patch changes
- // 1 an add path and the other a del path
+ // Handle alter table rename : will have exactly 2 path changes
+ // 1 is add path and the other is del path and oldName != newName
if (update.getPathChanges().size() == 2) {
List<TPathChanges> pathChanges = update.getPathChanges();
TPathChanges newPathInfo = null;
@@ -102,10 +102,11 @@ public class UpdateableAuthzPaths implements AuthzPaths, Updateable<PathsUpdate>
newPathInfo = pathChanges.get(1);
oldPathInfo = pathChanges.get(0);
}
- if (newPathInfo != null && oldPathInfo != null) {
+ if (newPathInfo != null && oldPathInfo != null &&
+ !newPathInfo.getAuthzObj().equalsIgnoreCase(oldPathInfo.getAuthzObj())) {
paths.renameAuthzObject(
- oldPathInfo.getAuthzObj(), oldPathInfo.getDelPaths().get(0),
- newPathInfo.getAuthzObj(), newPathInfo.getAddPaths().get(0));
+ oldPathInfo.getAuthzObj(), oldPathInfo.getDelPaths(),
+ newPathInfo.getAuthzObj(), newPathInfo.getAddPaths());
return;
}
}
http://git-wip-us.apache.org/repos/asf/sentry/blob/f4a9ad51/sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
----------------------------------------------------------------------
diff --git a/sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java b/sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
index bb74779..0dee102 100644
--- a/sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
+++ b/sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
@@ -17,7 +17,7 @@
*/
package org.apache.sentry.hdfs;
-import java.util.List;
+import java.util.*;
import org.apache.hadoop.fs.Path;
import org.junit.Assert;
@@ -353,6 +353,49 @@ public class TestHMSPaths {
Assert.assertEquals(prefix, root.findPrefixEntry(
Lists.newArrayList("a", "b", "t", "p3")));
}
+ @Test
+ public void testRenameDiffPaths() {
+ String[] prefixes = {"/user/hive/warehouse"};
+ HMSPaths paths = new HMSPaths(prefixes);
+ //Create old table and partition locations
+ String table1Path = "/user/hive/warehouse/db1.db/table1";
+ String partition1Path = "/user/hive/warehouse/db1.db/table1/part1";
+ paths.addAuthzObject("db1.table1", HMSPaths.getPathsElements(Arrays.asList(table1Path, partition1Path)));
+
+ //Create new table location
+ String table2Path = "/user/hive/warehouse/db2.db/table2";
+ paths.renameAuthzObject("db1.table1", HMSPaths.getPathsElements(Arrays.asList(table1Path)),
+ "db2.table2", HMSPaths.getPathsElements(Arrays.asList(table2Path)));
+
+ //Assert that old path is not associated with a table
+ Assert.assertEquals(null, paths.findAuthzObject(HMSPaths.getPathElements(table1Path).toArray(new String[0])));
+ Assert.assertEquals(null, paths.findAuthzObject(HMSPaths.getPathElements(partition1Path).toArray(new String[0])));
+ //Assert that new path is associated with new table
+ Set<String> expectedSet = new HashSet<String>();
+ expectedSet.add("db2.table2");
+ Assert.assertEquals(expectedSet, paths.findAuthzObject(HMSPaths.getPathElements(table2Path).toArray(new String[0])));
+
+ }
+
+ @Test
+ public void testRenameSamePaths() {
+ String[] prefixes = {"/user/hive/warehouse"};
+ HMSPaths paths = new HMSPaths(prefixes);
+ //Create table and partition locations
+ String tablePath = "/user/hive/warehouse/db1.db/table1";
+ String partitionPath = "/user/hive/warehouse/db1.db/table1/part1";
+ paths.addAuthzObject("db1.table1", HMSPaths.getPathsElements(Arrays.asList(tablePath, partitionPath)));
+
+ //Create new table
+ paths.renameAuthzObject("db1.table1", HMSPaths.getPathsElements(Arrays.asList(tablePath)),
+ "db2.table2", HMSPaths.getPathsElements(Arrays.asList(tablePath)));
+
+ //Assert that old path is associated with new table
+ Set<String> expectedSet = new HashSet<String>();
+ expectedSet.add("db2.table2");
+ Assert.assertEquals(expectedSet, paths.findAuthzObject(HMSPaths.getPathElements(tablePath).toArray(new String[0])));
+ Assert.assertEquals(expectedSet, paths.findAuthzObject(HMSPaths.getPathElements(partitionPath).toArray(new String[0])));
+ }
@Test
public void testAuthzObjCaseInsensitive() {
http://git-wip-us.apache.org/repos/asf/sentry/blob/f4a9ad51/sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java
----------------------------------------------------------------------
diff --git a/sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java b/sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java
index a5bc313..909ff3a 100644
--- a/sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java
+++ b/sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java
@@ -17,17 +17,15 @@
*/
package org.apache.sentry.hdfs;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertNull;
-
import java.util.concurrent.locks.ReentrantReadWriteLock;
import org.apache.sentry.hdfs.service.thrift.TPathChanges;
-import static org.junit.Assert.assertTrue;
import org.junit.Test;
import com.google.common.collect.Lists;
+import static org.junit.Assert.*;
+
public class TestUpdateableAuthzPaths {
@Test
@@ -105,10 +103,9 @@ public class TestUpdateableAuthzPaths {
// Verify name change
assertTrue(authzPaths.findAuthzObjectExactMatches(new String[]{"db1"}).contains("db1"));
assertTrue(authzPaths.findAuthzObjectExactMatches(new String[]{"db1", "xtbl11"}).contains("db1.xtbl11"));
- // Explicit set location has to be done on the partition else it will be associated to
- // the old location
- assertTrue(authzPaths.findAuthzObjectExactMatches(new String[]{"db1", "tbl11", "part111"}).contains("db1.xtbl11"));
- assertTrue(authzPaths.findAuthzObjectExactMatches(new String[]{"db1", "tbl11", "part112"}).contains("db1.xtbl11"));
+ // When both name and location are changed, old paths should not contain either new or old table name
+ assertNull(authzPaths.findAuthzObjectExactMatches(new String[]{"db1", "tbl11", "part111"}));
+ assertNull(authzPaths.findAuthzObjectExactMatches(new String[]{"db1", "tbl11", "part112"}));
// Verify other tables are not touched
assertNull(authzPaths.findAuthzObjectExactMatches(new String[]{"db1", "xtbl12"}));
assertNull(authzPaths.findAuthzObjectExactMatches(new String[]{"db1", "xtbl12", "part121"}));
http://git-wip-us.apache.org/repos/asf/sentry/blob/f4a9ad51/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
----------------------------------------------------------------------
diff --git a/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java b/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
index 4799d36..d726176 100644
--- a/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
+++ b/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
@@ -621,10 +621,14 @@ public class TestHDFSIntegration {
stmt.execute("revoke select on table p1 from role p1_admin");
verifyOnAllSubDirs("/user/hive/warehouse/p1", FsAction.WRITE_EXECUTE, "hbase", true);
- // Verify table rename works
+
+ // Verify table rename works when locations are also changed
stmt.execute("alter table p1 rename to p3");
verifyOnAllSubDirs("/user/hive/warehouse/p3", FsAction.WRITE_EXECUTE, "hbase", true);
+ //This is true as parent hive object's (p3) ACLS are used.
+ verifyOnAllSubDirs("/user/hive/warehouse/p3/month=1/day=1", FsAction.WRITE_EXECUTE, "hbase", true);
+ // Verify when oldName == newName and oldPath != newPath
stmt.execute("alter table p3 partition (month=1, day=1) rename to partition (month=1, day=3)");
verifyOnAllSubDirs("/user/hive/warehouse/p3", FsAction.WRITE_EXECUTE, "hbase", true);
verifyOnAllSubDirs("/user/hive/warehouse/p3/month=1/day=3", FsAction.WRITE_EXECUTE, "hbase", true);
@@ -816,6 +820,24 @@ public class TestHDFSIntegration {
verifyOnPath("/tmp/external/tables/ext2_after/i=2/stuff.txt", FsAction.ALL, "hbase", true);
// END : Verify external table set location..
+ //Create a new table partition on the existing partition
+ stmt.execute("create table tmp (s string) partitioned by (i int)");
+ stmt.execute("alter table tmp add partition (i=1)");
+ stmt.execute("alter table tmp partition (i=1) set location \'hdfs:///tmp/external/tables/ext2_after/i=1\'");
+ stmt.execute("grant all on table tmp to role tab_role");
+ verifyOnPath("/tmp/external/tables/ext2_after/i=1", FsAction.ALL, "flume", true);
+
+ //Alter table rename of external table => oldName != newName, oldPath == newPath
+ stmt.execute("alter table ext2 rename to ext3");
+ //Verify all original paths still have the privileges
+ verifyOnPath("/tmp/external/tables/ext2_after", FsAction.ALL, "hbase", true);
+ verifyOnPath("/tmp/external/tables/ext2_after/i=1", FsAction.ALL, "hbase", true);
+ verifyOnPath("/tmp/external/tables/ext2_after/i=1", FsAction.ALL, "flume", true);
+ verifyOnPath("/tmp/external/tables/ext2_after/i=2", FsAction.ALL, "hbase", true);
+ verifyOnPath("/tmp/external/tables/ext2_after/i=1/stuff.txt", FsAction.ALL, "hbase", true);
+ verifyOnPath("/tmp/external/tables/ext2_after/i=2/stuff.txt", FsAction.ALL, "hbase", true);
+
+
// Restart HDFS to verify if things are fine after re-start..
// TODO : this is currently commented out since miniDFS.restartNameNode() does
@@ -889,6 +911,7 @@ public class TestHDFSIntegration {
//Partition on local file system
stmt.execute("alter table tab2 add partition (i=1)");
stmt.execute("alter table tab2 partition (i=1) set location 'file:///tmp/external/tab2_loc/i=1'");
+
verifyOnAllSubDirs("/tmp/external/tab2_loc/i=1", null, StaticUserGroup.USERGROUP1, false);
//HDFS to local file system, also make sure does not specifying scheme still works