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

HeartSaVioR · 2019-09-05T12:32:18Z

What changes were proposed in this pull request?

This patch proposes to add new tests to test the username of HiveClient to prevent changing the semantic unintentionally. The owner of Hive table has been changed back-and-forth, principal -> username -> principal, and looks like the change is not intentional. (Please refer SPARK-28996 for more details.) This patch intends to prevent this.

This patch also renames previous HiveClientSuite(s) to HivePartitionFilteringSuite(s) as it was commented as TODO, as well as previous tests are too narrowed to test only partition filtering.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Newly added UTs.

HeartSaVioR · 2019-09-05T12:42:24Z

Looks like renaming is not recognized though git status showed it correctly. (Shouldn't I reuse the name of HiveClientSuite?) I'll try to play with this if someone concerns about the excessive lines of change.

SparkQA · 2019-09-05T14:41:25Z

Test build #110182 has finished for PR 25696 at commit 8c1efd3.

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

HeartSaVioR · 2019-09-06T03:11:42Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HivePartitionFilteringSuite.scala

@@ -0,0 +1,341 @@
+/*


This is just a renaming, though Github doesn't seem to recognize it.

HeartSaVioR · 2019-09-06T03:11:51Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HivePartitionFilteringSuites.scala

@@ -0,0 +1,29 @@
+/*


Same: This is just a renaming, though Github doesn't seem to recognize it.

HeartSaVioR · 2019-09-06T03:34:00Z

cc. @cloud-fan @gatorsmile @dongjoon-hyun

I'm also asking to review #23952 in pair, as I feel #23952 is the right way but that should have test. I'm OK to rebase this to reflect #23952 if we would like to merge #23952 before this.

SparkQA · 2019-09-06T05:22:52Z

Test build #110215 has finished for PR 25696 at commit 0bdc71a.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class HiveClientUserNameSuite(version: String)
class HiveClientUserNameSuites extends Suite with HiveClientVersions

squito · 2019-09-09T15:25:49Z

shouldn't this and #23952 be put together and merged together as one PR?

HeartSaVioR · 2019-09-09T18:28:48Z

There're some reasons to make this be separated PR:

I suggested adding UT in [SPARK-26929][SQL]fix table owner use user instead of principal when create table through spark-sql or beeline #23952 before, and the author didn't follow the suggestion so I had to put my own effort to deal with this. If I needed to raise the PR against author's fork, I should also persuade to merge it to reflect the PR, which seems to be unnecessary effort (and in my personal experience it has been progressed slowly on coordinating with other contributors... I've been also waiting for other contributor in [WIP][CORE][SPARK-28867] InMemoryStore checkpoint to speed up replay log file in HistoryServer #25577 to coordinate). If this patch got merged before, [SPARK-26929][SQL]fix table owner use user instead of principal when create table through spark-sql or beeline #23952 will make new tests be broken so [SPARK-26929][SQL]fix table owner use user instead of principal when create table through spark-sql or beeline #23952 is forced to apply the change. If this patch will be planned to be merged after [SPARK-26929][SQL]fix table owner use user instead of principal when create table through spark-sql or beeline #23952, I'm open to apply the change.
This patch can be independent with [SPARK-26929][SQL]fix table owner use user instead of principal when create table through spark-sql or beeline #23952, as the new tests are passing with current behavior. This patch is still valuable without [SPARK-26929][SQL]fix table owner use user instead of principal when create table through spark-sql or beeline #23952 as we should have to have tests to prevent changing behavior unintentionally. We didn't notice the behavioral change in latest change of SPARK-22846 - we thought it fixed NPE but it also changed the behavior. This is a non-trivial issue.
Minor, but authorship issue. IMHO I'm not sure who is the right main author of merged PR, as I expect there's similar amount of effort being put between [SPARK-26929][SQL]fix table owner use user instead of principal when create table through spark-sql or beeline #23952 and this.

vanzin · 2019-09-16T18:16:43Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HiveClientUserNameSuite.scala

+      "[email protected]", ugi, Array.empty)
+    proxyUgi.doAs(new PrivilegedExceptionAction[Unit]() {
+      override def run(): Unit = {
+        assert(proxyUgi.getUserName === getUserNameFromHiveClient)


This test seems wrong for that the bug is about. proxyUgi.getUserName will return [email protected], and isn't the bug about it needing to be only proxyprincipal to match Hive's behavior?

Before SPARK-26929 it was "[email protected]" - as test passed. So setting HADOOP_USER_NAME and proxy user being set provided different results. I couldn't add test for HADOOP_USER_NAME as artificially setting system env. is hard.

Anyway SPARK-26929 removes the case and makes tests here failing (intended). I'll reflect the change.

SparkQA · 2019-09-16T22:55:25Z

Test build #110663 has finished for PR 25696 at commit dfca82b.

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

HeartSaVioR · 2019-09-16T23:19:12Z

Reflected the change of SPARK-26929 and test passed. Could we have a next round of review?

cloud-fan · 2019-09-17T02:11:52Z

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

  }

-  private val userName = UserGroupInformation.getCurrentUser.getShortUserName
+  override val userName = UserGroupInformation.getCurrentUser.getShortUserName


Can we change user name at runtime? If we can then it should be def instead of val.

I'm not 100% sure, but we have been defining userName as val from Spark 2.2.0, starting from this commit 344f38b

Changing it to def is safer anyway (can handle both cases - whether user name is changed or not) so please let me know if we would like to make change.

Btw, that would be better to be handled via separate PR if necessary, to isolate two different things "adding test" and "fixing logic".

yea, we can investigate it in a separated pr

cloud-fan · 2019-09-17T02:25:29Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HiveClientUserNameSuite.scala

+import org.apache.spark.util.Utils
+
+class HiveClientUserNameSuite(version: String)
+    extends HiveVersionSuite(version) with BeforeAndAfterAll {


BeforeAndAfterAll is not used

Nice finding! Updated.

cloud-fan · 2019-09-17T02:28:38Z

LGTM

SparkQA · 2019-09-17T05:09:35Z

Test build #110719 has finished for PR 25696 at commit ef220ef.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class HiveClientUserNameSuite(version: String) extends HiveVersionSuite(version)

cloud-fan · 2019-09-17T06:04:40Z

thanks, merging to master!

HeartSaVioR · 2019-09-17T06:32:36Z

Thanks all for reviewing and merging!

HeartSaVioR mentioned this pull request Sep 5, 2019

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

Closed

dongjoon-hyun added the SQL label Sep 5, 2019

HeartSaVioR commented Sep 6, 2019

View reviewed changes

vanzin reviewed Sep 16, 2019

View reviewed changes

HeartSaVioR added 4 commits September 17, 2019 04:14

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

4d5a389

slight modification of test name

a98c0b9

Renaming to help Github to indicate renaming

1da9bd8

Reflect the change of SPARK-26929

dfca82b

HeartSaVioR force-pushed the SPARK-28996 branch from 0bdc71a to dfca82b Compare September 16, 2019 19:27

HeartSaVioR changed the title ~~[SPARK-28996][SQL] Add tests regarding username of HiveClient~~ [SPARK-28996][SQL][TESTS] Add tests regarding username of HiveClient Sep 16, 2019

cloud-fan reviewed Sep 17, 2019

View reviewed changes

Reflect review comment

ef220ef

cloud-fan approved these changes Sep 17, 2019

View reviewed changes

cloud-fan closed this in c862835 Sep 17, 2019

HeartSaVioR deleted the SPARK-28996 branch September 17, 2019 06:32

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

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

Uh oh!

Conversation

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

Uh oh!

What changes were proposed in this pull request?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

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

Uh oh!

Uh oh!

SparkQA commented Sep 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR commented Sep 6, 2019

Uh oh!

SparkQA commented Sep 6, 2019

Uh oh!

squito commented Sep 9, 2019

Uh oh!

HeartSaVioR commented Sep 9, 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!

SparkQA commented Sep 16, 2019

Uh oh!

HeartSaVioR commented Sep 16, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Sep 17, 2019

Uh oh!

SparkQA commented Sep 17, 2019

Uh oh!

cloud-fan commented Sep 17, 2019

Uh oh!

HeartSaVioR commented Sep 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

HeartSaVioR commented Sep 5, 2019 •

edited

Loading

HeartSaVioR commented Sep 5, 2019 •

edited

Loading

HeartSaVioR commented Sep 9, 2019 •

edited

Loading