gaborkaszab commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1018774071
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -365,6 +374,13 @@ public boolean dropNamespace(Namespace namespace) {
@Override
public boolean setProperties(Namespace namespace, Map<String, String>
properties) {
+ Preconditions.checkArgument(
+ (properties.get(HMS_DB_OWNER_TYPE) == null) ==
(properties.get(HMS_DB_OWNER) == null),
+ "Setting "
Review Comment:
nit: Here we could use the same error message as in createNamespace() if we
decide that the same condition can be used.
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -426,6 +510,194 @@ public void testSetNamespaceProperties() throws
TException {
});
}
+ @Test
+ public void testSetNamespaceOwnership() throws TException {
+ setNamespaceOwnershipAndVerify(
+ "set_individual_ownership_on_default_owner",
+ ImmutableMap.of(),
+ ImmutableMap.of(
+ HiveCatalog.HMS_DB_OWNER,
+ "some_individual_owner",
+ HiveCatalog.HMS_DB_OWNER_TYPE,
+ PrincipalType.USER.name()),
+ System.getProperty("user.name"),
+ PrincipalType.USER,
+ "some_individual_owner",
+ PrincipalType.USER);
+
+ setNamespaceOwnershipAndVerify(
+ "set_group_ownership_on_default_owner",
+ 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_individual_to_group_ownership",
+ 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(
+ "change_group_to_individual_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_individual_owner",
+ HiveCatalog.HMS_DB_OWNER_TYPE,
+ PrincipalType.USER.name()),
+ "some_group_owner",
+ PrincipalType.GROUP,
+ "some_individual_owner",
+ PrincipalType.USER);
+
+ AssertHelpers.assertThrows(
+ "Setting "
+ + HiveCatalog.HMS_DB_OWNER_TYPE
+ + " and "
+ + HiveCatalog.HMS_DB_OWNER
+ + " has to be performed together or not at all",
+ IllegalArgumentException.class,
+ () -> {
+ try {
+ setNamespaceOwnershipAndVerify(
+ "set_owner_without_setting_owner_type",
+ ImmutableMap.of(),
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER,
"some_individual_owner"),
+ System.getProperty("user.name"),
+ PrincipalType.USER,
+ "no_post_setting_expectation_due_to_exception_thrown",
+ null);
+ } catch (TException e) {
+ throw new RuntimeException("Unexpected Exception", e);
+ }
+ });
+
+ AssertHelpers.assertThrows(
+ "Setting "
+ + HiveCatalog.HMS_DB_OWNER_TYPE
+ + " and "
+ + HiveCatalog.HMS_DB_OWNER
+ + " has to be performed together or not at all",
+ IllegalArgumentException.class,
+ () -> {
+ try {
+ setNamespaceOwnershipAndVerify(
+ "set_owner_type_without_setting_owner",
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER, "some_owner"),
+ ImmutableMap.of(HiveCatalog.HMS_DB_OWNER_TYPE,
PrincipalType.GROUP.name()),
+ "some_owner",
+ PrincipalType.USER,
+ "no_post_setting_expectation_due_to_exception_thrown",
+ null);
+ } catch (TException e) {
+ throw new RuntimeException("Unexpected Exception", e);
+ }
+ });
+
+ AssertHelpers.assertThrows(
+ HiveCatalog.HMS_DB_OWNER_TYPE
+ + " has an invalid value of: "
+ + meta.get(HiveCatalog.HMS_DB_OWNER_TYPE)
+ + ". Acceptable values are: "
+ +
Stream.of(PrincipalType.values()).map(Enum::name).collect(Collectors.joining(",
")),
+ IllegalArgumentException.class,
+ () -> {
+ try {
+ setNamespaceOwnershipAndVerify(
+ "set_invalid_owner_type",
+ ImmutableMap.of(),
+ ImmutableMap.of(
+ HiveCatalog.HMS_DB_OWNER, "iceberg",
+ HiveCatalog.HMS_DB_OWNER_TYPE, "invalidOwnerType"),
+ System.getProperty("user.name"),
+ PrincipalType.USER,
+ "no_post_setting_expectation_due_to_exception_thrown",
+ null);
+ } catch (TException e) {
+ throw new RuntimeException("Unexpected Exception", e);
+ }
+ });
+ }
+
+ @Test
+ public void testSetNamespaceOwnershipNoop() throws TException {
Review Comment:
One additional use-case comes to my mind, where we create the namespace with
some ownership and then run an unrelated setProperties() (can be with empty
properties or using something ownership unrelated property) and check that we
still have the ownership we provided at creation.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -365,6 +374,13 @@ public boolean dropNamespace(Namespace namespace) {
@Override
public boolean setProperties(Namespace namespace, Map<String, String>
properties) {
+ Preconditions.checkArgument(
+ (properties.get(HMS_DB_OWNER_TYPE) == null) ==
(properties.get(HMS_DB_OWNER) == null),
Review Comment:
Just thinking out load but would it make sense to set DB owner without
setting the owner type? We do that in createNamespace() and it seems just fine.
Why is setProperties() different? I thought that there are 2 valid use-cases:
We either set the owner only, or the owner and the owner type together, but
never the owner type alone.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
