[iceberg-hive-metastore] Support setting individual and group ownership for Namespace #6045

haizhou-zhao · 2022-10-25T04:38:59Z

Build on top of this PR: #5763

This is to add individual and group ownership support for namespaces in iceberg-hive-metastore module

Change list:

Add extra properties: owner and ownerType, on the namespace. And those property will be converted to Hive DB owner and ownerType when creating DB with Hive Catalog.
HiveCatalog's createNamespace, setProperties & removeProperties are impacted. Added new unit test accordingly.

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

gaborkaszab · 2022-10-25T08:56:10Z

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


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?

Your concern make sense.

Currently, convertToDatabase is used by createNamespace, setProperties and removeProperties.

Here is the change I propose:

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

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.

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.

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

gaborkaszab · 2022-10-25T09:48:41Z

Thanks for this patch, @haizhou-zhao! I managed to go through the changes and left some comments. Hope they make sense.

szehon-ho · 2022-10-31T19:33:46Z

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 to separate pr, as there seems to be some questions
Only set the type if we are on Hive 3 , and do nothing on Hive2 as its not supported there (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 make createNamespace in line with the createTable behavior today.

szehon-ho

I think looks much simpler now, thanks @haizhou-zhao . Some comments

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

core/src/main/java/org/apache/iceberg/TableProperties.java

gaborkaszab

Reducing the scope of this patch was a great idea, @haizhou-zhao! It is way more readable this way. I've left some comments but nothing really serious. What do you think?

core/src/main/java/org/apache/iceberg/TableProperties.java

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

gaborkaszab · 2022-11-02T12:47:31Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

+    String expectedOwner = "apache";
+    PrincipalType expectedOwnerType = PrincipalType.USER;


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.

Not sure if I get your suggestion exactly. Do you mean using parameterized junit test here?

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.

Got it. I'll make the change

gaborkaszab · 2022-11-02T12:51:12Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

+
+    createNamespaceAndVerifyOwnership("ownedByDefaultUser", prop, expectedOwner, expectedOwnerType);
+
+    prop = ImmutableMap.of(TableProperties.HMS_DB_OWNER_TYPE, PrincipalType.GROUP.name());


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?

Yes, let's add a precondition check to forbid setting owner type without setting owner.

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

gaborkaszab · 2022-11-02T12:57:43Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

+    removeNamespaceOwnershipAndVerify("remove_group_ownership", prop);
+  }
+
+  private void removeNamespaceOwnershipAndVerify(String name, Map<String, String> prop)


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).

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).

see the "remove_ownership_noop" test case at line 593 above.

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.

I see what you mean, make sense

danielcweeks · 2022-11-04T16:23:39Z

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";


@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.

thanks @danielcweeks , I'll put them in HiveCatalog class.

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?

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.

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.
#6154

danielcweeks

See comment on properties.

gaborkaszab

Thanks for taking care of the comments so far! I feel we are getting to the end soon, my comments are mainly about tests.

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

gaborkaszab · 2022-11-08T08:47:55Z

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";


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?

gaborkaszab · 2022-11-08T08:51:20Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

+    String expectedOwner = "apache";
+    PrincipalType expectedOwnerType = PrincipalType.USER;


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.

gaborkaszab · 2022-11-08T08:56:24Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

+    removeNamespaceOwnershipAndVerify("remove_group_ownership", prop);
+  }
+
+  private void removeNamespaceOwnershipAndVerify(String name, Map<String, String> prop)


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.

gaborkaszab · 2022-11-08T09:01:49Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

+
+    prop = ImmutableMap.of();
+
+    removeNamespaceOwnershipAndVerify("remove_ownership_noop", prop);


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.

em, good idea.

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

gaborkaszab

Thanks for another round of changes!
I mainly have 2 main comments:

I made another PR to change the HMS_TABLE_OWNER value to follow the naming convention. It got merged so a rebase could be useful here. As a result we shouldn't worry about breaking backward compatibility anymore if this PR doesn't make it into 1.1.0.
The use cases when the DB_OWNER is not given but DB_OWNER_TYPE is given seems a bit off to me. I left some further explanation in comments.

gaborkaszab · 2022-11-09T10:05:23Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

+
+    createNamespaceAndVerifyOwnership(
+        "default_ownership_2",
+        ImmutableMap.of(HiveCatalog.HMS_DB_OWNER_TYPE, PrincipalType.USER.name()),


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.

cool, taken

gaborkaszab · 2022-11-09T10:28:53Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

+    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()),


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.

Taken. Indeed, and this works with what you later suggested: have separate precondition check at each public API, not at the convertToDatabaseLevel

gaborkaszab · 2022-11-09T10:30:50Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

+            "some_owner",
+            HiveCatalog.HMS_DB_OWNER_TYPE,
+            PrincipalType.GROUP.name()),
+        ImmutableMap.of(HiveCatalog.HMS_DB_OWNER_TYPE, PrincipalType.USER.name()),


See my comment above.

gaborkaszab · 2022-11-09T10:48:44Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

