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

hddong · 2019-03-04T08:47:30Z

…create table through spark-sql or beeline

What changes were proposed in this pull request?

fix table owner use user instead of principal when create table through spark-sql
private val userName = conf.getUser will get ugi's userName which is principal info, and i copy the source code into HiveClientImpl, and use ugi.getShortUserName() instead of ugi.getUserName(). The owner display correctly.

How was this patch tested?

create a table in kerberos cluster
use "desc formatted tbName" check owner

Please review http://spark.apache.org/contributing.html before opening a pull request.

…create table through spark-sql or beeline

hddong · 2019-03-04T08:50:05Z

link to #23837
@HyukjinKwon , i created a new PR, please review here, thanks.

hddong · 2019-03-08T06:00:55Z

@HyukjinKwon @felixcheung @vanzin , could you help to review this?

felixcheung · 2019-03-10T21:32:35Z

I think it’s best you find someone on the SQL side to review this.

HyukjinKwon · 2019-03-13T11:37:23Z

cc @dongjoon-hyun FYI.

hddong · 2019-03-17T01:04:41Z

@dongjoon-hyun could you help to review this?

hddong · 2019-03-20T07:07:42Z

@rxin , @cloud-fan could you help to review?

cloud-fan · 2019-03-21T20:36:01Z

ok to test

cloud-fan · 2019-03-21T20:37:44Z

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

