[SPARK-19970][SQL] Table owner should be USER instead of PRINCIPAL in kerberized clusters #17311

dongjoon-hyun · 2017-03-16T09:19:22Z

What changes were proposed in this pull request?

In the kerberized hadoop cluster, when Spark creates tables, the owner of tables are filled with PRINCIPAL strings instead of USER names. This is inconsistent with Hive and causes problems when using ROLE in Hive. We had better to fix this.

BEFORE

scala> sql("create table t(a int)").show
scala> sql("desc formatted t").show(false)
...
|Owner:                      |spark@EXAMPLE.COM                                         |       |

AFTER

scala> sql("create table t(a int)").show
scala> sql("desc formatted t").show(false)
...
|Owner:                      |spark                                         |       |

How was this patch tested?

Manually do create table and desc formatted because this happens in Kerberized clusters.

… kerberized clusters

dongjoon-hyun · 2017-03-16T09:31:36Z

Hi, @vanzin .
Could you review this PR?

SparkQA · 2017-03-16T10:09:41Z

Test build #74657 has finished for PR 17311 at commit 9dfbf82.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2017-03-16T10:26:25Z

Oh, there exists a version issue. Let me fix that.

dongjoon-hyun · 2017-03-16T10:41:06Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala

    }
    hiveTable.setPartCols(partCols.asJava)
-    conf.foreach(c => hiveTable.setOwner(c.getUser))
+    conf.foreach(c => hiveTable.setOwner(SessionState.getUserFromAuthenticator()))


getUserFromAuthenticator was added 0.13.

SparkQA · 2017-03-16T12:30:40Z

Test build #74664 has finished for PR 17311 at commit e8ef7d1.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2017-03-17T16:04:39Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala

    }
    hiveTable.setPartCols(partCols.asJava)
-    conf.foreach(c => hiveTable.setOwner(c.getUser))
+    conf.foreach(c => hiveTable.setOwner(SessionState.get().getAuthenticator().getUserName()))


We cannot use the following since it was introduced in 0.13.

conf.foreach(c => hiveTable.setOwner(SessionState.getUserFromAuthenticator()))

Since you're touching this, could you make it follow the usual style?

.foreach { c => ... }

Thank you for review, @vanzin !
Which object do you mean for .foreach here?
For conf, we already use that.
For SessionState.get(), it's not Option.

There's only one foreach call and it's using a different style than the rest of the code.

Oh, I see. It's about removing .foreach.

No, it's about style. Parentheses v. braces.

Ah. Sorry again.

dongjoon-hyun · 2017-03-17T16:05:32Z

Hi, @gatorsmile .
Could you review this issue?

dongjoon-hyun · 2017-03-17T17:26:37Z

I fixed that, @vanzin . Thank you again.

dongjoon-hyun · 2017-03-17T17:32:53Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala

    }
    hiveTable.setPartCols(partCols.asJava)
-    conf.foreach(c => hiveTable.setOwner(c.getUser))
+    conf.foreach { _ => hiveTable.setOwner(SessionState.get().getAuthenticator().getUserName()) }


Oh, I think we can remove conf here.

Ah, yes, you're not using it anymore. So away it goes.

@vanzin . Some test cases seems to related with conf is None, it causes failures. In this PR, I'll focus on putting the correct name only with the exactly same situations of the previous existing logic.

I think we should pass the user name.

SparkQA · 2017-03-17T18:06:28Z

Test build #74750 has finished for PR 17311 at commit 5efdf8b.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-03-17T18:56:45Z

Test build #74748 has finished for PR 17311 at commit 2959d98.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

This reverts commit 5efdf8b.

SparkQA · 2017-03-17T20:36:02Z

Test build #74753 has finished for PR 17311 at commit 258ff8d.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2017-03-18T06:58:36Z

If there is anything to do more, please let me know.

dongjoon-hyun · 2017-03-18T20:11:20Z

Hi, @vanzin and @srowen .
Could you review this PR (again) when you have sometime?
I feel guilty because this PR need to be verified by manually on kerberized clusters.

dongjoon-hyun · 2017-03-20T10:15:16Z

Retest this please

SparkQA · 2017-03-20T12:43:20Z

Test build #74877 has finished for PR 17311 at commit 258ff8d.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2017-03-20T16:53:30Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala

    }
    hiveTable.setPartCols(partCols.asJava)