+        PrincipalType.GROUP);
+
+    AssertHelpers.assertThrows(
+        "Setting non default value for "


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?

Make sense, taken.

gaborkaszab · 2022-11-09T10:51:45Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

+    removeNamespaceOwnershipAndVerify(
+        "remove_ownership_noop_0",
+        ImmutableMap.of(HiveCatalog.HMS_DB_OWNER, "some_owner"),
+        ImmutableSet.of(HiveCatalog.HMS_DB_OWNER_TYPE),


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.

Make sense, taken

gaborkaszab · 2022-11-09T17:28:27Z

@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 #6154 so I believe this one shouldn't be a blocker for 1.1.0.

haizhou-zhao · 2022-11-09T21:20:52Z

Hey Gabor,

Thanks for your last round of review. All your comments make sense to me and taken. Major changes in the latest commit:

createNamespace, setProp, removeProp each check input parameters separately
createNamespace allows users to create with both owner & owner-type specified or with only owner specified (in which case default owner-type to "user")
setProp requires users to specify owner & owner-type at the same time or not at all (in which case, there's no ownership change)
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)
unit test modified accordingly

gaborkaszab

This PR is looking pretty good. I just had some minor comments.

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

haizhou-zhao · 2022-11-11T22:00:02Z

@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.

danielcweeks · 2022-11-14T19:50:20Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

          } else {
            if (value != null) {
              parameter.put(key, value);
            }
          }
        });
+
+    if (database.getOwnerName() == null) {
+      database.setOwnerName(System.getProperty("user.name"));


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.

Hi, its probably my fault here, @haizhou-zhao had this initially but it seemed there was some unresolved concern at the time : #6045 (comment) , 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.

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).

@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!

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.

danielcweeks · 2022-11-14T19:50:56Z

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")),


Possibly consider UserGroupInformation.getCurrentUser() here as well.

Original change request was addressed

danielcweeks · 2022-11-14T19:53:24Z

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.

haizhou-zhao · 2022-11-14T21:30:28Z

@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.

gaborkaszab · 2022-11-15T11:49:58Z

@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.

gaborkaszab · 2022-11-15T11:54:20Z

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!

danielcweeks · 2022-11-15T23:45:09Z

@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.

szehon-ho · 2022-11-16T09:25:18Z

@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

haizhou-zhao · 2022-11-16T16:43:16Z

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.

szehon-ho · 2022-11-16T17:27:56Z

@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.

gaborkaszab · 2022-11-17T08:37:18Z

@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.

szehon-ho · 2022-11-17T09:18:14Z

Thought so, no worries :), thought it was worth a try

szehon-ho

Hi thanks for getting close @haizhou-zhao , had a few minor comments for your consideration.

szehon-ho · 2022-11-17T15:48:12Z

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 "


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

haizhou-zhao · 2022-11-17T21:23:56Z

thanks @szehon-ho . All comments from the last round of review has been taken and committed.

szehon-ho

Thanks @haizhou-zhao for those nits! Just one perhaps misunderstanding there.

szehon-ho · 2022-11-18T11:52:22Z

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(


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)