-  private val userName = conf.getUser
+  private val userName: String = try {
+    val ugi = HiveUtils.getUGI
+    ugi.getShortUserName


How do we prove this is the right fix? What if there is no UGI?

@cloud-fan , It will return system user information if there is no ugi. Actually, the source code of conf.getUser is:

public String getUser() throws IOException { try { UserGroupInformation ugi = Utils.getUGI(); return ugi.getUserName(); } catch (LoginException var2) { throw new IOException(var2); } }

this change just use ugi.getShortUserName instead of ugi.getUserName()

Looks like Utils is accessible from here, so if we just want to leverage Hive code and call getShortUserName instead, we can just do it with one-liner.

private val userName = org.apache.hadoop.hive.shims.Utils.getUGI.getShortUserName

Here I intended to not catch LoginException as we don't do anything but throw it.

org.apache.hadoop.hive.shims.Utils.getUGI.getShortUserName

Yep, but shims.Utils is not compatible with hive0.*, and cannot pass all tests. This file is added after hive1.*, so, copy the source code here.

Ah OK. Didn't recognize it doesn't match with some versions. Makes sense to me.

f1eb8cf was for this, sorry I had to track down commits as well.

SparkQA · 2019-03-21T20:46:01Z

Test build #103790 has finished for PR 23952 at commit 2609c9c.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

hddong · 2019-03-26T02:42:54Z

@cloud-fan, how to fix error 'fails Scala style tests ', should i move code to function? Could you give some suggest.

hddong · 2019-04-17T01:36:20Z

cc @rxin , @cloud-fan, @dongjoon-hyun

Modify exception

SparkQA · 2019-07-25T06:09:54Z

Test build #108151 has finished for PR 23952 at commit 05e54fe.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

AngersZhuuuu · 2019-07-25T06:25:35Z

Only fix table owner is not enough. we should create table will the right user not just change table owner

hddong · 2019-07-25T06:47:03Z

Only fix table owner is not enough. we should create table will the right user not just change table owner

Just owner display error information, other is correct.

SparkQA · 2019-07-25T06:51:27Z

Test build #108153 has finished for PR 23952 at commit bdcc9fa.

This patch fails Scala style tests.
This patch does not merge cleanly.
This patch adds no public classes.

SparkQA · 2019-07-25T06:59:54Z

Test build #108154 has finished for PR 23952 at commit c4e3468.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-07-25T07:22:33Z

Test build #108158 has finished for PR 23952 at commit e8c0571.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

conflict of hiveUtils

SparkQA · 2019-07-25T09:07:17Z

Test build #108161 has finished for PR 23952 at commit c27d4af.

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

SparkQA · 2019-07-26T04:04:10Z

Test build #108189 has finished for PR 23952 at commit f1eb8cf.

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

hddong · 2019-07-26T05:47:37Z

cc @rxin , @cloud-fan, @dongjoon-hyun

hddong · 2019-08-01T08:25:35Z

@rxin , @cloud-fan, @dongjoon-hyun could you help to review?

HeartSaVioR · 2019-09-05T12:38:10Z

I'm addressing test part as #25696 - I didn't raise a PR against this PR as the change was more than what I expected. This patch can reflect the change (change to shortUserName) once #25696 is merged.

HeartSaVioR · 2019-09-06T03:36:14Z

FYI, #17311 explained why this should be applied.

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.

Let me try to ping again cc. @cloud-fan @gatorsmile @dongjoon-hyun

vanzin · 2019-09-09T21:36:53Z

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

+    }
+    ugi.getShortUserName
+  } catch {
+    case _: LoginException => throw new LoginException("Can not get login user.")


What's the purpose of this? Just let the original exception propagate.

According to the previous comment, I guess the origin code was IOException wrapped LoginException:
https://github.com/apache/spark/pull/23952/files#r268466632

so if we want to follow the origin code, use IOException, otherwise, just remove try/catch.

@vanzin @HeartSaVioR
In hive source code, it throw two exception: LoginException, IOException. I wonder if Exception is ok here.

I guess things will fail with logging exception in caller site, so I guess the recommendation is just removing try/catch altogether.

Hive source code has to do something (like wrapping as you've seen) because both LoginException and IOException are checked exceptions - given you're coding in Scala you can forget about that.

I guess things will fail with logging exception in caller site, so I guess the recommendation is just removing try/catch altogether.

Hive source code has to do something (like wrapping as you've seen) because both LoginException and IOException are checked exceptions - given you're coding in Scala you can forget about that.

Yes, you are right, it will fail with logging exception. Changed it.

SparkQA · 2019-09-10T07:05:01Z

Test build #110401 has finished for PR 23952 at commit 6011598.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

HeartSaVioR · 2019-09-10T09:46:37Z

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

  } catch {
    case e: Exception =>
-      logWarning("Can not get login user.")
+      logError("Can not get login user.")


Same comment here: I'm not sure we really want to log error message here, as I expect thrown exception will make query fail somewhere in caller side, and it will log the exception correctly - stack trace in exception will tell us.

SparkQA · 2019-09-10T10:30:35Z

Test build #110412 has finished for PR 23952 at commit 76192b2.

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

HeartSaVioR · 2019-09-10T10:55:53Z

retest this, please

SparkQA · 2019-09-10T12:41:41Z

Test build #110416 has finished for PR 23952 at commit 76192b2.

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

HeartSaVioR

LGTM

hddong · 2019-09-11T01:36:58Z

@HeartSaVioR
An unrelated question, why most tests failed, and passed all after your reset.

HeartSaVioR · 2019-09-11T01:41:00Z

Hive test seems to be a bit flaky. So the Spark community tries to fix the flay tests as many as possible, but there're many cases Spark community can't fix the flakiness. Rerunning build doesn't matter - if the test is failing consistently that's the matter.

hddong · 2019-09-11T02:03:02Z

Hive test seems to be a bit flaky. So the Spark community tries to fix the flay tests as many as possible, but there're many cases Spark community can't fix the flakiness. Rerunning build doesn't matter - if the test is failing consistently that's the matter.

Thanks.

SparkQA · 2019-09-11T03:15:52Z

Test build #110451 has finished for PR 23952 at commit a8d6b1d.

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

vanzin · 2019-09-11T17:41:47Z

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

-  private val userName = conf.getUser
+  private val userName = {
+    val doAs = sys.env.get("HADOOP_USER_NAME").orNull
+    val ugi = if (doAs != null && doAs.length() > 0) {


This is wrong or, at the very least, backwards: the current UGI should be preferred.

What's the goal of this code? Why can't you just get the current UGI's name?

Uh, at least that is not a regression, as conf.getUser calls Hive side and Hive does it in Utils.getUGI, at least from Hive 1.2.1.spark2.
So the code is copied and pasted to modify calling ugi.getShortUserName() instead of ugi.getUserName() - @hddong left a comment before to explain why the code had to be copied and pasted - #23952 (comment)

Right, and there's a huge comment in the Hive method (at least in the branch I'm looking at) that explains why that is done. And if you read that comment you'll see that it does not apply here.

In a way, the HMS API is broken in that it lets the caller set the owner (instead of getting it from the auth info).

But we really should at least try to get the correct information through, and that comes from the UGI, not from env variables.

Yeah actually I just read decompiled code of Utils.getUGI() (missed to read comment in source) and didn't indicate the intention - let other application be able to pass it. Yes I agree it's not needed in Spark side, and it would be weird HADOOP_USER_NAME is only used here and undocumented. Thanks for explaining!

Thanks for you two, I missed to read comment in source too. Yes, current UGI is ok here, SPARK-22846 Fix table owner is null when creating table through spark sql or thriftserver, but lead to an issue that have occurred(owner is principal in kerberized cluster).

SparkQA · 2019-09-12T03:32:04Z

Test build #110496 has finished for PR 23952 at commit 2f37513.

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

HeartSaVioR

LGTM

vanzin · 2019-09-16T18:07:24Z

Merging to master.

AmplabJenkins · 2019-09-16T18:15:36Z

Can one of the admins verify this patch?

HeartSaVioR · 2019-09-16T19:37:37Z

Thanks again @hddong for making this patch - pretty useful.

[SPARK-26929][SQL]fix table owner use user instead of principal when …

2609c9c

…create table through spark-sql or beeline

hddong mentioned this pull request Mar 4, 2019

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

Closed

cloud-fan reviewed Mar 21, 2019

View reviewed changes

dongjoon-hyun added the SQL label Jun 14, 2019

Update HiveClientImpl.scala

05e54fe

Modify exception

Update HiveClientImpl.scala

bdcc9fa

Merge branch 'master' into SPARK-26929-fix-table-owner

c4e3468

Update HiveClientImpl.scala

e8c0571

Update HiveClientImpl.scala

c27d4af

conflict of hiveUtils

Update HiveClientImpl.scala

f1eb8cf

HeartSaVioR mentioned this pull request Sep 6, 2019

[SPARK-28996][SQL][TESTS] Add tests regarding username of HiveClient #25696

Closed

vanzin reviewed Sep 9, 2019

View reviewed changes

hddong added 2 commits September 10, 2019 13:48

Update HiveClientImpl.scala

0c7ddb1

Update HiveClientImpl.scala

6011598

Update HiveClientImpl.scala

76192b2

HeartSaVioR reviewed Sep 10, 2019

View reviewed changes

Update HiveClientImpl.scala

a8d6b1d

HeartSaVioR approved these changes Sep 11, 2019

View reviewed changes

vanzin reviewed Sep 11, 2019

View reviewed changes

Update HiveClientImpl.scala

2f37513

HeartSaVioR approved these changes Sep 14, 2019

View reviewed changes

vanzin closed this in 5881871 Sep 16, 2019

Uh oh!

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

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

Uh oh!

Conversation

hddong commented Mar 4, 2019

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

hddong commented Mar 4, 2019

Uh oh!

hddong commented Mar 8, 2019

Uh oh!

felixcheung commented Mar 10, 2019 via email

Uh oh!

HyukjinKwon commented Mar 13, 2019

Uh oh!

hddong commented Mar 17, 2019

Uh oh!

hddong commented Mar 20, 2019

Uh oh!

cloud-fan commented Mar 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hddong Mar 25, 2019 • 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!

HeartSaVioR Aug 23, 2019 • 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!

hddong Aug 26, 2019 • 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!

SparkQA commented Mar 21, 2019

Uh oh!

hddong commented Mar 26, 2019

Uh oh!

hddong commented Apr 17, 2019

Uh oh!

SparkQA commented Jul 25, 2019

Uh oh!

AngersZhuuuu commented Jul 25, 2019

Uh oh!

hddong commented Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Jul 25, 2019

Uh oh!

SparkQA commented Jul 25, 2019

Uh oh!

SparkQA commented Jul 25, 2019

Uh oh!

SparkQA commented Jul 25, 2019

Uh oh!

SparkQA commented Jul 26, 2019

Uh oh!

hddong commented Jul 26, 2019

Uh oh!

hddong commented Aug 1, 2019

Uh oh!

HeartSaVioR commented Sep 5, 2019

Uh oh!

HeartSaVioR commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

HeartSaVioR Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

hddong Mar 25, 2019 •

edited

Loading

HeartSaVioR Aug 23, 2019 •

edited

Loading

hddong Aug 26, 2019 •

edited

Loading

hddong commented Jul 25, 2019 •

edited

Loading

HeartSaVioR commented Sep 6, 2019 •

edited

Loading

HeartSaVioR Sep 10, 2019 •

edited

Loading

HeartSaVioR Sep 11, 2019 •

edited

Loading