You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/10/25 04:39:02 UTC
[GitHub] [iceberg] haizhou-zhao opened a new pull request, #6045: [iceberg-hive-metastore] Add support for group ownership
haizhou-zhao opened a new pull request, #6045:
URL: https://github.com/apache/iceberg/pull/6045
Build on top of this PR: https://github.com/apache/iceberg/pull/5763
This is to add group ownership support to iceberg-hive-metastore
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] gaborkaszab commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1021195893
##########
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:
I think you're right. With setOwneship() we had to be aware of the current state in case we gave only the owner without the owner type. I'm fine with as it is now, thanks!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1022032530
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -539,12 +577,22 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
database.setDescription(value);
} else if (key.equals("location")) {
database.setLocationUri(value);
+ } else if (key.equals(HMS_DB_OWNER)) {
+ database.setOwnerName(value);
+ } else if (key.equals(HMS_DB_OWNER_TYPE) && value != null) {
+ database.setOwnerType(PrincipalType.valueOf(value));
} else {
if (value != null) {
parameter.put(key, value);
}
}
});
+
+ if (database.getOwnerName() == null) {
+ database.setOwnerName(System.getProperty("user.name"));
Review Comment:
Hi, its probably my fault here, @haizhou-zhao had this initially but it seemed there was some unresolved concern at the time : https://github.com/apache/iceberg/pull/6045#discussion_r1004221149 , so I actually asked @haizhou-zhao to follow what the table implementation uses for consistency , and we could change both to UserGroupInformation.getCurrentUser() later once there's consensus.
But looks like its cleared up since then, @gaborkaszab ? If so we can go ahead and use UserGroupInformation.getCurrentUser() here and then change table to be consistent as well?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#issuecomment-1318326348
Thought so, no worries :), thought it was worth a try
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on pull request #6045: [iceberg-hive-metastore] Add support for group ownership
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#issuecomment-1297574296
Took a look with @haizhou-zhao , I think we can make the following changes with to make this patch more mergeable and address some of the review comments:
- Split out changing user.name => UserGroupInformation, as there seems to be some questions
- Only set the type if we are on Hive 3 (using MetastoreUtil for version-specific code)
So then the change will only set owner, type on HiveDb for createNamespace, if the user passes it in via properties. Owner will take from user.name if that is not passed it. This will be in line with the table today.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1010590993
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -437,6 +451,18 @@ private Table newHmsTable(TableMetadata metadata) {
null,
TableType.EXTERNAL_TABLE.toString());
+ // hive2 does not support setting table ownership type, therefore we leave it as
+ // a table property. Once upgraded to hive3, then we can simply set
+ // newTable.setOwnerType(PrincipalType.valueOf(metadata.get(HMS_TABLE_OWNER_TYPE)))
+ String ownerType = metadata.property(TableProperties.HMS_TABLE_OWNER_TYPE, null);
+ if (ownerType != null && ownerType.equalsIgnoreCase(PrincipalType.GROUP.name())) {
+ newTable
+ .getParameters()
+ .put(TableProperties.HMS_TABLE_OWNER_TYPE, PrincipalType.GROUP.name());
Review Comment:
Decided to not support group ownership for tables here since hive2 does not support it. Hopefully this will simplify this PR. So this thread may not be relevant anymore.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for group ownership
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1004956670
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -532,13 +536,27 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
database.setName(namespace.level(0));
database.setLocationUri(databaseLocation(namespace.level(0)));
+ // default ownership is determined from Hadoop.UGI.currentUser
+ // and the owner type is default to individual user
+ try {
+ // getShortUserName will remove the realm part of the whole username
+ database.setOwnerName(UserGroupInformation.getCurrentUser().getShortUserName());
+ database.setOwnerType(PrincipalType.USER);
+ } catch (IOException e) {
+ LOG.error("Unable to determine the current UGI user", e);
+ throw new RuntimeException(e);
Review Comment:
I'm good with not setting these fields. Additionally, I think we should keep the behavior the same between database and table.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1026348753
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -356,6 +365,11 @@ 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),
+ String.format(
Review Comment:
Ah sorry should have been more clear, Preconditions(..., "%s %s", myVarA, myVarB) should work, that's what I meant! (see the other Iceberg uses for example)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] gaborkaszab commented on pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#issuecomment-1318275597
> @gaborkaszab sorry i know we are super close, but do you think we could change both to use UserGroupInformation quickly before cutting final 1.1 RC? That way no need to change behavior.
@szehon-ho Frankly we already had 2 RCs and we'll only create a third one because of a test failure so I wouldn't add another dependency for the release and do the cut early today to get the voting going again as early as possible.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1016944554
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -448,6 +575,59 @@ public void testRemoveNamespaceProperties() throws TException {
});
}
+ @Test
+ public void testRemoveNamespaceOwnership() throws TException {
+ Map<String, String> prop = ImmutableMap.of(HiveCatalog.HMS_DB_OWNER, "some_owner");
+ removeNamespaceOwnershipAndVerify("remove_individual_ownership", prop);
+
+ prop =
+ ImmutableMap.of(
+ HiveCatalog.HMS_DB_OWNER,
+ "some_owner",
+ HiveCatalog.HMS_DB_OWNER_TYPE,
+ PrincipalType.GROUP.name());
+ removeNamespaceOwnershipAndVerify("remove_group_ownership", prop);
+
+ prop = ImmutableMap.of();
+
+ removeNamespaceOwnershipAndVerify("remove_ownership_noop", prop);
Review Comment:
em, good idea.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] gaborkaszab commented on pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#issuecomment-1309096900
@danielcweeks I made a separate PR to rename the one existing property so we won't break backward compatibility with this patch. It has already gone in https://github.com/apache/iceberg/pull/6154 so I believe this one shouldn't be a blocker for 1.1.0.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] gaborkaszab commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1022696908
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -539,12 +577,22 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
database.setDescription(value);
} else if (key.equals("location")) {
database.setLocationUri(value);
+ } else if (key.equals(HMS_DB_OWNER)) {
+ database.setOwnerName(value);
+ } else if (key.equals(HMS_DB_OWNER_TYPE) && value != null) {
+ database.setOwnerType(PrincipalType.valueOf(value));
} else {
if (value != null) {
parameter.put(key, value);
}
}
});
+
+ if (database.getOwnerName() == null) {
+ database.setOwnerName(System.getProperty("user.name"));
Review Comment:
I'm fine using UGI.currentUser() as the default I just wanted to be on the safe side and make sure we won't break any backward compatibility when we change the default during table creation.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] gaborkaszab commented on pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#issuecomment-1315199732
> @danielcweeks thx for your last round of review. I agree with you that UGI.currentUser is a better default owner, though I plan to make that change in a separate follow up PR. (See my reply above for more details)
>
> Also, I tried to rebase on master branch, but the change to `TableProperties` is still there.
The property is meant to remain in TableProperties I just renamed it in another patch. I expected this patch to move that property out from TableProperties.
About using UGI.currentUser() as the default, If there is consensus we can use this, I'm fine. I just wanted to know if we break any backward compatibility if we change the default from system's "user.name" property to UGI.currentUser() for table creation.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#issuecomment-1316669286
@danielcweeks @gaborkaszab Make sense , for this patch we should go with user.name then on Database side, to be consistent with Table? Then switch both after 1.1? Probably makes most sense from user point of view
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1018262364
##########
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:
cool, taken
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1018262364
##########
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:
cool, taking
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1016947075
##########
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:
I see what you mean, make sense
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1025370462
##########
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: we usually use Preconditions with formatted strings, it makes it fit more easily on one line ie, ("Setting %s and %s", ...)
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -518,11 +541,26 @@ private Map<String, String> convertToMetadata(Database database) {
if (database.getDescription() != null) {
meta.put("comment", database.getDescription());
}
+ if (database.getOwnerName() != null) {
+ meta.put(HMS_DB_OWNER, database.getOwnerName());
+ if (database.getOwnerType() != null) {
+ meta.put(HMS_DB_OWNER_TYPE, database.getOwnerType().name());
+ }
+ }
return meta;
}
Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
+ Preconditions.checkArgument(
+ meta.get(HMS_DB_OWNER_TYPE) == null
Review Comment:
Optional: A bit related to previous comment, I actually think here there may not be such a need for this extra bulky check, and call to a third party library? If I'm not mistaken the valueOf anyway throws IllegalArgumentException if it does not match a value, and it would be pretty easy for user to find out?
not sure if it throws exception for nulls, if not we could do that I suppose.
Just trying to reduce bulky code here if possible.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -380,6 +396,13 @@ public boolean setProperties(Namespace namespace, Map<String, String> properties
@Override
public boolean removeProperties(Namespace namespace, Set<String> properties) {
+ Preconditions.checkArgument(
Review Comment:
same
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1025502497
##########
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:
ack, taken
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1021914426
##########
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:
see set_ownership_noop_4 at line 682 below
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#issuecomment-1314275816
Thx @gaborkaszab . Your last suggestion on having an extra unit test scenario is implemented in my latest commit.
@danielcweeks Based on my conversation with Gabor up till now, I'm expecting this change to at least close to its final state. Since you put a request for change on this PR, I'll probably need you to take another look once you get a chance.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho merged pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
szehon-ho merged PR #6045:
URL: https://github.com/apache/iceberg/pull/6045
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1025379199
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -518,11 +541,26 @@ private Map<String, String> convertToMetadata(Database database) {
if (database.getDescription() != null) {
meta.put("comment", database.getDescription());
}
+ if (database.getOwnerName() != null) {
+ meta.put(HMS_DB_OWNER, database.getOwnerName());
+ if (database.getOwnerType() != null) {
+ meta.put(HMS_DB_OWNER_TYPE, database.getOwnerType().name());
+ }
+ }
return meta;
}
Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
+ Preconditions.checkArgument(
+ meta.get(HMS_DB_OWNER_TYPE) == null
Review Comment:
Optional: I actually think here there may not be such a need for this extra bulky check, and call to a third party library? If I'm not mistaken the valueOf anyway throws IllegalArgumentException if it does not match a value, and it would be pretty easy for user to find out?
not sure if it throws exception for nulls, if not we could do that I suppose.
Just trying to reduce bulky code here if possible.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#issuecomment-1317390253
@gaborkaszab sorry i know we are super close, but do you think we should change both to use UserGroupInformation quickly before cutting final 1.1 RC? That way no need to change behavior.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1013067243
##########
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:
see the "remove_ownership_noop" test case at line 593 above.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] danielcweeks commented on pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#issuecomment-1314292521
> Thx @gaborkaszab . Your last suggestion on having an extra unit test scenario is implemented in my latest commit.
>
> @danielcweeks Based on my conversation with Gabor up till now, I'm expecting this change to at least close to its final state. Since you put a request for change on this PR, I'll probably need you to take another look once you get a chance.
I dismissed the change request because that's been addressed. Minor comments about what user to fallback to. It probably won't be an issue, but you might be able to rebase against master to drop the changes to `TableProperties` since I believe the property was removed in another PR.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#issuecomment-1314415372
@danielcweeks thx for your last round of review. I agree with you that UGI.currentUser is a better default owner, though I plan to make that change in a separate follow up PR. (See my comment above for more details)
Also, I tried to rebase on master branch, but the change to `TableProperties` is still there.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1026820406
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -356,6 +365,11 @@ 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),
+ String.format(
Review Comment:
Ah, I see.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#issuecomment-1329241014
Merged, thanks @haizhou-zhao . Thanks @gaborkaszab and @danielcweeks for extended review.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1018274521
##########
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:
taking. Indeed, and this works with what you later suggested: have separate precondition check at each public API, not at the convertToDatabaseLevel
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1016943632
##########
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:
Got it. I'll make the change
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] gaborkaszab commented on pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#issuecomment-1315204067
> Thx @gaborkaszab . Your last suggestion on having an extra unit test scenario is implemented in my latest commit.
>
> @danielcweeks Based on my conversation with Gabor up till now, I'm expecting this change to at least close to its final state. Since you put a request for change on this PR, I'll probably need you to take another look once you get a chance.
Sorry, somehow I missed that. Thanks!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] danielcweeks commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1021993453
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -426,7 +426,7 @@ private Table newHmsTable(TableMetadata metadata) {
new Table(
tableName,
database,
- metadata.property(TableProperties.HMS_TABLE_OWNER, System.getProperty("user.name")),
+ metadata.property(HiveCatalog.HMS_TABLE_OWNER, System.getProperty("user.name")),
Review Comment:
Possibly consider `UserGroupInformation.getCurrentUser()` here as well.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1020582495
##########
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:
taken. Good idea.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1009975523
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -257,21 +259,42 @@ public void testReplaceTxnBuilder() throws Exception {
public void testCreateTableWithOwner() throws Exception {
Schema schema = getTestSchema();
PartitionSpec spec = PartitionSpec.builderFor(schema).bucket("data", 16).build();
- TableIdentifier tableIdent = TableIdentifier.of(DB_NAME, "tbl");
- String location = temp.newFolder("tbl").toString();
String owner = "some_owner";
- ImmutableMap<String, String> properties =
- ImmutableMap.of(TableProperties.HMS_TABLE_OWNER, owner);
-
+ Map<String, String> prop = ImmutableMap.of(TableProperties.HMS_TABLE_OWNER, owner);
Review Comment:
Curiuos, this is no longer related to the change, is it? (This is for createTable?)
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -539,12 +541,26 @@ 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)) {
+ database.setOwnerType(PrincipalType.valueOf(value));
} else {
if (value != null) {
parameter.put(key, value);
}
}
});
+
+ // default ownership on db level kept uniform with table level, see HiveTableOperations#
Review Comment:
I think the code comment is unnecessary (like, example, why not comment the other method and say its kept uniform with this one..), I would say remove comment entirely.
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -257,21 +259,42 @@ public void testReplaceTxnBuilder() throws Exception {
public void testCreateTableWithOwner() throws Exception {
Schema schema = getTestSchema();
PartitionSpec spec = PartitionSpec.builderFor(schema).bucket("data", 16).build();
- TableIdentifier tableIdent = TableIdentifier.of(DB_NAME, "tbl");
- String location = temp.newFolder("tbl").toString();
String owner = "some_owner";
- ImmutableMap<String, String> properties =
- ImmutableMap.of(TableProperties.HMS_TABLE_OWNER, owner);
-
+ Map<String, String> prop = ImmutableMap.of(TableProperties.HMS_TABLE_OWNER, owner);
+
+ createTableAndVerifyOwnership(DB_NAME, "tbl", schema, spec, prop, owner);
+
+ prop = ImmutableMap.of();
+
+ // a table without an owner explicitly specified should have a default owner
+ createTableAndVerifyOwnership(
+ DB_NAME,
+ "no_explicit_owner_specified_tbl",
+ schema,
+ spec,
+ prop,
Review Comment:
NIt, can we just pass ImmutableMap.of() here?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1010534560
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -257,21 +259,42 @@ public void testReplaceTxnBuilder() throws Exception {
public void testCreateTableWithOwner() throws Exception {
Schema schema = getTestSchema();
PartitionSpec spec = PartitionSpec.builderFor(schema).bucket("data", 16).build();
- TableIdentifier tableIdent = TableIdentifier.of(DB_NAME, "tbl");
- String location = temp.newFolder("tbl").toString();
String owner = "some_owner";
- ImmutableMap<String, String> properties =
- ImmutableMap.of(TableProperties.HMS_TABLE_OWNER, owner);
-
+ Map<String, String> prop = ImmutableMap.of(TableProperties.HMS_TABLE_OWNER, owner);
Review Comment:
You are right. This is just some changes I forget to remove.
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -257,21 +259,42 @@ public void testReplaceTxnBuilder() throws Exception {
public void testCreateTableWithOwner() throws Exception {
Schema schema = getTestSchema();
PartitionSpec spec = PartitionSpec.builderFor(schema).bucket("data", 16).build();
- TableIdentifier tableIdent = TableIdentifier.of(DB_NAME, "tbl");
- String location = temp.newFolder("tbl").toString();
String owner = "some_owner";
- ImmutableMap<String, String> properties =
- ImmutableMap.of(TableProperties.HMS_TABLE_OWNER, owner);
-
+ Map<String, String> prop = ImmutableMap.of(TableProperties.HMS_TABLE_OWNER, owner);
Review Comment:
You are right. This is just some change I forget to remove.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1010582748
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -270,9 +275,34 @@ public void testCreateTableWithOwner() throws Exception {
Assert.assertEquals(owner, hmsTable.getOwner());
Map<String, String> hmsTableParams = hmsTable.getParameters();
Assert.assertFalse(hmsTableParams.containsKey(TableProperties.HMS_TABLE_OWNER));
+ Assert.assertEquals(
+ PrincipalType.USER.name(), hmsTableParams.get(TableProperties.HMS_TABLE_OWNER_TYPE));
} finally {
catalog.dropTable(tableIdent);
}
+
+ TableIdentifier tableIdent2 = TableIdentifier.of(DB_NAME, "tbl_group_owned");
+ String location2 = temp.newFolder("tbl_group_owned").toString();
+ String owner2 = "some_group_owner";
+ ImmutableMap<String, String> properties2 =
+ ImmutableMap.of(
+ TableProperties.HMS_TABLE_OWNER,
+ owner2,
+ TableProperties.HMS_TABLE_OWNER_TYPE,
+ PrincipalType.GROUP.name());
+
+ try {
+ Table table2 = catalog.createTable(tableIdent2, schema, spec, location2, properties2);
+ org.apache.hadoop.hive.metastore.api.Table hmsTable =
+ metastoreClient.getTable(DB_NAME, "tbl_group_owned");
+ Assert.assertEquals(owner2, hmsTable.getOwner());
+ Map<String, String> hmsTableParams = hmsTable.getParameters();
+ Assert.assertFalse(hmsTableParams.containsKey(TableProperties.HMS_TABLE_OWNER));
+ Assert.assertEquals(
+ PrincipalType.GROUP.name(), hmsTableParams.get(TableProperties.HMS_TABLE_OWNER_TYPE));
+ } finally {
+ catalog.dropTable(tableIdent2);
+ }
Review Comment:
Since table group ownership support is no longer relevant to this PR. This change has also been reverted.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1015549849
##########
core/src/main/java/org/apache/iceberg/TableProperties.java:
##########
@@ -360,5 +360,7 @@ private TableProperties() {}
public static final String UPSERT_ENABLED = "write.upsert.enabled";
public static final boolean UPSERT_ENABLED_DEFAULT = false;
- public static final String HMS_TABLE_OWNER = "hms_table_owner";
+ public static final String HMS_TABLE_OWNER = "hive.metastore.table.owner";
Review Comment:
thanks @danielcweeks , I'll put them in HiveCatalog class.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1016941186
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -518,11 +522,36 @@ private Map<String, String> convertToMetadata(Database database) {
if (database.getDescription() != null) {
meta.put("comment", database.getDescription());
}
+ if (database.getOwnerName() != null) {
+ meta.put(TableProperties.HMS_DB_OWNER, database.getOwnerName());
+ if (database.getOwnerType() != null) {
+ meta.put(TableProperties.HMS_DB_OWNER_TYPE, database.getOwnerType().name());
+ }
+ }
return meta;
}
Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
+ Preconditions.checkArgument(
Review Comment:
I think Precondition are meant to check whether user/caller invoked the method with the correct input/parameters.
Understandably, different sources will hold different opinions, but these are the sources I found when I tried to google around:
Quoting these two java tutorial sites https://www.geeksforgeeks.org/preconditions-guava-java/ & https://www.baeldung.com/guava-preconditions
"The Preconditions Class provides a list of static methods for checking that a method or a constructor is invoked with valid parameter values"
Quoting java doc for guava at https://guava.dev/releases/19.0/api/docs/com/google/common/base/Preconditions.html
"Non-preconditions
It is of course possible to use the methods of this class to check for invalid conditions which are not the caller's fault. Doing so is not recommended because it is misleading to future readers of the code and of stack traces. "
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] gaborkaszab commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1015406783
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -518,11 +522,36 @@ private Map<String, String> convertToMetadata(Database database) {
if (database.getDescription() != null) {
meta.put("comment", database.getDescription());
}
+ if (database.getOwnerName() != null) {
+ meta.put(TableProperties.HMS_DB_OWNER, database.getOwnerName());
+ if (database.getOwnerType() != null) {
+ meta.put(TableProperties.HMS_DB_OWNER_TYPE, database.getOwnerType().name());
+ }
+ }
return meta;
}
Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
+ Preconditions.checkArgument(
Review Comment:
In my understanding Preconditions are for checking some internal state that should be always true. Here, if I'm not mistaken you use it to verify user input. This seems a bit odd for me.
##########
core/src/main/java/org/apache/iceberg/TableProperties.java:
##########
@@ -360,5 +360,7 @@ private TableProperties() {}
public static final String UPSERT_ENABLED = "write.upsert.enabled";
public static final boolean UPSERT_ENABLED_DEFAULT = false;
- public static final String HMS_TABLE_OWNER = "hms_table_owner";
+ public static final String HMS_TABLE_OWNER = "hive.metastore.table.owner";
Review Comment:
What I'm not sure about is if we release 1.1.0 now with HMS_TABLE_OWNER = "hms_table_owner" but then we change this property to "hive.metastore.table.owner" then when this patch gets released (most probably in 1.2.0) wouldn't it be a breaking change?
@danielcweeks could you please share your view on this?
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -518,11 +522,36 @@ private Map<String, String> convertToMetadata(Database database) {
if (database.getDescription() != null) {
meta.put("comment", database.getDescription());
}
+ if (database.getOwnerName() != null) {
+ meta.put(TableProperties.HMS_DB_OWNER, database.getOwnerName());
+ if (database.getOwnerType() != null) {
+ meta.put(TableProperties.HMS_DB_OWNER_TYPE, database.getOwnerType().name());
+ }
+ }
return meta;
}
Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
+ Preconditions.checkArgument(
+ meta.get(TableProperties.HMS_DB_OWNER_TYPE) == null
+ || meta.get(TableProperties.HMS_DB_OWNER) != null,
+ "Setting "
+ + TableProperties.HMS_DB_OWNER_TYPE
+ + " without setting "
+ + TableProperties.HMS_DB_OWNER
+ + "is not allowed");
+
+ Preconditions.checkArgument(
Review Comment:
Same here
##########
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:
See my comment above about preconditions.
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -448,6 +575,59 @@ public void testRemoveNamespaceProperties() throws TException {
});
}
+ @Test
+ public void testRemoveNamespaceOwnership() throws TException {
+ Map<String, String> prop = ImmutableMap.of(HiveCatalog.HMS_DB_OWNER, "some_owner");
+ removeNamespaceOwnershipAndVerify("remove_individual_ownership", prop);
+
+ prop =
+ ImmutableMap.of(
+ HiveCatalog.HMS_DB_OWNER,
+ "some_owner",
+ HiveCatalog.HMS_DB_OWNER_TYPE,
+ PrincipalType.GROUP.name());
+ removeNamespaceOwnershipAndVerify("remove_group_ownership", prop);
+
+ prop = ImmutableMap.of();
+
+ removeNamespaceOwnershipAndVerify("remove_ownership_noop", prop);
Review Comment:
This tests that what happens when there is no specific owner/ownertype set for the namespace but then we try to remove them, right?
What would also be beneficial in my opinion is to create the namespace with some owner and then run a noop remove and check if the owner is still the same what we provided during creation.
##########
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:
I think you got me wrong. I just wanted to say that when you call catalog.createNamespace() at L619 then you immediately call catalog.removeNamespace(). To be on the safe side I'd add an extra check between these two steps that right after creating the namespace the owner and owner type are as expected.
##########
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:
No, what I meant is to instead of this:
`String expectedOwner = "apache";
PrincipalType expectedOwnerType = PrincipalType.USER;
createNamespaceAndVerifyOwnership("userOwnership", prop, expectedOwner, expectedOwnerType);`
we could have this:
`createNamespaceAndVerifyOwnership(
"userOwnership",
prop,
"apache",
PrincipalType.USER);`
Might be personal preference. Let me know if you feel that this wouldn't improve readability.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1018274521
##########
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:
Taken. Indeed, and this works with what you later suggested: have separate precondition check at each public API, not at the convertToDatabaseLevel
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1016941614
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -518,11 +522,36 @@ private Map<String, String> convertToMetadata(Database database) {
if (database.getDescription() != null) {
meta.put("comment", database.getDescription());
}
+ if (database.getOwnerName() != null) {
+ meta.put(TableProperties.HMS_DB_OWNER, database.getOwnerName());
+ if (database.getOwnerType() != null) {
+ meta.put(TableProperties.HMS_DB_OWNER_TYPE, database.getOwnerType().name());
+ }
+ }
return meta;
}
Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
+ Preconditions.checkArgument(
+ meta.get(TableProperties.HMS_DB_OWNER_TYPE) == null
+ || meta.get(TableProperties.HMS_DB_OWNER) != null,
+ "Setting "
+ + TableProperties.HMS_DB_OWNER_TYPE
+ + " without setting "
+ + TableProperties.HMS_DB_OWNER
+ + "is not allowed");
+
+ Preconditions.checkArgument(
Review Comment:
Replied above.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] danielcweeks commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1017130690
##########
core/src/main/java/org/apache/iceberg/TableProperties.java:
##########
@@ -360,5 +360,7 @@ private TableProperties() {}
public static final String UPSERT_ENABLED = "write.upsert.enabled";
public static final boolean UPSERT_ENABLED_DEFAULT = false;
- public static final String HMS_TABLE_OWNER = "hms_table_owner";
+ public static final String HMS_TABLE_OWNER = "hive.metastore.table.owner";
Review Comment:
If this goes in before cutting 1.1, we're fine. If it goes in after, then we just need to mark that deprecated for removal in 1.2.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] gaborkaszab commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1017654923
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -518,11 +522,36 @@ private Map<String, String> convertToMetadata(Database database) {
if (database.getDescription() != null) {
meta.put("comment", database.getDescription());
}
+ if (database.getOwnerName() != null) {
+ meta.put(TableProperties.HMS_DB_OWNER, database.getOwnerName());
+ if (database.getOwnerType() != null) {
+ meta.put(TableProperties.HMS_DB_OWNER_TYPE, database.getOwnerType().name());
+ }
+ }
return meta;
}
Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
+ Preconditions.checkArgument(
Review Comment:
Thanks for the explanation, @haizhou-zhao! We definitely use Preconditions in Impala for other purposes as we instead guarantee that the internal invariant is not hurt and we don't validate user input with them. But we might use it against the original purpose then :)
Thanks again for the explanation!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for group ownership
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1005077283
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -437,6 +451,18 @@ private Table newHmsTable(TableMetadata metadata) {
null,
TableType.EXTERNAL_TABLE.toString());
+ // hive2 does not support setting table ownership type, therefore we leave it as
+ // a table property. Once upgraded to hive3, then we can simply set
+ // newTable.setOwnerType(PrincipalType.valueOf(metadata.get(HMS_TABLE_OWNER_TYPE)))
+ String ownerType = metadata.property(TableProperties.HMS_TABLE_OWNER_TYPE, null);
+ if (ownerType != null && ownerType.equalsIgnoreCase(PrincipalType.GROUP.name())) {
+ newTable
+ .getParameters()
+ .put(TableProperties.HMS_TABLE_OWNER_TYPE, PrincipalType.GROUP.name());
Review Comment:
nvm, ignore my previous comment. I think I mis-understood what you are saying. Yes, just directly put `ownerType` works. I probably shouldn't use `equalsIgnoreCase` in L458 in the first place. I think we should make clear to users, if they want to tweak owner type, their key should be all-cap `"GROUP"` in the property.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -437,6 +451,18 @@ private Table newHmsTable(TableMetadata metadata) {
null,
TableType.EXTERNAL_TABLE.toString());
+ // hive2 does not support setting table ownership type, therefore we leave it as
+ // a table property. Once upgraded to hive3, then we can simply set
+ // newTable.setOwnerType(PrincipalType.valueOf(metadata.get(HMS_TABLE_OWNER_TYPE)))
+ String ownerType = metadata.property(TableProperties.HMS_TABLE_OWNER_TYPE, null);
+ if (ownerType != null && ownerType.equalsIgnoreCase(PrincipalType.GROUP.name())) {
+ newTable
+ .getParameters()
+ .put(TableProperties.HMS_TABLE_OWNER_TYPE, PrincipalType.GROUP.name());
+ } else { // default: individual ownership
Review Comment:
Good idea.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for group ownership
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1005072035
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -437,6 +451,18 @@ private Table newHmsTable(TableMetadata metadata) {
null,
TableType.EXTERNAL_TABLE.toString());
+ // hive2 does not support setting table ownership type, therefore we leave it as
Review Comment:
It needs extra implementation to get the ownership type from the property.
Hopefully, let's not upgrade Iceberg's hive dependency version just because of this. I actually tried, and found that is no small task.
This is some temporary solution to compensate for the fact that hive2 does not support table ownership type. I'm open to other suggestions.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for group ownership
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1005077466
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -261,7 +262,11 @@ public void testCreateTableWithOwner() throws Exception {
String location = temp.newFolder("tbl").toString();
String owner = "some_owner";
ImmutableMap<String, String> properties =
- ImmutableMap.of(TableProperties.HMS_TABLE_OWNER, owner);
+ ImmutableMap.of(
+ TableProperties.HMS_TABLE_OWNER,
+ owner,
+ TableProperties.HMS_TABLE_OWNER_TYPE,
Review Comment:
Yes, good idea.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1010574348
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -532,13 +536,27 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
database.setName(namespace.level(0));
database.setLocationUri(databaseLocation(namespace.level(0)));
+ // default ownership is determined from Hadoop.UGI.currentUser
+ // and the owner type is default to individual user
+ try {
+ // getShortUserName will remove the realm part of the whole username
+ database.setOwnerName(UserGroupInformation.getCurrentUser().getShortUserName());
+ database.setOwnerType(PrincipalType.USER);
+ } catch (IOException e) {
+ LOG.error("Unable to determine the current UGI user", e);
+ throw new RuntimeException(e);
Review Comment:
Splitting UGI changes into another patch later. So this is no longer relevant for this PR.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1010590535
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -422,11 +425,22 @@ private Table newHmsTable(TableMetadata metadata) {
Preconditions.checkNotNull(metadata, "'metadata' parameter can't be null");
final long currentTimeMillis = System.currentTimeMillis();
+ String defaultUser;
+ // default ownership is determined from Hadoop.UGI.currentUser
+ // and the owner type is default to individual user
+ try {
+ // getShortUserName will remove the realm part of the whole username
+ defaultUser = UserGroupInformation.getCurrentUser().getShortUserName();
Review Comment:
Since adding group ownership for table is removed from this PR, we no longer need this.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1013067243
##########
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:
see the "remove_ownership_noop" test case at line 595 above.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1025582073
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -518,11 +541,26 @@ private Map<String, String> convertToMetadata(Database database) {
if (database.getDescription() != null) {
meta.put("comment", database.getDescription());
}
+ if (database.getOwnerName() != null) {
+ meta.put(HMS_DB_OWNER, database.getOwnerName());
+ if (database.getOwnerType() != null) {
+ meta.put(HMS_DB_OWNER_TYPE, database.getOwnerType().name());
+ }
+ }
return meta;
}
Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
+ Preconditions.checkArgument(
+ meta.get(HMS_DB_OWNER_TYPE) == null
Review Comment:
Yes, Enum.Valueof will through IllegalArgumentException if string is invalid or null. I originally was following the principal of "if something has to fail then fail early". In this case, I would take your recommendation because this function is too small and it's not much "earlier" + yes, this error could be figured out by users
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] danielcweeks commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1021993031
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -539,12 +577,22 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
database.setDescription(value);
} else if (key.equals("location")) {
database.setLocationUri(value);
+ } else if (key.equals(HMS_DB_OWNER)) {
+ database.setOwnerName(value);
+ } else if (key.equals(HMS_DB_OWNER_TYPE) && value != null) {
+ database.setOwnerType(PrincipalType.valueOf(value));
} else {
if (value != null) {
parameter.put(key, value);
}
}
});
+
+ if (database.getOwnerName() == null) {
+ database.setOwnerName(System.getProperty("user.name"));
Review Comment:
You might want to consider falling back to `UserGroupInformation.getCurrentUser()` as opposed to just the java user. I believe this is used in many placed by Spark and Hive to identify the current user and may be different from the process owner.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1022032530
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -539,12 +577,22 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
database.setDescription(value);
} else if (key.equals("location")) {
database.setLocationUri(value);
+ } else if (key.equals(HMS_DB_OWNER)) {
+ database.setOwnerName(value);
+ } else if (key.equals(HMS_DB_OWNER_TYPE) && value != null) {
+ database.setOwnerType(PrincipalType.valueOf(value));
} else {
if (value != null) {
parameter.put(key, value);
}
}
});
+
+ if (database.getOwnerName() == null) {
+ database.setOwnerName(System.getProperty("user.name"));
Review Comment:
Hi, its probably my fault here, @haizhou-zhao had this initially but it seemed there was some unresolved concern at the time : https://github.com/apache/iceberg/pull/6045#discussion_r1004221149 , so I actually asked @haizhou-zhao to follow what the table implementation uses for consistency , and we could change both to UserGroupInformation.getCurrentUser() later once there's consensus.
But looks like its cleared up since then, @gaborkaszab ? If so we can go ahead and use UserGroupInformation.getCurrentUser() here and then change table to be consistent as well? I also agree that it's more optimal.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -539,12 +577,22 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
database.setDescription(value);
} else if (key.equals("location")) {
database.setLocationUri(value);
+ } else if (key.equals(HMS_DB_OWNER)) {
+ database.setOwnerName(value);
+ } else if (key.equals(HMS_DB_OWNER_TYPE) && value != null) {
+ database.setOwnerType(PrincipalType.valueOf(value));
} else {
if (value != null) {
parameter.put(key, value);
}
}
});
+
+ if (database.getOwnerName() == null) {
+ database.setOwnerName(System.getProperty("user.name"));
Review Comment:
Hi, its probably my fault here, @haizhou-zhao had this initially but it seemed there was some unresolved concern at the time : https://github.com/apache/iceberg/pull/6045#discussion_r1004221149 , so I actually asked @haizhou-zhao to follow what the table implementation uses for consistency , and we could change both to UserGroupInformation.getCurrentUser() later once there's consensus.
But looks like its cleared up since then, @gaborkaszab ? If so we can go ahead and use UserGroupInformation.getCurrentUser() here and then change table to be consistent as well? I also agree that it's more complete.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1020581546
##########
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:
See my other comments for why I think preconditions for createNamespace and setProperty are different.
##########
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:
If your table originally have group ownership, and imagine you set property only on owner but not explicitly on owner type for this particular table, then it will be hard to define what exactly the user wants in this case. Does the user want to switch ownership to a different group, or does the user want to switch to individual ownership under a different person's name? I believe it's better not guess user intention in this case and force the users to be explicit about exactly what ownership they want to switch to both in terms of owner and owner type.
In summary, I think createNamespace and setProperty are different because we can assume that when user calls createNamespace, then the namespace never existed before, nor is there a previous ownership (and it's easier to say, under this circumstances that "if no owner type specificed, then take owner-type=USER as default"). With setProperty, we do have to consider what state of ownership is there before the change.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1022077957
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -539,12 +577,22 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
database.setDescription(value);
} else if (key.equals("location")) {
database.setLocationUri(value);
+ } else if (key.equals(HMS_DB_OWNER)) {
+ database.setOwnerName(value);
+ } else if (key.equals(HMS_DB_OWNER_TYPE) && value != null) {
+ database.setOwnerType(PrincipalType.valueOf(value));
} else {
if (value != null) {
parameter.put(key, value);
}
}
});
+
+ if (database.getOwnerName() == null) {
+ database.setOwnerName(System.getProperty("user.name"));
Review Comment:
@danielcweeks I actually had conversation with both @gaborkaszab and @szehon-ho on this topic. We want to put "adding ownership support for namespaces" and "use UGI.getCurrentUser as default value" into two separate PRs. My very first version of this PR was actually a combination of both, which people feel is just too much content into a single PR for reviewing purpose. Context is: the current iceberg-hive-metastore code is using Java/OS username as default value.
Yes, I will follow up this PR with another one to switch to UGI.currentUser (yes, I agree with you that it would be a more correct default value in the Hive world comparing to Java/OS username). If you have a strong opinion on these two changes should be in one single PR, then, for sure, please let us know. Thx!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1022077957
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -539,12 +577,22 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
database.setDescription(value);
} else if (key.equals("location")) {
database.setLocationUri(value);
+ } else if (key.equals(HMS_DB_OWNER)) {
+ database.setOwnerName(value);
+ } else if (key.equals(HMS_DB_OWNER_TYPE) && value != null) {
+ database.setOwnerType(PrincipalType.valueOf(value));
} else {
if (value != null) {
parameter.put(key, value);
}
}
});
+
+ if (database.getOwnerName() == null) {
+ database.setOwnerName(System.getProperty("user.name"));
Review Comment:
@danielcweeks I actually had conversation with both @gaborkaszab and @szehon-ho on this topic. We want to put "adding ownership support for namespaces" and "use UGI.getCurrentUser as default value" into two separate PRs. My very first version of this PR was actually a combination of both, which people felt was just too much content into a single PR for reviewing purpose. Context is: the current iceberg-hive-metastore code is using Java/OS username as default value.
Yes, I will follow up this PR with another one to switch to UGI.currentUser (yes, I agree with you that it would be a more correct default value in the Hive world comparing to Java/OS username). If you have a strong opinion on these two changes should be in one single PR, then, for sure, please let us know. Thx!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for group ownership
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1005069022
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -422,11 +425,22 @@ private Table newHmsTable(TableMetadata metadata) {
Preconditions.checkNotNull(metadata, "'metadata' parameter can't be null");
final long currentTimeMillis = System.currentTimeMillis();
+ String defaultUser;
+ // default ownership is determined from Hadoop.UGI.currentUser
+ // and the owner type is default to individual user
+ try {
+ // getShortUserName will remove the realm part of the whole username
+ defaultUser = UserGroupInformation.getCurrentUser().getShortUserName();
Review Comment:
I see there're some similar comments on UGI, so I'll just put my reply-to-all here.
To the best of my understanding, most production Hive Metastore is protected with AuthN mechanism. For hive2&3, Kerberos is the only choice; for hive4, token exchange is supported. I'm not familiar with the new AuthN in hive4, and it is not used by Iceberg, so I'm focusing on Kerberos in this case, specifically Hive Metastore server with Kerberos enabled.
Context/Assumption
1. `System.getProperty("user.name")` is OS username. OS security and Kerberos realm (KDC) recognizes different set of users/principals, so let's assume your OS username is not going to be recognizable by Kerberos KDC.
2. If you are connecting to Hive Metastore in unsecured mod, then you do not need to do kerberos init as in `UGI.loginUser()`. In this case, `UGI.getCurrentUser()` will default to OS username (aka. `System.getProperty("user.name")`) or whatever your environment variable `HADOOP_USER_NAME` is set to. For now, let's talk about production Hive Metastore cluster with Kerberos enabled and set aside the unsecured mode is out of scope. The unsecured mode is still important, I'll come back to it later.
3. This is my understanding of the life cycle of IcebergHiveCatalog:
a. user code: `UGI.loginUser()` (this is like kinit, that initialize your initial ticket granting ticket with Kerberos KDC)
b. user code: call `CatalogUtil.loadCatalog(HiveCatalog.class, 'myCatalog', prop, hadooConf)` (this creates a new IcebergHiveCatalog object)
c. under the hood, Iceberg code will init Hive Metastore Client during init of IcebergHiveCatalog object. Since we are talking about connecting to a kerberized Hive Metastore server, this will fail if user do not call `UGI.loginUser()` preemptively
d. users call things like `createNamespace` or `createTable` with the initialized IcebergHiveMetastore
Now, the original way of registering OS username as the default owner of iceberg table with Hive won't cause problem even when connecting to a secured Hive Metastore server, because for Hive, you can set your owner to any string.
**The real problem with using OS username comes with AuthZ**. And here, since the only option of AuthN with hive2&hive3 is kerberos, let's assume the AuthZ policies on Hive Metastore ties privileges to Kerberos User Principals (aka. identities that is only recognizable to Kerberos KDC, not local OS security).
I would guess the intention of the original author of this code is: "if the user of the Iceberg API does not want to set any ownership informations on create, then by default the user running this request should be the owner of the resource created. And later this user has get admin access to this particular db/table". However, treating the OS user of the current process as owner is not well hooked with Kerberos Realm and subsequently, the AuthZ/Privilege system that depends on Kerberos Principals. In a sense, the practice of setting OS user as the owner is similar to setting the owner of the resource to any string that is not recognizable as an identity by the Kerberos Realm.
**I saw there's comment regarding the performance** of Hadoop `UGI.getCurrentUser`. My understanding: `UGI.getCurrentUser` conceptually should not be more time consuming than `System.getProperty("user.name")`, because before you call `UGI.getCurrentUser`, you've already done with kerberos init (getting your initial Ticket Granting Ticket), or otherwise you cannot even connect to Hive Metastore. It is when you invoke `UGI.loginUser`, there's nasty business of getting TGT and doing things like sha256 decryption and encryption under the hood. Since kerberos cache only needs to be updated every time your cache expires (likely your TGT has 24 hours of validity or more), overall, it's not taking too much toll on your system.
(under the hood, HADOOP UGI still depends on some javax.* or sun.* packages for Kerberos related things, so the exact encryption/decryption implementation may differ based on your JDK version)
**Now back to the case you are just connecting to an unsecured Hive Metastore** in your PoC or testing environment. In that case, you would not call `UGI.loginUser` beforehand, then when calling `UGI.getCurrentUser`, it's essentially equivalent of calling `System.getProperty("user.name")`. Effectively, I understand `UGI.getCurrentUser` as doing this for me:
```
if (connecting to a secured Hive Metastore) {
get the current kerberos cache, find the principal of the user currently logging in
}
else { // not connecting to a secured Hive Metastore, not messing around with Kerberos
get the OS user that is running this process, or fetch environment variable 'HADOOP_USER_NAME'
}
```
(Again, this implementation that Hadoop depends on is deep down inside javax.security.* packages, may differ as you use different JDKs. But I think that's a good thing because your get free updates from JDK regarding all these security things)
In that regard, I think `UGI.getCurrentUser` is more robust and comprehensive than `System.getProperty("user.name")`.
**Finally, how reliable is `UGI.getCurrentUser`? When does it through `IOException`s?** I can only say I personally have not encountered issue with `UGI.getCurrentUser`, and the Hadoop's UGI utility is actually widely used. However, when I tried to search around, bugs like this do turn up against UGI utilities: https://issues.apache.org/jira/browse/HADOOP-13805. Though I still want to say, if you are not dealing with a secured Hive Metastore or Kerberos (which is likely the status quo of the current Iceberg code), and only seeking to get the current OS user, I see very low chance that this can fail.
To summarize, I believe it's possible to not change the underlying Iceberg API code, and ask user of Iceberg API to handle authN and authZ by explicitly setting the table attributes related to ownership. However, I think we should maintain the principle that "if the user of the Iceberg API does not want to set any ownership informations on create, then by default the current user running this request should be the owner of the resource created". And it would be a good user experience if we can figure out the correct "current user running this request".
Now I'm not gonna pretend I'm an expert of Hadoop or Kerberos, the above are to the best of my knowledge, maybe not entirely correct. So definitely welcome HADOOP/Kerberos expert from the community to correct any of my words above, and that also includes you (@gaborkaszab) as well! Let me know what you think.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for group ownership
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1005073773
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -437,6 +451,18 @@ private Table newHmsTable(TableMetadata metadata) {
null,
TableType.EXTERNAL_TABLE.toString());
+ // hive2 does not support setting table ownership type, therefore we leave it as
+ // a table property. Once upgraded to hive3, then we can simply set
+ // newTable.setOwnerType(PrincipalType.valueOf(metadata.get(HMS_TABLE_OWNER_TYPE)))
+ String ownerType = metadata.property(TableProperties.HMS_TABLE_OWNER_TYPE, null);
+ if (ownerType != null && ownerType.equalsIgnoreCase(PrincipalType.GROUP.name())) {
+ newTable
+ .getParameters()
+ .put(TableProperties.HMS_TABLE_OWNER_TYPE, PrincipalType.GROUP.name());
Review Comment:
sure, I was originally think about if I simply put a string like `"GROUP"` down, the later readers will wonder why it's not `"Group"`, `"group"` or `"groups"`. This would demonstrate where the string value comes from. But yes, I'm good with just a plain string here.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1010533184
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -539,12 +541,26 @@ 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)) {
+ database.setOwnerType(PrincipalType.valueOf(value));
} else {
if (value != null) {
parameter.put(key, value);
}
}
});
+
+ // default ownership on db level kept uniform with table level, see HiveTableOperations#
Review Comment:
ack
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for group ownership
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1005081704
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -270,9 +275,34 @@ public void testCreateTableWithOwner() throws Exception {
Assert.assertEquals(owner, hmsTable.getOwner());
Map<String, String> hmsTableParams = hmsTable.getParameters();
Assert.assertFalse(hmsTableParams.containsKey(TableProperties.HMS_TABLE_OWNER));
+ Assert.assertEquals(
+ PrincipalType.USER.name(), hmsTableParams.get(TableProperties.HMS_TABLE_OWNER_TYPE));
} finally {
catalog.dropTable(tableIdent);
}
+
+ TableIdentifier tableIdent2 = TableIdentifier.of(DB_NAME, "tbl_group_owned");
+ String location2 = temp.newFolder("tbl_group_owned").toString();
+ String owner2 = "some_group_owner";
+ ImmutableMap<String, String> properties2 =
+ ImmutableMap.of(
+ TableProperties.HMS_TABLE_OWNER,
+ owner2,
+ TableProperties.HMS_TABLE_OWNER_TYPE,
+ PrincipalType.GROUP.name());
+
+ try {
+ Table table2 = catalog.createTable(tableIdent2, schema, spec, location2, properties2);
+ org.apache.hadoop.hive.metastore.api.Table hmsTable =
+ metastoreClient.getTable(DB_NAME, "tbl_group_owned");
+ Assert.assertEquals(owner2, hmsTable.getOwner());
+ Map<String, String> hmsTableParams = hmsTable.getParameters();
+ Assert.assertFalse(hmsTableParams.containsKey(TableProperties.HMS_TABLE_OWNER));
+ Assert.assertEquals(
+ PrincipalType.GROUP.name(), hmsTableParams.get(TableProperties.HMS_TABLE_OWNER_TYPE));
+ } finally {
+ catalog.dropTable(tableIdent2);
+ }
Review Comment:
Yes, it definitely would. I should add a test case with no prop specified. Though we should probably figure out what is the correct behavior in previous comments/conversations. Such as, if we can't figure out the owner, should be just set null to both owner and ownerTypes.
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -358,6 +388,36 @@ public void testCreateNamespace() throws Exception {
"There no same location for db and namespace", database2.getLocationUri(), hiveLocalDir);
}
+ @Test
+ public void testCreateNamespaceWithOwnership() throws Exception {
+ Namespace namespace1 = Namespace.of("userOwnership");
+ Map<String, String> prop1 =
+ ImmutableMap.of(
+ TableProperties.HMS_DB_OWNER,
+ "apache",
+ TableProperties.HMS_DB_OWNER_TYPE,
+ PrincipalType.USER.name());
+ catalog.createNamespace(namespace1, prop1);
+ Database database1 = metastoreClient.getDatabase(namespace1.toString());
+
+ Assert.assertEquals("apache", database1.getOwnerName());
+ Assert.assertEquals(PrincipalType.USER, database1.getOwnerType());
+
+ Map<String, String> prop2 =
+ ImmutableMap.of(
+ TableProperties.HMS_DB_OWNER,
+ "iceberg",
+ TableProperties.HMS_DB_OWNER_TYPE,
+ PrincipalType.GROUP.name());
+ Namespace namespace2 = Namespace.of("groupOwnership");
+
+ catalog.createNamespace(namespace2, prop2);
+ Database database2 = metastoreClient.getDatabase(namespace2.toString());
+
+ Assert.assertEquals("iceberg", database2.getOwnerName());
+ Assert.assertEquals(PrincipalType.GROUP, database2.getOwnerType());
+ }
Review Comment:
Yes, good idea.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] gaborkaszab commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1011646405
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -422,11 +425,22 @@ private Table newHmsTable(TableMetadata metadata) {
Preconditions.checkNotNull(metadata, "'metadata' parameter can't be null");
final long currentTimeMillis = System.currentTimeMillis();
+ String defaultUser;
+ // default ownership is determined from Hadoop.UGI.currentUser
+ // and the owner type is default to individual user
+ try {
+ // getShortUserName will remove the realm part of the whole username
+ defaultUser = UserGroupInformation.getCurrentUser().getShortUserName();
Review Comment:
Thanks for the long explanation! It makes sense for me.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] gaborkaszab commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] gaborkaszab commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] gaborkaszab commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#issuecomment-1312222748
@gaborkaszab Thanks for the last round of review. I have some different opinions on whether the preconditions for createNamespace and setProperty are the same or different. Feel free to take a look at my response when you get a chance.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1013014239
##########
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:
Let's allow the test to execute on databases without ownership.
In case customer removes ownership on databases which does not have any ownership set (ownership equals default value), then it's a noop (ownership stays with default).
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1010577743
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -422,11 +425,22 @@ private Table newHmsTable(TableMetadata metadata) {
Preconditions.checkNotNull(metadata, "'metadata' parameter can't be null");
final long currentTimeMillis = System.currentTimeMillis();
+ String defaultUser;
+ // default ownership is determined from Hadoop.UGI.currentUser
+ // and the owner type is default to individual user
+ try {
+ // getShortUserName will remove the realm part of the whole username
+ defaultUser = UserGroupInformation.getCurrentUser().getShortUserName();
Review Comment:
Splitting UGI changes into another patch later. So this is no longer relevant for this PR.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1025379199
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -518,11 +541,26 @@ private Map<String, String> convertToMetadata(Database database) {
if (database.getDescription() != null) {
meta.put("comment", database.getDescription());
}
+ if (database.getOwnerName() != null) {
+ meta.put(HMS_DB_OWNER, database.getOwnerName());
+ if (database.getOwnerType() != null) {
+ meta.put(HMS_DB_OWNER_TYPE, database.getOwnerType().name());
+ }
+ }
return meta;
}
Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
+ Preconditions.checkArgument(
+ meta.get(HMS_DB_OWNER_TYPE) == null
Review Comment:
Optional: I actually think here there may not be such a need for this extra bulky check, and call to a third party library? If I'm not mistaken the valueOf anyway throws IllegalArgumentException if it does not match a value, which would suffice for us? The only thing we dont get is maybe the valid values, but I think the user can find that out kind of easily?
not sure if it throws exception for nulls, if not we could have a Precondition for that only?
Just trying to reduce bulky code here if possible, and also avoid call to third party library if there's no existing precedent and overwhelming need, as that can make future upgrades kind of hard.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1025379199
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -518,11 +541,26 @@ private Map<String, String> convertToMetadata(Database database) {
if (database.getDescription() != null) {
meta.put("comment", database.getDescription());
}
+ if (database.getOwnerName() != null) {
+ meta.put(HMS_DB_OWNER, database.getOwnerName());
+ if (database.getOwnerType() != null) {
+ meta.put(HMS_DB_OWNER_TYPE, database.getOwnerType().name());
+ }
+ }
return meta;
}
Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
+ Preconditions.checkArgument(
+ meta.get(HMS_DB_OWNER_TYPE) == null
Review Comment:
Optional: I actually think here there may not be such a need for this extra bulky check, and call to a third party library? If I'm not mistaken the valueOf anyway throws IllegalArgumentException if it does not match a value, which would suffice for us? The only thing we dont get is maybe the valid values, but I think the user can find that out kind of easily?
not sure if it throws exception for nulls, if not we could do that I suppose.
Just trying to reduce bulky code here if possible, and also avoid call to third party library if there's no existing precedent and overwhelming need, as that can make future upgrades kind of hard.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1022032530
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -539,12 +577,22 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
database.setDescription(value);
} else if (key.equals("location")) {
database.setLocationUri(value);
+ } else if (key.equals(HMS_DB_OWNER)) {
+ database.setOwnerName(value);
+ } else if (key.equals(HMS_DB_OWNER_TYPE) && value != null) {
+ database.setOwnerType(PrincipalType.valueOf(value));
} else {
if (value != null) {
parameter.put(key, value);
}
}
});
+
+ if (database.getOwnerName() == null) {
+ database.setOwnerName(System.getProperty("user.name"));
Review Comment:
Hi, its probably my fault here, @haizhou-zhao had this initially but it seemed there was some unresolved concern at the time : https://github.com/apache/iceberg/pull/6045#discussion_r1004221149 , so I actually asked @haizhou-zhao to follow what the table implementation uses for consistency , and we could change both to UserGroupInformation.getCurrentUser() later.
But looks like its cleared up, @gaborkaszab ? If so we can go ahead and use UserGroupInformation.getCurrentUser() here and then change table to be consistent as well?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#issuecomment-1319223257
thanks @szehon-ho . All comments from the last round of review has been taken and committed.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1013004798
##########
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:
yes, nice catch.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1013062617
##########
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:
Not sure if I get your suggestion exactly. Do you mean using parameterized junit test here?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1012079739
##########
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:
ack, @danielcweeks @gaborkaszab I'll rename those properties based on Daniel's suggestions.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for group ownership
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1004726357
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -532,13 +536,27 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
database.setName(namespace.level(0));
database.setLocationUri(databaseLocation(namespace.level(0)));
+ // default ownership is determined from Hadoop.UGI.currentUser
Review Comment:
Your concern make sense.
Currently, `convertToDatabase` is used by `createNamespace`, `setProperties` and `removeProperties`.
Here is the change I propose:
1. HiveCatalog.class should expose a public method named `setOwnership`, it helps user setting ownership either at creation time or if they intentionally want ownership change for a namespace. Like `setProperties`, `setOwnership` can be only invoked on existing `Namespaces`
2. Under the hood, only `createNamespace` and `setOwnership` can set/alter ownership; `setProperties` and `removeProperties` cannot change ownership in anyway
a. If user attempts to change ownership with `setProperties` and `removeProperties`, then they should explicitly get a RuntimeException. I'm against just silently ignoring users' attempts to change ownership with `setProperties` and `removeProperties` because that does not foster the right user behavior.
Let me know what you think.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1010574847
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -532,13 +536,27 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
database.setName(namespace.level(0));
database.setLocationUri(databaseLocation(namespace.level(0)));
+ // default ownership is determined from Hadoop.UGI.currentUser
+ // and the owner type is default to individual user
+ try {
+ // getShortUserName will remove the realm part of the whole username
+ database.setOwnerName(UserGroupInformation.getCurrentUser().getShortUserName());
Review Comment:
Splitting UGI changes into another patch later. So this is no longer relevant for this PR.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1010577209
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -532,13 +536,27 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
database.setName(namespace.level(0));
database.setLocationUri(databaseLocation(namespace.level(0)));
+ // default ownership is determined from Hadoop.UGI.currentUser
Review Comment:
ack, updated in the latest commit
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] danielcweeks commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1010730010
##########
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:
I feel like we probably want to update these properties to be consistent with the rest of the table properties convention (i.e. `hive.metastore.table.owner`, `hive.metastore.database.owner`, `hive.metastore.database.owner-type`). Is there a specific reason for the proposed property names?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] gaborkaszab commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1017594660
##########
core/src/main/java/org/apache/iceberg/TableProperties.java:
##########
@@ -360,5 +360,7 @@ private TableProperties() {}
public static final String UPSERT_ENABLED = "write.upsert.enabled";
public static final boolean UPSERT_ENABLED_DEFAULT = false;
- public static final String HMS_TABLE_OWNER = "hms_table_owner";
+ public static final String HMS_TABLE_OWNER = "hive.metastore.table.owner";
Review Comment:
Just to be on the safe side I opened a one-liner PR to rename the HMS_TABLE_OWNER that we can squeeze into 1.1.0 easier and once that's in we don't have to break backward compatibility.
https://github.com/apache/iceberg/pull/6154
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] danielcweeks commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1014232442
##########
core/src/main/java/org/apache/iceberg/TableProperties.java:
##########
@@ -360,5 +360,7 @@ private TableProperties() {}
public static final String UPSERT_ENABLED = "write.upsert.enabled";
public static final boolean UPSERT_ENABLED_DEFAULT = false;
- public static final String HMS_TABLE_OWNER = "hms_table_owner";
+ public static final String HMS_TABLE_OWNER = "hive.metastore.table.owner";
Review Comment:
@haizhou-zhao thanks for updating the property names, but now looking more closely at how these are used, I don't think they should live in the class.
Two of the properties are database properties (not table properties) and they are all hive specific. We're trying to keep the properties here isolated to general properties for tables (I know there are a few spark ones, but they should probably move as well).
I think it makes sense just to put these as properties in the HiveCatalog class or create a HiveCatalogProperties to encapsulate these behaviors.
Other than that, this looks good.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for group ownership
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1004833814
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -532,13 +536,27 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
database.setName(namespace.level(0));
database.setLocationUri(databaseLocation(namespace.level(0)));
+ // default ownership is determined from Hadoop.UGI.currentUser
+ // and the owner type is default to individual user
+ try {
+ // getShortUserName will remove the realm part of the whole username
+ database.setOwnerName(UserGroupInformation.getCurrentUser().getShortUserName());
Review Comment:
"we should do this only if there is no HMS_DB_OWNER set in 'meta'"
Make sense, yet I'd encourage uniformity between database ownership and table ownership. If you look at how table ownership is handled right now, there is a default owner (`System.getProperty("user.name")`) if no owner is set: [ref](https://github.com/apache/iceberg/blob/master/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java#L429).
I see there's another places where you comment about UGI, so I'm making comments on using UGI there.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] gaborkaszab commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for group ownership
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1004161902
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -532,13 +536,27 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
database.setName(namespace.level(0));
database.setLocationUri(databaseLocation(namespace.level(0)));
+ // default ownership is determined from Hadoop.UGI.currentUser
+ // and the owner type is default to individual user
+ try {
+ // getShortUserName will remove the realm part of the whole username
+ database.setOwnerName(UserGroupInformation.getCurrentUser().getShortUserName());
Review Comment:
I don't know how expensive it is to get the current user from UserGroupInformation (taking into account it's Hadoop functionality) but I think we should do this only if there is no HMS_DB_OWNER set in 'meta'.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -532,13 +536,27 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
database.setName(namespace.level(0));
database.setLocationUri(databaseLocation(namespace.level(0)));
+ // default ownership is determined from Hadoop.UGI.currentUser
+ // and the owner type is default to individual user
+ try {
+ // getShortUserName will remove the realm part of the whole username
+ database.setOwnerName(UserGroupInformation.getCurrentUser().getShortUserName());
+ database.setOwnerType(PrincipalType.USER);
+ } catch (IOException e) {
+ LOG.error("Unable to determine the current UGI user", e);
+ throw new RuntimeException(e);
Review Comment:
If there is an exception, can't we fallback to the original behaviour of not setting these fields?
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -422,11 +425,22 @@ private Table newHmsTable(TableMetadata metadata) {
Preconditions.checkNotNull(metadata, "'metadata' parameter can't be null");
final long currentTimeMillis = System.currentTimeMillis();
+ String defaultUser;
+ // default ownership is determined from Hadoop.UGI.currentUser
Review Comment:
nit: line could be up to 100 chars long
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -532,13 +536,27 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
database.setName(namespace.level(0));
database.setLocationUri(databaseLocation(namespace.level(0)));
+ // default ownership is determined from Hadoop.UGI.currentUser
Review Comment:
nit: I think the standard line length is 100 chars
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -422,11 +425,22 @@ private Table newHmsTable(TableMetadata metadata) {
Preconditions.checkNotNull(metadata, "'metadata' parameter can't be null");
final long currentTimeMillis = System.currentTimeMillis();
+ String defaultUser;
+ // default ownership is determined from Hadoop.UGI.currentUser
+ // and the owner type is default to individual user
+ try {
+ // getShortUserName will remove the realm part of the whole username
+ defaultUser = UserGroupInformation.getCurrentUser().getShortUserName();
Review Comment:
Is this UserGroupInformation.getCurrentUser() always available?
Is that different from System.getProperty("user.name")? Could you help me understand what is the benefit of using this instead? What I have in mind here is that in case UGI gives a different user than what we get now from the system then this would be a breaking change (except if we consider this a bug fix). And if it gives the same user then there is no point of this part of the change. I might miss something here, though.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -422,11 +425,22 @@ private Table newHmsTable(TableMetadata metadata) {
Preconditions.checkNotNull(metadata, "'metadata' parameter can't be null");
final long currentTimeMillis = System.currentTimeMillis();
+ String defaultUser;
+ // default ownership is determined from Hadoop.UGI.currentUser
+ // and the owner type is default to individual user
+ try {
+ // getShortUserName will remove the realm part of the whole username
+ defaultUser = UserGroupInformation.getCurrentUser().getShortUserName();
+ } catch (IOException e) {
+ LOG.error("Unable to determine the current UGI user", e);
+ throw new RuntimeException(e);
Review Comment:
Similarly to the DB code, in case of an exception, can't we fallback to the original behaviour?
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -437,6 +451,18 @@ private Table newHmsTable(TableMetadata metadata) {
null,
TableType.EXTERNAL_TABLE.toString());
+ // hive2 does not support setting table ownership type, therefore we leave it as
+ // a table property. Once upgraded to hive3, then we can simply set
+ // newTable.setOwnerType(PrincipalType.valueOf(metadata.get(HMS_TABLE_OWNER_TYPE)))
+ String ownerType = metadata.property(TableProperties.HMS_TABLE_OWNER_TYPE, null);
+ if (ownerType != null && ownerType.equalsIgnoreCase(PrincipalType.GROUP.name())) {
+ newTable
+ .getParameters()
+ .put(TableProperties.HMS_TABLE_OWNER_TYPE, PrincipalType.GROUP.name());
Review Comment:
Cant you simply put ownerType instead of PrincipalType.GROUP.name()?
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -437,6 +451,18 @@ private Table newHmsTable(TableMetadata metadata) {
null,
TableType.EXTERNAL_TABLE.toString());
+ // hive2 does not support setting table ownership type, therefore we leave it as
Review Comment:
I'm not really sure about this approach. If we can't add ownership type into HMS and have to use a table property for this purpose would the clients of Iceberg respect this table property or would it need extra implementation to get the ownership type from the property?
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -358,6 +388,36 @@ public void testCreateNamespace() throws Exception {
"There no same location for db and namespace", database2.getLocationUri(), hiveLocalDir);
}
+ @Test
+ public void testCreateNamespaceWithOwnership() throws Exception {
+ Namespace namespace1 = Namespace.of("userOwnership");
+ Map<String, String> prop1 =
+ ImmutableMap.of(
+ TableProperties.HMS_DB_OWNER,
+ "apache",
+ TableProperties.HMS_DB_OWNER_TYPE,
+ PrincipalType.USER.name());
+ catalog.createNamespace(namespace1, prop1);
+ Database database1 = metastoreClient.getDatabase(namespace1.toString());
+
+ Assert.assertEquals("apache", database1.getOwnerName());
+ Assert.assertEquals(PrincipalType.USER, database1.getOwnerType());
+
+ Map<String, String> prop2 =
+ ImmutableMap.of(
+ TableProperties.HMS_DB_OWNER,
+ "iceberg",
+ TableProperties.HMS_DB_OWNER_TYPE,
+ PrincipalType.GROUP.name());
+ Namespace namespace2 = Namespace.of("groupOwnership");
+
+ catalog.createNamespace(namespace2, prop2);
+ Database database2 = metastoreClient.getDatabase(namespace2.toString());
+
+ Assert.assertEquals("iceberg", database2.getOwnerName());
+ Assert.assertEquals(PrincipalType.GROUP, database2.getOwnerType());
+ }
Review Comment:
Can we also add a check to verify that PrincipalType.USER is the default when we don't provide the ownership type in parameters?
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -270,9 +275,34 @@ public void testCreateTableWithOwner() throws Exception {
Assert.assertEquals(owner, hmsTable.getOwner());
Map<String, String> hmsTableParams = hmsTable.getParameters();
Assert.assertFalse(hmsTableParams.containsKey(TableProperties.HMS_TABLE_OWNER));
+ Assert.assertEquals(
+ PrincipalType.USER.name(), hmsTableParams.get(TableProperties.HMS_TABLE_OWNER_TYPE));
} finally {
catalog.dropTable(tableIdent);
}
+
+ TableIdentifier tableIdent2 = TableIdentifier.of(DB_NAME, "tbl_group_owned");
Review Comment:
The below part of this test is almost identical to the part above except the table properties. Would it make sense to add a helper function that executes the test based on different properties that it gets as a parameter?
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -422,11 +425,22 @@ private Table newHmsTable(TableMetadata metadata) {
Preconditions.checkNotNull(metadata, "'metadata' parameter can't be null");
final long currentTimeMillis = System.currentTimeMillis();
+ String defaultUser;
+ // default ownership is determined from Hadoop.UGI.currentUser
+ // and the owner type is default to individual user
+ try {
+ // getShortUserName will remove the realm part of the whole username
+ defaultUser = UserGroupInformation.getCurrentUser().getShortUserName();
Review Comment:
Similarly to the DB code, if there is the HMS_TABLE_OWNER property set then we shouldn't call this Hadoop function.
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -270,9 +275,34 @@ public void testCreateTableWithOwner() throws Exception {
Assert.assertEquals(owner, hmsTable.getOwner());
Map<String, String> hmsTableParams = hmsTable.getParameters();
Assert.assertFalse(hmsTableParams.containsKey(TableProperties.HMS_TABLE_OWNER));
+ Assert.assertEquals(
+ PrincipalType.USER.name(), hmsTableParams.get(TableProperties.HMS_TABLE_OWNER_TYPE));
} finally {
catalog.dropTable(tableIdent);
}
+
+ TableIdentifier tableIdent2 = TableIdentifier.of(DB_NAME, "tbl_group_owned");
+ String location2 = temp.newFolder("tbl_group_owned").toString();
+ String owner2 = "some_group_owner";
+ ImmutableMap<String, String> properties2 =
+ ImmutableMap.of(
+ TableProperties.HMS_TABLE_OWNER,
+ owner2,
+ TableProperties.HMS_TABLE_OWNER_TYPE,
+ PrincipalType.GROUP.name());
+
+ try {
+ Table table2 = catalog.createTable(tableIdent2, schema, spec, location2, properties2);
+ org.apache.hadoop.hive.metastore.api.Table hmsTable =
+ metastoreClient.getTable(DB_NAME, "tbl_group_owned");
+ Assert.assertEquals(owner2, hmsTable.getOwner());
+ Map<String, String> hmsTableParams = hmsTable.getParameters();
+ Assert.assertFalse(hmsTableParams.containsKey(TableProperties.HMS_TABLE_OWNER));
+ Assert.assertEquals(
+ PrincipalType.GROUP.name(), hmsTableParams.get(TableProperties.HMS_TABLE_OWNER_TYPE));
+ } finally {
+ catalog.dropTable(tableIdent2);
+ }
Review Comment:
I wonder if it would make sense to check the owner when no table property is given.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -437,6 +451,18 @@ private Table newHmsTable(TableMetadata metadata) {
null,
TableType.EXTERNAL_TABLE.toString());
+ // hive2 does not support setting table ownership type, therefore we leave it as
+ // a table property. Once upgraded to hive3, then we can simply set
+ // newTable.setOwnerType(PrincipalType.valueOf(metadata.get(HMS_TABLE_OWNER_TYPE)))
+ String ownerType = metadata.property(TableProperties.HMS_TABLE_OWNER_TYPE, null);
+ if (ownerType != null && ownerType.equalsIgnoreCase(PrincipalType.GROUP.name())) {
+ newTable
+ .getParameters()
+ .put(TableProperties.HMS_TABLE_OWNER_TYPE, PrincipalType.GROUP.name());
+ } else { // default: individual ownership
Review Comment:
I think instead of this else branch you could pass PrincipalType.USER.name() to metadata.property() instead of null as a second parameter in L457 that is in fact the default value when the first param is not a valid key.
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -261,7 +262,11 @@ public void testCreateTableWithOwner() throws Exception {
String location = temp.newFolder("tbl").toString();
String owner = "some_owner";
ImmutableMap<String, String> properties =
- ImmutableMap.of(TableProperties.HMS_TABLE_OWNER, owner);
+ ImmutableMap.of(
+ TableProperties.HMS_TABLE_OWNER,
+ owner,
+ TableProperties.HMS_TABLE_OWNER_TYPE,
Review Comment:
I'd also keep the test coverage when there is no HMS_TABLE_OWNER_TYPE is set.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -532,13 +536,27 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
database.setName(namespace.level(0));
database.setLocationUri(databaseLocation(namespace.level(0)));
+ // default ownership is determined from Hadoop.UGI.currentUser
Review Comment:
As I see it's not just createNamespace() that calls this convertToDatabase() function but it's also used in HiveCatalog.setProperties() and removeProperties(). I'm no expert here but what if user-A calls createNamespace() and then user-B calls setProperties() (but not changing the DB owner) then won't we end up unintentionally changing the DB owner as a result of convertToDatabase() setting the owner during a setProperties() as well?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] gaborkaszab commented on pull request #6045: [iceberg-hive-metastore] Add support for group ownership
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#issuecomment-1290283087
Thanks for this patch, @haizhou-zhao! I managed to go through the changes and left some comments. Hope they make sense.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1013067243
##########
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:
see the "remove_ownership_noop" test case at line 576 above.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1013065188
##########
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:
see the precondition check added above
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#issuecomment-1309388616
Hey Gabor,
Thanks for your last round of review. All your comments make sense to me and taken. Major changes in the latest commit:
1. createNamespace, setProp, removeProp each check input parameters separately
2. createNamespace allows users to create with both owner & owner-type specified or with only owner specified (in which case default owner-type to "user")
3. setProp requires users to specify owner & owner-type at the same time or not at all (in which case, there's no ownership change)
4. removeProp requires users to specify owner & owner-type at the same time (in which case, ownership is reset to default) or not at all (in which case, ownership is not changed)
5. unit test modified accordingly
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] danielcweeks commented on pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#issuecomment-1316034660
> @danielcweeks thx for your last round of review. I agree with you that UGI.currentUser is a better default owner, though I plan to make that change in a separate follow up PR. (See my reply above for more details)
>
> Also, I tried to rebase on master branch, but the change to `TableProperties` is still there.
I'm fine separating UGI into a separate PR, but in that case we should hold off until the `1.1.0` release is finalized. I wouldn't want to introduce something that would like change behavior across the releases.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] danielcweeks commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1022041978
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -539,12 +577,22 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
database.setDescription(value);
} else if (key.equals("location")) {
database.setLocationUri(value);
+ } else if (key.equals(HMS_DB_OWNER)) {
+ database.setOwnerName(value);
+ } else if (key.equals(HMS_DB_OWNER_TYPE) && value != null) {
+ database.setOwnerType(PrincipalType.valueOf(value));
} else {
if (value != null) {
parameter.put(key, value);
}
}
});
+
+ if (database.getOwnerName() == null) {
+ database.setOwnerName(System.getProperty("user.name"));
Review Comment:
I think it actually makes sense to use just `user.name` in other places in Iceberg since we want to avoid hard dependencies on Hadoop/Hive where possible. In this case though, Hive requires a Hadoop dependency and since this catalog is likely used in that context, `UserGroupInformation` seems like it would produce more consistent behavior with the Spark/Hive engine itself.
Other catalog implementation probably should not use UGI since they may be running on environments where hadoop is not available (e.g. some flink environments don't have hadoop).
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1025379199
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -518,11 +541,26 @@ private Map<String, String> convertToMetadata(Database database) {
if (database.getDescription() != null) {
meta.put("comment", database.getDescription());
}
+ if (database.getOwnerName() != null) {
+ meta.put(HMS_DB_OWNER, database.getOwnerName());
+ if (database.getOwnerType() != null) {
+ meta.put(HMS_DB_OWNER_TYPE, database.getOwnerType().name());
+ }
+ }
return meta;
}
Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
+ Preconditions.checkArgument(
+ meta.get(HMS_DB_OWNER_TYPE) == null
Review Comment:
Optional: I actually think here there may not be such a need for this extra bulky check, and call to a third party library? If I'm not mistaken the valueOf anyway throws IllegalArgumentException if it does not match a value, and it would be pretty easy for user to find out?
not sure if it throws exception for nulls, if not we could do that I suppose.
Just trying to reduce bulky code here if possible, and avoid call to third party library (that can make future upgrades kind of hard)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1026348753
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -356,6 +365,11 @@ 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),
+ String.format(
Review Comment:
Ah sorry should have been more clear, Preconditions(..., "%s %s", myVarA, myVarB) should work, that's what I meant, no need to explicitly do String.format (see the other Iceberg uses for example)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1010591538
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -539,12 +541,26 @@ 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)) {
+ database.setOwnerType(PrincipalType.valueOf(value));
} else {
if (value != null) {
parameter.put(key, value);
}
}
});
+
+ // default ownership on db level kept uniform with table level, see HiveTableOperations#
Review Comment:
comment removed in the latest commit
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1010576636
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -532,13 +536,27 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
database.setName(namespace.level(0));
database.setLocationUri(databaseLocation(namespace.level(0)));
+ // default ownership is determined from Hadoop.UGI.currentUser
Review Comment:
Eventually, I made a code change so that users are allowed to alter ownership properties through `setProperties` and `removeProperties` methods. See my latest commit for more details.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for group ownership
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1005080607
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -270,9 +275,34 @@ public void testCreateTableWithOwner() throws Exception {
Assert.assertEquals(owner, hmsTable.getOwner());
Map<String, String> hmsTableParams = hmsTable.getParameters();
Assert.assertFalse(hmsTableParams.containsKey(TableProperties.HMS_TABLE_OWNER));
+ Assert.assertEquals(
+ PrincipalType.USER.name(), hmsTableParams.get(TableProperties.HMS_TABLE_OWNER_TYPE));
} finally {
catalog.dropTable(tableIdent);
}
+
+ TableIdentifier tableIdent2 = TableIdentifier.of(DB_NAME, "tbl_group_owned");
Review Comment:
Yes, good idea.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1010578058
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -422,11 +425,22 @@ private Table newHmsTable(TableMetadata metadata) {
Preconditions.checkNotNull(metadata, "'metadata' parameter can't be null");
final long currentTimeMillis = System.currentTimeMillis();
+ String defaultUser;
+ // default ownership is determined from Hadoop.UGI.currentUser
+ // and the owner type is default to individual user
+ try {
+ // getShortUserName will remove the realm part of the whole username
+ defaultUser = UserGroupInformation.getCurrentUser().getShortUserName();
+ } catch (IOException e) {
+ LOG.error("Unable to determine the current UGI user", e);
+ throw new RuntimeException(e);
Review Comment:
Splitting UGI changes into another patch later. So this is no longer relevant for this PR.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1010591871
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -257,21 +259,42 @@ public void testReplaceTxnBuilder() throws Exception {
public void testCreateTableWithOwner() throws Exception {
Schema schema = getTestSchema();
PartitionSpec spec = PartitionSpec.builderFor(schema).bucket("data", 16).build();
- TableIdentifier tableIdent = TableIdentifier.of(DB_NAME, "tbl");
- String location = temp.newFolder("tbl").toString();
String owner = "some_owner";
- ImmutableMap<String, String> properties =
- ImmutableMap.of(TableProperties.HMS_TABLE_OWNER, owner);
-
+ Map<String, String> prop = ImmutableMap.of(TableProperties.HMS_TABLE_OWNER, owner);
+
+ createTableAndVerifyOwnership(DB_NAME, "tbl", schema, spec, prop, owner);
+
+ prop = ImmutableMap.of();
+
+ // a table without an owner explicitly specified should have a default owner
+ createTableAndVerifyOwnership(
+ DB_NAME,
+ "no_explicit_owner_specified_tbl",
+ schema,
+ spec,
+ prop,
Review Comment:
This change is no longer needed
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1010580952
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -437,6 +451,18 @@ private Table newHmsTable(TableMetadata metadata) {
null,
TableType.EXTERNAL_TABLE.toString());
+ // hive2 does not support setting table ownership type, therefore we leave it as
Review Comment:
Decided to not support group ownership for tables here since hive2 does not support it. Hopefully this will simplify this PR. So this thread may not be relevant anymore.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Add support for individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1010588701
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -422,11 +425,22 @@ private Table newHmsTable(TableMetadata metadata) {
Preconditions.checkNotNull(metadata, "'metadata' parameter can't be null");
final long currentTimeMillis = System.currentTimeMillis();
+ String defaultUser;
+ // default ownership is determined from Hadoop.UGI.currentUser
Review Comment:
ack, addressed in the latest commit.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] danielcweeks commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1010731108
##########
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:
Also, @gaborkaszab is `user` the only owner type for tables?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1013005818
##########
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:
Yes, let's add a precondition check to forbid setting owner type without setting owner.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1013008144
##########
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:
Yes, nice catch. I have perhaps made some copy paste error.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1018360013
##########
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:
Make sense, taken.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1018359737
##########
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:
Make sense, taken
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1025379199
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -518,11 +541,26 @@ private Map<String, String> convertToMetadata(Database database) {
if (database.getDescription() != null) {
meta.put("comment", database.getDescription());
}
+ if (database.getOwnerName() != null) {
+ meta.put(HMS_DB_OWNER, database.getOwnerName());
+ if (database.getOwnerType() != null) {
+ meta.put(HMS_DB_OWNER_TYPE, database.getOwnerType().name());
+ }
+ }
return meta;
}
Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
+ Preconditions.checkArgument(
+ meta.get(HMS_DB_OWNER_TYPE) == null
Review Comment:
Optional: I actually think here there may not be such a need for this extra bulky check, and call to a third party library? If I'm not mistaken the valueOf anyway throws IllegalArgumentException if it does not match a value, and it would be pretty easy for user to find out?
not sure if it throws exception for nulls, if not we could do that I suppose.
Just trying to reduce bulky code here if possible, and also avoid call to third party library if there's no existing precedent and overwhelming need, as that can make future upgrades kind of hard.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on a diff in pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1026350489
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -518,11 +541,26 @@ private Map<String, String> convertToMetadata(Database database) {
if (database.getDescription() != null) {
meta.put("comment", database.getDescription());
}
+ if (database.getOwnerName() != null) {
+ meta.put(HMS_DB_OWNER, database.getOwnerName());
+ if (database.getOwnerType() != null) {
+ meta.put(HMS_DB_OWNER_TYPE, database.getOwnerType().name());
+ }
+ }
return meta;
}
Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
+ Preconditions.checkArgument(
+ meta.get(HMS_DB_OWNER_TYPE) == null
Review Comment:
Thanks for understanding!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] haizhou-zhao commented on pull request #6045: [iceberg-hive-metastore] Support setting individual and group ownership for Namespace
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#issuecomment-1317323654
Thank you all! In that case, I'll raise a PR for UGI change later and we can merge after 1.1 release.
Let me know if there's more I can do for this PR.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org