gaborkaszab commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1017726904
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -358,6 +361,85 @@ public void testCreateNamespace() throws Exception {
"There no same location for db and namespace",
database2.getLocationUri(), hiveLocalDir);
}
+ @Test
+ public void testCreateNamespaceWithOwnership() throws Exception {
+ createNamespaceAndVerifyOwnership(
+ "individual_ownership_1",
+ ImmutableMap.of(
+ HiveCatalog.HMS_DB_OWNER,
+ "apache",
+ HiveCatalog.HMS_DB_OWNER_TYPE,
+ PrincipalType.USER.name()),
+ "apache",
+ PrincipalType.USER);
+
+ createNamespaceAndVerifyOwnership(
+ "individual_ownership_2",
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER, "someone"),
+ "someone",
+ PrincipalType.USER);
+
+ createNamespaceAndVerifyOwnership(
+ "group_ownership",
+ ImmutableMap.of(
+ HiveCatalog.HMS_DB_OWNER,
+ "iceberg",
+ HiveCatalog.HMS_DB_OWNER_TYPE,
+ PrincipalType.GROUP.name()),
+ "iceberg",
+ PrincipalType.GROUP);
+
+ createNamespaceAndVerifyOwnership(
+ "default_ownership_1",
+ ImmutableMap.of(),
+ System.getProperty("user.name"),
+ PrincipalType.USER);
+
+ createNamespaceAndVerifyOwnership(
+ "default_ownership_2",
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER_TYPE,
PrincipalType.USER.name()),
Review Comment:
I believe we shouldn't differentiate between the default value and
non-default value. If the owner type is given but the owner is not then we
should raise an error.
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -426,6 +508,167 @@ public void testSetNamespaceProperties() throws
TException {
});
}
+ @Test
+ public void testSetNamespaceOwnership() throws TException {
+ setNamespaceOwnershipAndVerify(
+ "set_individual_ownership_with_no_ownership_on_create",
+ ImmutableMap.of(),
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER, "some_individual_owner"),
+ System.getProperty("user.name"),
+ PrincipalType.USER,
+ "some_individual_owner",
+ PrincipalType.USER);
+
+ setNamespaceOwnershipAndVerify(
+ "change_individual_ownership",
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER, "some_individual_owner"),
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER,
"some_other_individual_owner"),
+ "some_individual_owner",
+ PrincipalType.USER,
+ "some_other_individual_owner",
+ PrincipalType.USER);
+
+ setNamespaceOwnershipAndVerify(
+ "change_individual_to_group_ownership_1",
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER, "some_owner"),
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER_TYPE,
PrincipalType.GROUP.name()),
Review Comment:
Shouldn't we also get an error here when setting owner type only? Is there a
chance that there is going to be a user and a group with the same name? I'd bet
it's highly unlikely so to keep the logic consistent I'd vote for raising an
error here as well.
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -426,6 +508,167 @@ public void testSetNamespaceProperties() throws
TException {
});
}
+ @Test
+ public void testSetNamespaceOwnership() throws TException {
+ setNamespaceOwnershipAndVerify(
+ "set_individual_ownership_with_no_ownership_on_create",
+ ImmutableMap.of(),
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER, "some_individual_owner"),
+ System.getProperty("user.name"),
+ PrincipalType.USER,
+ "some_individual_owner",
+ PrincipalType.USER);
+
+ setNamespaceOwnershipAndVerify(
+ "change_individual_ownership",
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER, "some_individual_owner"),
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER,
"some_other_individual_owner"),
+ "some_individual_owner",
+ PrincipalType.USER,
+ "some_other_individual_owner",
+ PrincipalType.USER);
+
+ setNamespaceOwnershipAndVerify(
+ "change_individual_to_group_ownership_1",
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER, "some_owner"),
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER_TYPE,
PrincipalType.GROUP.name()),
+ "some_owner",
+ PrincipalType.USER,
+ "some_owner",
+ PrincipalType.GROUP);
+
+ setNamespaceOwnershipAndVerify(
+ "change_individual_to_group_ownership_2",
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER, "some_owner"),
+ ImmutableMap.of(
+ HiveCatalog.HMS_DB_OWNER,
+ "some_group_owner",
+ HiveCatalog.HMS_DB_OWNER_TYPE,
+ PrincipalType.GROUP.name()),
+ "some_owner",
+ PrincipalType.USER,
+ "some_group_owner",
+ PrincipalType.GROUP);
+
+ setNamespaceOwnershipAndVerify(
+ "set_group_ownership_with_no_ownership_on_create",
+ ImmutableMap.of(),
+ ImmutableMap.of(
+ HiveCatalog.HMS_DB_OWNER,
+ "some_group_owner",
+ HiveCatalog.HMS_DB_OWNER_TYPE,
+ PrincipalType.GROUP.name()),
+ System.getProperty("user.name"),
+ PrincipalType.USER,
+ "some_group_owner",
+ PrincipalType.GROUP);
+
+ setNamespaceOwnershipAndVerify(
+ "change_group_ownership",
+ ImmutableMap.of(
+ HiveCatalog.HMS_DB_OWNER,
+ "some_group_owner",
+ HiveCatalog.HMS_DB_OWNER_TYPE,
+ PrincipalType.GROUP.name()),
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER, "some_other_group_owner"),
+ "some_group_owner",
+ PrincipalType.GROUP,
+ "some_other_group_owner",
+ PrincipalType.GROUP);
+
+ setNamespaceOwnershipAndVerify(
+ "change_group_to_individual_ownership_1",
+ ImmutableMap.of(
+ HiveCatalog.HMS_DB_OWNER,
+ "some_owner",
+ HiveCatalog.HMS_DB_OWNER_TYPE,
+ PrincipalType.GROUP.name()),
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER_TYPE,
PrincipalType.USER.name()),
Review Comment:
See my comment above.
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -448,6 +691,153 @@ public void testRemoveNamespaceProperties() throws
TException {
});
}
+ @Test
+ public void testRemoveNamespaceOwnership() throws TException {
+ removeNamespaceOwnershipAndVerify(
+ "remove_individual_ownership_1",
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER, "some_owner"),
+ ImmutableSet.of(HiveCatalog.HMS_DB_OWNER,
HiveCatalog.HMS_DB_OWNER_TYPE),
+ "some_owner",
+ PrincipalType.USER,
+ System.getProperty("user.name"),
+ PrincipalType.USER);
+
+ removeNamespaceOwnershipAndVerify(
+ "remove_individual_ownership_2",
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER, "some_owner"),
+ ImmutableSet.of(HiveCatalog.HMS_DB_OWNER),
+ "some_owner",
+ PrincipalType.USER,
+ System.getProperty("user.name"),
+ PrincipalType.USER);
+
+ removeNamespaceOwnershipAndVerify(
+ "remove_group_ownership",
+ ImmutableMap.of(
+ HiveCatalog.HMS_DB_OWNER,
+ "some_group_owner",
+ HiveCatalog.HMS_DB_OWNER_TYPE,
+ PrincipalType.GROUP.name()),
+ ImmutableSet.of(HiveCatalog.HMS_DB_OWNER,
HiveCatalog.HMS_DB_OWNER_TYPE),
+ "some_group_owner",
+ PrincipalType.GROUP,
+ System.getProperty("user.name"),
+ PrincipalType.USER);
+
+ removeNamespaceOwnershipAndVerify(
+ "remove_ownership_with_no_ownership_on_create_noop_0",
+ ImmutableMap.of(),
+ ImmutableSet.of(HiveCatalog.HMS_DB_OWNER,
HiveCatalog.HMS_DB_OWNER_TYPE),
+ System.getProperty("user.name"),
+ PrincipalType.USER,
+ System.getProperty("user.name"),
+ PrincipalType.USER);
+
+ removeNamespaceOwnershipAndVerify(
+ "remove_ownership_with_no_ownership_on_create_noop_1",
+ ImmutableMap.of(),
+ ImmutableSet.of(HiveCatalog.HMS_DB_OWNER),
+ System.getProperty("user.name"),
+ PrincipalType.USER,
+ System.getProperty("user.name"),
+ PrincipalType.USER);
+
+ removeNamespaceOwnershipAndVerify(
+ "remove_ownership_with_no_ownership_on_create_noop_2",
+ ImmutableMap.of(),
+ ImmutableSet.of(HiveCatalog.HMS_DB_OWNER_TYPE),
+ System.getProperty("user.name"),
+ PrincipalType.USER,
+ System.getProperty("user.name"),
+ PrincipalType.USER);
+
+ removeNamespaceOwnershipAndVerify(
+ "remove_ownership_with_no_ownership_on_create_noop_3",
+ ImmutableMap.of(),
+ ImmutableSet.of(),
+ System.getProperty("user.name"),
+ PrincipalType.USER,
+ System.getProperty("user.name"),
+ PrincipalType.USER);
+
+ removeNamespaceOwnershipAndVerify(
+ "remove_ownership_noop_0",
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER, "some_owner"),
+ ImmutableSet.of(HiveCatalog.HMS_DB_OWNER_TYPE),
+ "some_owner",
+ PrincipalType.USER,
+ "some_owner",
+ PrincipalType.USER);
+
+ removeNamespaceOwnershipAndVerify(
+ "remove_ownership_noop_1",
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER, "some_owner"),
+ ImmutableSet.of(),
+ "some_owner",
+ PrincipalType.USER,
+ "some_owner",
+ PrincipalType.USER);
+
+ removeNamespaceOwnershipAndVerify(
+ "remove_ownership_noop_2",
+ ImmutableMap.of(
+ HiveCatalog.HMS_DB_OWNER,
+ "some_group_owner",
+ HiveCatalog.HMS_DB_OWNER_TYPE,
+ PrincipalType.GROUP.name()),
+ ImmutableSet.of(),
+ "some_group_owner",
+ PrincipalType.GROUP,
+ "some_group_owner",
+ PrincipalType.GROUP);
+
+ AssertHelpers.assertThrows(
+ "Setting non default value for "
Review Comment:
This error message seems off from the users perspective as they tried to
remove the DB_OWNER property and then they get an error message that tells
about setting the DB_OWNER_TYPE property.
The problem might by implementation-wise is that these checks are in the
convertToDatabase() function that is in turned called by 3 different purpose
use-cases. Would it make sense to do the checks on the callsites of
convertToDatabase() instead and then they could be fine tuned for the actual
purpose?
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -448,6 +691,153 @@ public void testRemoveNamespaceProperties() throws
TException {
});
}
+ @Test
+ public void testRemoveNamespaceOwnership() throws TException {
+ removeNamespaceOwnershipAndVerify(
+ "remove_individual_ownership_1",
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER, "some_owner"),
+ ImmutableSet.of(HiveCatalog.HMS_DB_OWNER,
HiveCatalog.HMS_DB_OWNER_TYPE),
+ "some_owner",
+ PrincipalType.USER,
+ System.getProperty("user.name"),
+ PrincipalType.USER);
+
+ removeNamespaceOwnershipAndVerify(
+ "remove_individual_ownership_2",
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER, "some_owner"),
+ ImmutableSet.of(HiveCatalog.HMS_DB_OWNER),
+ "some_owner",
+ PrincipalType.USER,
+ System.getProperty("user.name"),
+ PrincipalType.USER);
+
+ removeNamespaceOwnershipAndVerify(
+ "remove_group_ownership",
+ ImmutableMap.of(
+ HiveCatalog.HMS_DB_OWNER,
+ "some_group_owner",
+ HiveCatalog.HMS_DB_OWNER_TYPE,
+ PrincipalType.GROUP.name()),
+ ImmutableSet.of(HiveCatalog.HMS_DB_OWNER,
HiveCatalog.HMS_DB_OWNER_TYPE),
+ "some_group_owner",
+ PrincipalType.GROUP,
+ System.getProperty("user.name"),
+ PrincipalType.USER);
+
+ removeNamespaceOwnershipAndVerify(
+ "remove_ownership_with_no_ownership_on_create_noop_0",
+ ImmutableMap.of(),
+ ImmutableSet.of(HiveCatalog.HMS_DB_OWNER,
HiveCatalog.HMS_DB_OWNER_TYPE),
+ System.getProperty("user.name"),
+ PrincipalType.USER,
+ System.getProperty("user.name"),
+ PrincipalType.USER);
+
+ removeNamespaceOwnershipAndVerify(
+ "remove_ownership_with_no_ownership_on_create_noop_1",
+ ImmutableMap.of(),
+ ImmutableSet.of(HiveCatalog.HMS_DB_OWNER),
+ System.getProperty("user.name"),
+ PrincipalType.USER,
+ System.getProperty("user.name"),
+ PrincipalType.USER);
+
+ removeNamespaceOwnershipAndVerify(
+ "remove_ownership_with_no_ownership_on_create_noop_2",
+ ImmutableMap.of(),
+ ImmutableSet.of(HiveCatalog.HMS_DB_OWNER_TYPE),
+ System.getProperty("user.name"),
+ PrincipalType.USER,
+ System.getProperty("user.name"),
+ PrincipalType.USER);
+
+ removeNamespaceOwnershipAndVerify(
+ "remove_ownership_with_no_ownership_on_create_noop_3",
+ ImmutableMap.of(),
+ ImmutableSet.of(),
+ System.getProperty("user.name"),
+ PrincipalType.USER,
+ System.getProperty("user.name"),
+ PrincipalType.USER);
+
+ removeNamespaceOwnershipAndVerify(
+ "remove_ownership_noop_0",
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER, "some_owner"),
+ ImmutableSet.of(HiveCatalog.HMS_DB_OWNER_TYPE),
Review Comment:
In case we only remove the OWNER_TYPE is there a chance that we expect that
there is going to be a USER with the same name as the GROUP has?
I feel that to keep consistency adding-removing ownership should be always
using the owner only, or with the owner + owner type, but shouldn't be using
owner type only.
I'm open to hear other opinions, though.
--
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]
