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/11/30 23:29:22 UTC
[GitHub] [iceberg] haizhou-zhao opened a new pull request, #6324: Make UGI current user the default owner of Hive db&tbl
haizhou-zhao opened a new pull request, #6324:
URL: https://github.com/apache/iceberg/pull/6324
Following up PR on https://github.com/apache/iceberg/pull/6045
Change Summary:
1. [HiveCatalog] Change default owner of Hive db&tbl from OS username to UGI current user, this impacts create, alter and remove operations of db&table in HiveCatalog
2. Relevant unit test changes
--
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 #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1040055218
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -567,8 +569,13 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
});
if (database.getOwnerName() == null) {
- database.setOwnerName(System.getProperty("user.name"));
- database.setOwnerType(PrincipalType.USER);
+ try {
+ database.setOwnerName(UserGroupInformation.getCurrentUser().getUserName());
+ database.setOwnerType(PrincipalType.USER);
+ } catch (IOException e) {
+ throw new RuntimeException(
Review Comment:
Looking through the `UGI` path, it looks like the only likely failure is that security (e.g. kerberos) is enabled, but something is misconfigured or the user isn't logged in. It probably makes sense to just throw here, but we should use the java built-in `UncheckedIOException` since we have an IO cause (Iceberg's `RuntimeIOException` is probably best for when we don't have an IO cause).
--
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 #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1068790644
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveHadoopUtil.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.hive;
+
+import java.io.IOException;
+import org.apache.hadoop.security.UserGroupInformation;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class HiveHadoopUtil {
+
+ private static final Logger LOG = LoggerFactory.getLogger(HiveHadoopUtil.class);
+
+ private HiveHadoopUtil() {}
+
+ public static String currentUser() {
+ try {
+ return UserGroupInformation.getCurrentUser().getUserName();
+ } catch (IOException e) {
+ LOG.warn("Fail to obtain Hadoop UGI user", e);
Review Comment:
Adopted 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] rdblue merged pull request #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #6324:
URL: https://github.com/apache/iceberg/pull/6324
--
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 #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1038443524
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -567,8 +569,13 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
});
if (database.getOwnerName() == null) {
- database.setOwnerName(System.getProperty("user.name"));
- database.setOwnerType(PrincipalType.USER);
+ try {
+ database.setOwnerName(UserGroupInformation.getCurrentUser().getUserName());
+ database.setOwnerType(PrincipalType.USER);
+ } catch (IOException e) {
+ throw new RuntimeException(
Review Comment:
I think this was also discussed in https://github.com/apache/iceberg/pull/6045#discussion_r1004158438. Should we just warn and fallback? @gaborkaszab @danielcweeks
by the way, if we throw we have our own UnchekcedIOException
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -470,6 +481,22 @@ private void setHmsTableParameters(
// remove any props from HMS that are no longer present in Iceberg table props
obsoleteProps.forEach(parameters::remove);
+ if (metadata.property(HiveCatalog.HMS_TABLE_OWNER, null) != null) {
Review Comment:
Looks like this code is for the update case and is going a bit beyond original behavior (which was for create case).
cc @gaborkaszab for thoughts
And does it align with database/namespace update?
--
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] rdblue commented on a diff in pull request #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1066390419
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveHadoopUtil.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.hive;
+
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import org.apache.hadoop.security.UserGroupInformation;
+
+public class HiveHadoopUtil {
+
+ private HiveHadoopUtil() {}
+
+ public static String getUserName() {
Review Comment:
How about `currentUser` instead of `getUserName`? The verb "get" is not valuable and so Iceberg doesn't use it. Instead, we use a more specific word or omit it. In this case, I think `currentUser` makes 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] rdblue commented on a diff in pull request #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1066391533
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveHadoopUtil.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.hive;
+
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import org.apache.hadoop.security.UserGroupInformation;
+
+public class HiveHadoopUtil {
+
+ private HiveHadoopUtil() {}
+
+ public static String getUserName() {
+ try {
+ return UserGroupInformation.getCurrentUser().getUserName();
+ } catch (IOException e) {
+ throw new UncheckedIOException("Fail to obtain Hadoop UGI user", e);
Review Comment:
In what cases is `IOException` thrown? Should this fall back to the old behavior and return `System.getProperty("user.name")` instead? I don't like that we have now introduced a new error case based on Hadoop internals.
I think I'd prefer falling back to the old logic.
--
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] rdblue commented on a diff in pull request #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1067562662
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveHadoopUtil.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.hive;
+
+import java.io.IOException;
+import org.apache.hadoop.security.UserGroupInformation;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class HiveHadoopUtil {
+
+ private static final Logger LOG = LoggerFactory.getLogger(HiveHadoopUtil.class);
+
+ private HiveHadoopUtil() {}
+
+ public static String currentUser() {
+ try {
+ return UserGroupInformation.getCurrentUser().getUserName();
+ } catch (IOException e) {
+ LOG.warn("Fail to obtain Hadoop UGI user", e);
Review Comment:
Minor: to fit with conventions, I think the error message should be `Failed to get Hadoop user`.
* Error messages use past tense to describe what was attempted
* "Obtain" is no more specific than "get", so prefer the simpler word
* `UGI` is an internal Hadoop term that is more confusing than helpful
--
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 #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#issuecomment-1344302142
> Hey folks, comments from last round of review all taken and implemented.
>
> Specifically, on one comment: @gaborkaszab @szehon-ho I removed support for changing ownership for tables from this PR. But before I committed to create a follow up PR on that, I want to double check and make sure if you guys feel these worth implementing (which are something we do not have today):
>
> 1. When user remove ownership on a table, revert the owner of the table to default owner (current UGI)
> 2. When user alter the ownership on a table, change the owner of table accordingly
I think it does make sense to have a support for altering ownership. Impala has "ALTER TABLE xy SET OWNER" for this purpose, and I guess Hive also has something similar.
Yes, the use-cases 1) and 2) you mention make 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] rdblue commented on a diff in pull request #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1051690244
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -567,8 +570,13 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
});
if (database.getOwnerName() == null) {
- database.setOwnerName(System.getProperty("user.name"));
- database.setOwnerType(PrincipalType.USER);
+ try {
+ database.setOwnerName(UserGroupInformation.getCurrentUser().getUserName());
+ database.setOwnerType(PrincipalType.USER);
+ } catch (IOException e) {
+ throw new UncheckedIOException(
+ String.format("Fail to obtain default (UGI) user for database %s", database), e);
Review Comment:
I think this should fall back to the previous behavior. This should be a warning, not an exception.
Also, I think the message could be more clear: "Failed to fetch Hadoop user". No need to log the database, it's unrelated to what failed.
--
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 #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1044433283
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -422,11 +425,21 @@ private Table newHmsTable(TableMetadata metadata) {
Preconditions.checkNotNull(metadata, "'metadata' parameter can't be null");
final long currentTimeMillis = System.currentTimeMillis();
+ String defaultUser;
+ try {
Review Comment:
Actually there is one thing: I think in case the HMS_TABLE_OWNER is set we don't have to query the UGI.getCurrentUser() because it won't be used anyway.
--
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 #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#issuecomment-1341735907
Hey folks, comments from last round of review all taken and implemented.
Specifically, on one comment:
@gaborkaszab @szehon-ho I removed support for changing ownership for tables from this PR. But before I committed to create a follow up PR on that, I want to double check and make sure if you guys feel these worth implementing (which are something we do not have today):
1. When user remove ownership on a table, revert the owner of the table to default owner (current UGI)
2. When user alter the ownership on a table, change the owner of table 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] rdblue commented on a diff in pull request #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1051690764
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -246,23 +248,33 @@ public void testReplaceTxnBuilder() throws Exception {
}
@Test
- 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(HiveCatalog.HMS_TABLE_OWNER, owner);
+ public void testCreateTableWithOwner() throws IOException {
+ createTableAndVerifyOwner(
+ DB_NAME,
+ "tbl_specified_owner",
+ ImmutableMap.of(HiveCatalog.HMS_TABLE_OWNER, "some_owner"),
+ "some_owner");
+ createTableAndVerifyOwner(
+ DB_NAME,
+ "tbl_default_owner",
+ ImmutableMap.of(),
+ UserGroupInformation.getCurrentUser().getUserName());
+ }
+ private void createTableAndVerifyOwner(
+ String db, String tbl, Map<String, String> prop, String expectedOwner) {
try {
- Table table = catalog.createTable(tableIdent, schema, spec, location, properties);
- org.apache.hadoop.hive.metastore.api.Table hmsTable =
- metastoreClient.getTable(DB_NAME, "tbl");
- Assert.assertEquals(owner, hmsTable.getOwner());
- Map<String, String> hmsTableParams = hmsTable.getParameters();
- Assert.assertFalse(hmsTableParams.containsKey(HiveCatalog.HMS_TABLE_OWNER));
+ Schema schema = getTestSchema();
+ PartitionSpec spec = PartitionSpec.builderFor(schema).bucket("data", 16).build();
+ String location = temp.newFolder(tbl).toString();
+ catalog.createTable(TableIdentifier.of(db, tbl), schema, spec, location, prop);
+ org.apache.hadoop.hive.metastore.api.Table hmsTable = metastoreClient.getTable(db, tbl);
+ Assert.assertEquals(expectedOwner, hmsTable.getOwner());
+ Assert.assertFalse(hmsTable.getParameters().containsKey(HiveCatalog.HMS_TABLE_OWNER));
Review Comment:
Please roll back unnecessary changes to this code block. For example, removing `hmsTableParams`. We want to minimize changes.
Also, this can throw Exception. There is no need for try/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] rdblue commented on a diff in pull request #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1051690326
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -422,11 +425,23 @@ private Table newHmsTable(TableMetadata metadata) {
Preconditions.checkNotNull(metadata, "'metadata' parameter can't be null");
final long currentTimeMillis = System.currentTimeMillis();
+ String owner = metadata.properties().get(HiveCatalog.HMS_TABLE_OWNER);
+ if (owner == null) {
+ try {
Review Comment:
Since this is used in two places, can you make it a static method in a Hadoop util 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] rdblue commented on a diff in pull request #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1067564415
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -247,17 +249,28 @@ public void testReplaceTxnBuilder() throws Exception {
@Test
public void testCreateTableWithOwner() throws Exception {
+ createTableAndVerifyOwner(
Review Comment:
Why are these mixed into a single test method rather than two test methods? I think this makes it harder to follow
--
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 #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1051069226
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -422,11 +425,21 @@ private Table newHmsTable(TableMetadata metadata) {
Preconditions.checkNotNull(metadata, "'metadata' parameter can't be null");
final long currentTimeMillis = System.currentTimeMillis();
+ String defaultUser;
+ try {
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 #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1041496806
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -422,20 +424,29 @@ private Table newHmsTable(TableMetadata metadata) {
Preconditions.checkNotNull(metadata, "'metadata' parameter can't be null");
final long currentTimeMillis = System.currentTimeMillis();
- Table newTable =
- new Table(
- tableName,
- database,
- metadata.property(HiveCatalog.HMS_TABLE_OWNER, System.getProperty("user.name")),
- (int) currentTimeMillis / 1000,
- (int) currentTimeMillis / 1000,
- Integer.MAX_VALUE,
- null,
- Collections.emptyList(),
- Maps.newHashMap(),
- null,
- null,
- TableType.EXTERNAL_TABLE.toString());
+ Table newTable;
+ try {
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 #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1041491569
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -567,8 +569,13 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
});
if (database.getOwnerName() == null) {
- database.setOwnerName(System.getProperty("user.name"));
- database.setOwnerType(PrincipalType.USER);
+ try {
+ database.setOwnerName(UserGroupInformation.getCurrentUser().getUserName());
+ database.setOwnerType(PrincipalType.USER);
+ } catch (IOException e) {
+ throw new RuntimeException(
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 #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1041502713
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -470,6 +481,22 @@ private void setHmsTableParameters(
// remove any props from HMS that are no longer present in Iceberg table props
obsoleteProps.forEach(parameters::remove);
+ if (metadata.property(HiveCatalog.HMS_TABLE_OWNER, null) != null) {
Review Comment:
I put it there cause I was thinking to unify the ownership behavior 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] rdblue commented on pull request #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#issuecomment-1387440358
Thanks, @haizhou-zhao!
--
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 #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1053646116
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -246,23 +248,33 @@ public void testReplaceTxnBuilder() throws Exception {
}
@Test
- 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(HiveCatalog.HMS_TABLE_OWNER, owner);
+ public void testCreateTableWithOwner() throws IOException {
+ createTableAndVerifyOwner(
+ DB_NAME,
+ "tbl_specified_owner",
+ ImmutableMap.of(HiveCatalog.HMS_TABLE_OWNER, "some_owner"),
+ "some_owner");
+ createTableAndVerifyOwner(
+ DB_NAME,
+ "tbl_default_owner",
+ ImmutableMap.of(),
+ UserGroupInformation.getCurrentUser().getUserName());
+ }
+ private void createTableAndVerifyOwner(
+ String db, String tbl, Map<String, String> prop, String expectedOwner) {
try {
- Table table = catalog.createTable(tableIdent, schema, spec, location, properties);
- org.apache.hadoop.hive.metastore.api.Table hmsTable =
- metastoreClient.getTable(DB_NAME, "tbl");
- Assert.assertEquals(owner, hmsTable.getOwner());
- Map<String, String> hmsTableParams = hmsTable.getParameters();
- Assert.assertFalse(hmsTableParams.containsKey(HiveCatalog.HMS_TABLE_OWNER));
+ Schema schema = getTestSchema();
+ PartitionSpec spec = PartitionSpec.builderFor(schema).bucket("data", 16).build();
+ String location = temp.newFolder(tbl).toString();
+ catalog.createTable(TableIdentifier.of(db, tbl), schema, spec, location, prop);
+ org.apache.hadoop.hive.metastore.api.Table hmsTable = metastoreClient.getTable(db, tbl);
+ Assert.assertEquals(expectedOwner, hmsTable.getOwner());
+ Assert.assertFalse(hmsTable.getParameters().containsKey(HiveCatalog.HMS_TABLE_OWNER));
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 pull request #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#issuecomment-1332878641
@danielcweeks @gaborkaszab @szehon-ho ^
Feel free to invite other folks to 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 #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1067317866
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveHadoopUtil.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.hive;
+
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import org.apache.hadoop.security.UserGroupInformation;
+
+public class HiveHadoopUtil {
+
+ private HiveHadoopUtil() {}
+
+ public static String getUserName() {
+ try {
+ return UserGroupInformation.getCurrentUser().getUserName();
+ } catch (IOException e) {
+ throw new UncheckedIOException("Fail to obtain Hadoop UGI user", e);
Review Comment:
Falling back to os username in the newest 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 #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1053645840
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -567,8 +570,13 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
});
if (database.getOwnerName() == null) {
- database.setOwnerName(System.getProperty("user.name"));
- database.setOwnerType(PrincipalType.USER);
+ try {
+ database.setOwnerName(UserGroupInformation.getCurrentUser().getUserName());
+ database.setOwnerType(PrincipalType.USER);
+ } catch (IOException e) {
+ throw new UncheckedIOException(
+ String.format("Fail to obtain default (UGI) user for database %s", database), e);
Review Comment:
Adopted comments error message in the latest commit.
As for error throwing, I would argue that because the error is only thrown should the user decide to use kerberos principal to authenticate with their Hive Metastore cluster, the nature of the error is the user's fault (invalid keytab, etc.) and not related to iceberg. In that case, it is most straight forward to throw this error back at the user instead of having iceberg code swallowing exception and try to run default logic.
Also, the purpose of the change is the original implementation of using OS username as default table owner is not well integrated with Hive Metastore because Hive Metastore relies on Hadoop UGI for identity and authn.
--
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] rdblue commented on a diff in pull request #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1066391533
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveHadoopUtil.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.hive;
+
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import org.apache.hadoop.security.UserGroupInformation;
+
+public class HiveHadoopUtil {
+
+ private HiveHadoopUtil() {}
+
+ public static String getUserName() {
+ try {
+ return UserGroupInformation.getCurrentUser().getUserName();
+ } catch (IOException e) {
+ throw new UncheckedIOException("Fail to obtain Hadoop UGI user", e);
Review Comment:
In what cases is `IOException` thrown? Should this fall back to the old behavior and return `System.getProperty("user.name")` instead? I don't like that we have now introduced a new error case based on Hadoop internals.
--
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 #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
gaborkaszab commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1040783493
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -422,20 +424,29 @@ private Table newHmsTable(TableMetadata metadata) {
Preconditions.checkNotNull(metadata, "'metadata' parameter can't be null");
final long currentTimeMillis = System.currentTimeMillis();
- Table newTable =
- new Table(
- tableName,
- database,
- metadata.property(HiveCatalog.HMS_TABLE_OWNER, System.getProperty("user.name")),
- (int) currentTimeMillis / 1000,
- (int) currentTimeMillis / 1000,
- Integer.MAX_VALUE,
- null,
- Collections.emptyList(),
- Maps.newHashMap(),
- null,
- null,
- TableType.EXTERNAL_TABLE.toString());
+ Table newTable;
+ try {
Review Comment:
Might be just personal preference but I'd find it more readable not to put the while "new Table()" call into a try-catch block to cover UGI.getCurrentUser(). I'd rather construct the user name before calling this function. What do you think?
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -470,6 +481,22 @@ private void setHmsTableParameters(
// remove any props from HMS that are no longer present in Iceberg table props
obsoleteProps.forEach(parameters::remove);
+ if (metadata.property(HiveCatalog.HMS_TABLE_OWNER, null) != null) {
Review Comment:
Agree @szehon-ho that this seems a different use-case for me. Might worth covering it in a separate PR.
Additionally, this function is for setting the table parameters and with this change it would also have a side effect of setting the tbl's owner that is not really a table property. I would feel this is a bit off in my opinion.
And anyway, when this code was run for a "create table" scenario then newHmsTable() would have already set the table owner so the below line would be redundant.
But first of all as this is for the update scenario I'd say that this deserve a separate PR because this one is for changing the defaults during table creation.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -567,8 +569,13 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
});
if (database.getOwnerName() == null) {
- database.setOwnerName(System.getProperty("user.name"));
- database.setOwnerType(PrincipalType.USER);
+ try {
+ database.setOwnerName(UserGroupInformation.getCurrentUser().getUserName());
+ database.setOwnerType(PrincipalType.USER);
+ } catch (IOException e) {
+ throw new RuntimeException(
Review Comment:
+1 for UncheckedIOException.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -470,6 +481,22 @@ private void setHmsTableParameters(
// remove any props from HMS that are no longer present in Iceberg table props
obsoleteProps.forEach(parameters::remove);
+ if (metadata.property(HiveCatalog.HMS_TABLE_OWNER, null) != null) {
Review Comment:
I think this should be simpler:
`if (metadata.properties().containsKey(HiveCatalog.HMS_TABLE_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 pull request #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#issuecomment-1355443482
> > Hey folks, comments from last round of review all taken and implemented.
> > Specifically, on one comment: @gaborkaszab @szehon-ho I removed support for changing ownership for tables from this PR. But before I committed to create a follow up PR on that, I want to double check and make sure if you guys feel these worth implementing (which are something we do not have today):
> >
> > 1. When user remove ownership on a table, revert the owner of the table to default owner (current UGI)
> > 2. When user alter the ownership on a table, change the owner of table accordingly
>
> I think it does make sense to have a support for altering ownership. Impala has "ALTER TABLE xy SET OWNER" for this purpose, and I guess Hive also has something similar. Yes, the use-cases 1) and 2) you mention make sense for me.
Thank you @gaborkaszab , I create a follow up PR for those use cases.
--
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 #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1053645840
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -567,8 +570,13 @@ Database convertToDatabase(Namespace namespace, Map<String, String> meta) {
});
if (database.getOwnerName() == null) {
- database.setOwnerName(System.getProperty("user.name"));
- database.setOwnerType(PrincipalType.USER);
+ try {
+ database.setOwnerName(UserGroupInformation.getCurrentUser().getUserName());
+ database.setOwnerType(PrincipalType.USER);
+ } catch (IOException e) {
+ throw new UncheckedIOException(
+ String.format("Fail to obtain default (UGI) user for database %s", database), e);
Review Comment:
Agree on comments for error message. On the other point, I assume you are suggesting if we cannot obtain UGI identity, then we should use OS username like before. I'd like to put more discussion on it.
This PR is created because OS username (System.getProperty("user.name")) is not a good representation of who the owner of this process is when interacting with Hive Metastore (@danielcweeks @szehon-ho @gaborkaszab correct me if needed). Hive Metastore and its accompanying execution engines (HQL or Spark) usually recognize the Kerberos principal as identity, hence Hadoop's UGI is chosen over OS username.
When UGI fail to get the current identity, that means the user do not have the correct keytab or other credentials to authenticate with Kerberos server. If we fall back to OS username, then the outcome is we are swallowing a user issue, and setting the owner to some user unrecognizable by Hive ecosystem. If user didn't set up their Kerberos credentials properly, then I think it's the user's issue, and the cleanest way is to throw the error back at user, instead of swallowing the error and try to do something for user.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -422,11 +425,23 @@ private Table newHmsTable(TableMetadata metadata) {
Preconditions.checkNotNull(metadata, "'metadata' parameter can't be null");
final long currentTimeMillis = System.currentTimeMillis();
+ String owner = metadata.properties().get(HiveCatalog.HMS_TABLE_OWNER);
+ if (owner == null) {
+ try {
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 #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1067317621
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveHadoopUtil.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.hive;
+
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import org.apache.hadoop.security.UserGroupInformation;
+
+public class HiveHadoopUtil {
+
+ private HiveHadoopUtil() {}
+
+ public static String getUserName() {
Review Comment:
changed in the newest 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 #6324: Make UGI current user the default owner of Hive db&tbl
Posted by GitBox <gi...@apache.org>.
haizhou-zhao commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1041501636
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -470,6 +481,22 @@ private void setHmsTableParameters(
// remove any props from HMS that are no longer present in Iceberg table props
obsoleteProps.forEach(parameters::remove);
+ if (metadata.property(HiveCatalog.HMS_TABLE_OWNER, null) != null) {
Review Comment:
cool, I can separate it out into a separate PR. I hope to discuss whether this feature is needed. Basically, what I'm trying to achieve here is users can tweak ownership by setting iceberg table properties. And should users remove the owner properties, then it falls back to default owner.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -470,6 +481,22 @@ private void setHmsTableParameters(
// remove any props from HMS that are no longer present in Iceberg table props
obsoleteProps.forEach(parameters::remove);
+ if (metadata.property(HiveCatalog.HMS_TABLE_OWNER, null) != null) {
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