-    conf.foreach(c => hiveTable.setOwner(c.getUser))
+    conf.foreach { _ => hiveTable.setOwner(SessionState.get().getAuthenticator().getUserName()) }


Shouldn't you be using the state field in the class instead of SessionState.get()? Maybe that will allow you to get rid of the foreach.

state is in class HiveClientImpl, but this function is in object HiveClientImpl.

Ahh. Now it makes sense why conf would be None.

vanzin · 2017-03-20T17:06:54Z

LGTM, merging to master / 2.1.

dongjoon-hyun · 2017-03-20T17:08:03Z

Thank you so much, @vanzin !

vanzin · 2017-03-20T17:08:26Z

Couldn't merge to 2.1.

dongjoon-hyun · 2017-03-20T17:09:58Z

Oh, I'll make a backport then.

dongjoon-hyun · 2017-03-20T17:19:26Z

The difference is due to 3881f34#diff-6fd847124f8eae45ba2de1cf7d6296feR855 .

…RINCIPAL in kerberized clusters apache#17311 ### What changes were proposed in this pull request? This is a follow-up for the PR: apache#17311 - For safety, use `sessionState` to get the user name, instead of calling `SessionState.get()` in the function `toHiveTable`. - Passing `user names` instead of `conf` when calling `toHiveTable`. ### How was this patch tested? N/A Author: Xiao Li <gatorsmile@gmail.com> Closes apache#17405 from gatorsmile/user.

[SPARK-19970][SQL] Table owner should be USER instead of PRINCIPAL in…

9dfbf82

… kerberized clusters

dongjoon-hyun commented Mar 16, 2017

View reviewed changes

Replace getUserFromAuthenticator because it is introduced in 0.13.

e8ef7d1

dongjoon-hyun commented Mar 17, 2017

View reviewed changes

Change styles.

2959d98

dongjoon-hyun commented Mar 17, 2017

View reviewed changes

conf is not used.

5efdf8b

Revert "conf is not used."

258ff8d

This reverts commit 5efdf8b.

vanzin reviewed Mar 20, 2017

View reviewed changes

asfgit closed this in fc75545 Mar 20, 2017

dongjoon-hyun mentioned this pull request Mar 20, 2017

[SPARK-19970][SQL][BRANCH-2.1] Table owner should be USER instead of PRINCIPAL in kerberized clusters #17363

Closed

gatorsmile mentioned this pull request Mar 24, 2017

[SPARK-19970][SQL][Follow-up] Table owner should be USER instead of PRINCIPAL in kerberized clusters #17311 #17405

Closed

dongjoon-hyun deleted the SPARK-19970 branch January 7, 2019 07:03

HeartSaVioR mentioned this pull request Sep 6, 2019

[SPARK-26929][SQL]fix table owner use user instead of principal when create table through spark-sql or beeline #23952

Closed

[SPARK-19970][SQL] Table owner should be USER instead of PRINCIPAL in kerberized clusters #17311

[SPARK-19970][SQL] Table owner should be USER instead of PRINCIPAL in kerberized clusters #17311

Uh oh!

Conversation

dongjoon-hyun commented Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

dongjoon-hyun commented Mar 16, 2017

Uh oh!

SparkQA commented Mar 16, 2017

Uh oh!

dongjoon-hyun commented Mar 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Mar 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Mar 17, 2017

Uh oh!

dongjoon-hyun commented Mar 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Mar 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 17, 2017

Uh oh!

SparkQA commented Mar 17, 2017

Uh oh!

SparkQA commented Mar 17, 2017

Uh oh!

dongjoon-hyun commented Mar 18, 2017

Uh oh!

dongjoon-hyun commented Mar 18, 2017

Uh oh!

dongjoon-hyun commented Mar 20, 2017

Uh oh!

SparkQA commented Mar 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vanzin commented Mar 20, 2017

Uh oh!

dongjoon-hyun commented Mar 20, 2017

Uh oh!

vanzin commented Mar 20, 2017

Uh oh!

dongjoon-hyun commented Mar 20, 2017

Uh oh!

dongjoon-hyun commented Mar 20, 2017

dongjoon-hyun commented Mar 16, 2017 •

edited

Loading

dongjoon-hyun Mar 17, 2017 •

edited

Loading

dongjoon-hyun Mar 17, 2017 •

edited

Loading