gaborkaszab commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1011702120
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -539,12 +541,22 @@ Database convertToDatabase(Namespace namespace,
Map<String, String> meta) {
database.setDescription(value);
} else if (key.equals("location")) {
database.setLocationUri(value);
+ } else if (key.equals(TableProperties.HMS_DB_OWNER)) {
+ database.setOwnerName(value);
+ } else if (key.equals(TableProperties.HMS_DB_OWNER_TYPE) && value !=
null) {
+ database.setOwnerType(PrincipalType.valueOf(value));
Review Comment:
I believe that valueOf() can throw an IllegalArgumentException if there is
no such enum value as the input parameter. This exception might be handled on
the API level but we might want to take care of it more gracefully here.
Additionally, some test coverage for this would be great.
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -358,6 +359,63 @@ public void testCreateNamespace() throws Exception {
"There no same location for db and namespace",
database2.getLocationUri(), hiveLocalDir);
}
+ @Test
+ public void testCreateNamespaceWithOwnership() throws Exception {
+ Map<String, String> prop =
+ ImmutableMap.of(
+ TableProperties.HMS_DB_OWNER,
+ "apache",
+ TableProperties.HMS_DB_OWNER_TYPE,
+ PrincipalType.USER.name());
+
+ String expectedOwner = "apache";
+ PrincipalType expectedOwnerType = PrincipalType.USER;
+
+ createNamespaceAndVerifyOwnership("userOwnership", prop, expectedOwner,
expectedOwnerType);
+
+ prop =
+ ImmutableMap.of(
+ TableProperties.HMS_DB_OWNER,
+ "iceberg",
+ TableProperties.HMS_DB_OWNER_TYPE,
+ PrincipalType.GROUP.name());
+ expectedOwner = "iceberg";
+ expectedOwnerType = PrincipalType.GROUP;
+
+ createNamespaceAndVerifyOwnership("groupOwnership", prop, expectedOwner,
expectedOwnerType);
+
+ prop = ImmutableMap.of(TableProperties.HMS_DB_OWNER, "someone");
+ expectedOwner = "someone";
+ expectedOwnerType = PrincipalType.USER; // default is user if not specified
+
+ createNamespaceAndVerifyOwnership("ownedBySomeone", prop, expectedOwner,
expectedOwnerType);
+
+ prop = ImmutableMap.of();
+ expectedOwner = System.getProperty("user.name"); // default value if not
specified
+ expectedOwnerType = PrincipalType.USER; // default is user if not specified
+
+ createNamespaceAndVerifyOwnership("ownedByDefaultUser", prop,
expectedOwner, expectedOwnerType);
+
+ prop = ImmutableMap.of(TableProperties.HMS_DB_OWNER_TYPE,
PrincipalType.GROUP.name());
Review Comment:
Just thinking out loud, but shouldn't we indicate failure if only the owner
type is set without the owner, instead of silently omitting it?
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -448,6 +540,36 @@ public void testRemoveNamespaceProperties() throws
TException {
});
}
+ @Test
+ public void testRemoveNamespaceOwnership() throws TException {
+ Map<String, String> prop = ImmutableMap.of(TableProperties.HMS_DB_OWNER,
"some_owner");
+ removeNamespaceOwnershipAndVerify("remove_individual_ownership", prop);
+ prop =
+ ImmutableMap.of(
+ TableProperties.HMS_DB_OWNER,
+ "some_owner",
+ TableProperties.HMS_DB_OWNER_TYPE,
+ PrincipalType.GROUP.name());
+ removeNamespaceOwnershipAndVerify("remove_group_ownership", prop);
+ }
+
+ private void removeNamespaceOwnershipAndVerify(String name, Map<String,
String> prop)
Review Comment:
This test has a precondition in a sense that prop is expected to hold some
username and owner type that is going to be used when creating a namespace. For
reflecting this I'd add an extra check right after the createNamespace() call
that these are reflected in the DB ownership (before we remove these
properties).
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -426,6 +484,40 @@ public void testSetNamespaceProperties() throws TException
{
});
}
+ @Test
+ public void testSetNamespaceOwnership() throws TException {
+ Map<String, String> prop =
+ ImmutableMap.of(TableProperties.HMS_DB_OWNER, "some_individual_owner");
+ String expectedOwner = "some_individual_owner";
+ PrincipalType expectedType = PrincipalType.USER;
+
+ setNamespaceOwnershipAndVerify("set_individual_ownership", prop,
expectedOwner, expectedType);
+
+ prop =
+ ImmutableMap.of(
+ TableProperties.HMS_DB_OWNER,
+ "some_group_owner",
+ TableProperties.HMS_DB_OWNER_TYPE,
+ PrincipalType.GROUP.name());
+ expectedOwner = "some_group_owner";
+ expectedType = PrincipalType.GROUP;
+
+ setNamespaceOwnershipAndVerify("set_group_ownership", prop, expectedOwner,
expectedType);
+ }
+
+ private void setNamespaceOwnershipAndVerify(
+ String name, Map<String, String> prop, String expectedOwner,
PrincipalType expectedType)
+ throws TException {
+ Namespace namespace = Namespace.of(name);
+ catalog.createNamespace(namespace);
+ catalog.setProperties(namespace, prop);
+ Database database = metastoreClient.getDatabase(namespace.level(0));
+
+ // Once ownership is removed, expect the ownership and type to fall back
to the default value
Review Comment:
nit: This comment is rather valid for the "removeOwnership" test and not for
the "setOwnership" test, right?
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -358,6 +359,63 @@ public void testCreateNamespace() throws Exception {
"There no same location for db and namespace",
database2.getLocationUri(), hiveLocalDir);
}
+ @Test
+ public void testCreateNamespaceWithOwnership() throws Exception {
+ Map<String, String> prop =
+ ImmutableMap.of(
+ TableProperties.HMS_DB_OWNER,
+ "apache",
+ TableProperties.HMS_DB_OWNER_TYPE,
+ PrincipalType.USER.name());
+
+ String expectedOwner = "apache";
+ PrincipalType expectedOwnerType = PrincipalType.USER;
Review Comment:
nit: I think its more readable to not introduce variables here for the
expected result but provide the expected values right at the test runner
function call. Might be only a personal preference, though.
##########
core/src/main/java/org/apache/iceberg/TableProperties.java:
##########
@@ -361,4 +361,6 @@ private TableProperties() {}
public static final boolean UPSERT_ENABLED_DEFAULT = false;
public static final String HMS_TABLE_OWNER = "hms_table_owner";
+ public static final String HMS_DB_OWNER = "hms_db_owner";
Review Comment:
@danielcweeks yes, currently the table owner is for "USER" types.
About the naming, when I introduced HMS_TABLE_OWNER frankly I wasn't aware
of the existing naming convention, so I'm absolutely open to rename them to
follow how other properties are named.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