…tion (apache#602) AWS Kms based Kms Client Implementation for envelope encryption

Draft save Add unit test Fix for compilation failure Remove some unneeded change to simplify Remove unneeded file from change Remove unneeded change Remove whitespace change Remove unneeded imports Adding unit test for invalid owner type and remove noop Moving hive properties to HiveCatalog Fix check style Misc. Unit test fix Remove unneeded change Change preconditions

szehon-ho · 2022-11-28T14:52:14Z

Merged, thanks @haizhou-zhao . Thanks @gaborkaszab and @danielcweeks for extended review.

github-actions bot added core hive labels Oct 25, 2022

gaborkaszab reviewed Oct 25, 2022

View reviewed changes

danielcweeks self-requested a review October 25, 2022 15:42

haizhou-zhao force-pushed the master branch from cdf879c to dda2c0d Compare October 31, 2022 22:28

github-actions bot added the AWS label Oct 31, 2022

haizhou-zhao changed the title ~~[iceberg-hive-metastore] Add support for group ownership~~ [iceberg-hive-metastore] Add support for individual and group ownership for Namespace Oct 31, 2022

github-actions bot removed the AWS label Oct 31, 2022

szehon-ho reviewed Nov 1, 2022

View reviewed changes

haizhou-zhao force-pushed the master branch from bd5a41f to c9dfd90 Compare November 1, 2022 15:28

haizhou-zhao changed the title ~~[iceberg-hive-metastore] Add support for individual and group ownership for Namespace~~ [iceberg-hive-metastore] Support setting individual and group ownership for Namespace Nov 1, 2022

danielcweeks reviewed Nov 1, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/TableProperties.java Outdated Show resolved Hide resolved

gaborkaszab reviewed Nov 2, 2022

View reviewed changes

haizhou-zhao force-pushed the master branch from d96a10f to c8aeea7 Compare November 3, 2022 15:30

danielcweeks reviewed Nov 4, 2022

View reviewed changes

danielcweeks previously requested changes Nov 4, 2022

View reviewed changes

haizhou-zhao force-pushed the master branch from c8aeea7 to d8df2f6 Compare November 7, 2022 16:13

gaborkaszab reviewed Nov 8, 2022

View reviewed changes

haizhou-zhao force-pushed the master branch from 453d4c1 to bf7e683 Compare November 8, 2022 19:22

szehon-ho mentioned this pull request Nov 9, 2022

Core: Rename HMS_TABLE_OWNER to follow naming convention #6154

Merged

gaborkaszab reviewed Nov 9, 2022

View reviewed changes

danielcweeks added this to the Iceberg 1.1.0 Release milestone Nov 9, 2022

danielcweeks removed this from the Iceberg 1.1.0 Release milestone Nov 9, 2022

haizhou-zhao force-pushed the master branch from 96ac4f7 to 228d4fe Compare November 9, 2022 19:56

gaborkaszab reviewed Nov 10, 2022

View reviewed changes

danielcweeks reviewed Nov 14, 2022

View reviewed changes

haizhou-zhao force-pushed the master branch from 64b3565 to 2920595 Compare November 14, 2022 21:27

danielcweeks self-requested a review November 15, 2022 23:45

szehon-ho reviewed Nov 17, 2022

View reviewed changes

haizhou-zhao force-pushed the master branch from 2920595 to 50dd713 Compare November 17, 2022 18:56

szehon-ho reviewed Nov 18, 2022

View reviewed changes

JiJiTang and others added 5 commits November 18, 2022 11:47

Internal: Aws Kms based Kms Client Implementation for envelope encryp…

3448118

…tion (apache#602) AWS Kms based Kms Client Implementation for envelope encryption

Add another scenario for set namespace noop unit test

755aa1f

Format argument check string

1f40bc8

Format exception string

e0057d9

haizhou-zhao force-pushed the master branch from 50dd713 to e0057d9 Compare November 18, 2022 19:47

szehon-ho approved these changes Nov 21, 2022

View reviewed changes

szehon-ho merged commit 6b8f7e0 into apache:master Nov 28, 2022

nastra mentioned this pull request Nov 30, 2022

Core: Re-add and deprecate HMS_TABLE_OWNER to TableProperties #6314

Merged

haizhou-zhao mentioned this pull request Nov 30, 2022

Make UGI current user the default owner of Hive db&tbl #6324

Merged

		String expectedOwner = "apache";
		PrincipalType expectedOwnerType = PrincipalType.USER;


		createNamespaceAndVerifyOwnership("ownedByDefaultUser", prop, expectedOwner, expectedOwnerType);

		prop = ImmutableMap.of(TableProperties.HMS_DB_OWNER_TYPE, PrincipalType.GROUP.name());


		prop = ImmutableMap.of();

		removeNamespaceOwnershipAndVerify("remove_ownership_noop", prop);

[iceberg-hive-metastore] Support setting individual and group ownership for Namespace #6045

[iceberg-hive-metastore] Support setting individual and group ownership for Namespace #6045

Conversation

haizhou-zhao commented Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborkaszab commented Oct 25, 2022

szehon-ho commented Oct 31, 2022 • edited Loading

szehon-ho left a comment

Choose a reason for hiding this comment

gaborkaszab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haizhou-zhao Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielcweeks left a comment

Choose a reason for hiding this comment

gaborkaszab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborkaszab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haizhou-zhao Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haizhou-zhao Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborkaszab commented Nov 9, 2022

haizhou-zhao commented Nov 9, 2022

gaborkaszab left a comment

Choose a reason for hiding this comment

haizhou-zhao commented Nov 11, 2022

Choose a reason for hiding this comment

szehon-ho Nov 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haizhou-zhao Nov 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielcweeks commented Nov 14, 2022

haizhou-zhao commented Nov 14, 2022 • edited Loading

gaborkaszab commented Nov 15, 2022

gaborkaszab commented Nov 15, 2022

danielcweeks commented Nov 15, 2022

szehon-ho commented Nov 16, 2022

haizhou-zhao commented Nov 16, 2022

szehon-ho commented Nov 16, 2022 • edited Loading

gaborkaszab commented Nov 17, 2022

szehon-ho commented Nov 17, 2022

szehon-ho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haizhou-zhao commented Nov 17, 2022

haizhou-zhao commented Oct 25, 2022 •

edited

Loading

szehon-ho commented Oct 31, 2022 •

edited

Loading

haizhou-zhao Nov 3, 2022 •

edited

Loading

haizhou-zhao Nov 9, 2022 •

edited

Loading

haizhou-zhao Nov 9, 2022 •

edited

Loading

szehon-ho Nov 14, 2022 •

edited

Loading

haizhou-zhao Nov 14, 2022 •

edited

Loading

haizhou-zhao commented Nov 14, 2022 •

edited

Loading

szehon-ho commented Nov 16, 2022 •

edited

Loading

szehon-ho Nov 18, 2022 •

edited

Loading