[SPARK-28527][SQL][TEST] Enable ThriftServerQueryTestSuite #25868

wangyum · 2019-09-20T10:35:02Z

What changes were proposed in this pull request?

This PR enable ThriftServerQueryTestSuite and fix previously flaky test by:

Start thriftserver in beforeAll().
Disable spark.sql.hive.thriftServer.async.

Why are the changes needed?

Improve test coverage.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

build/sbt "hive-thriftserver/test-only *.ThriftServerQueryTestSuite "  -Phive-thriftserver
build/mvn -Dtest=none -DwildcardSuites=org.apache.spark.sql.hive.thriftserver.ThriftServerQueryTestSuite test  -Phive-thriftserver

SparkQA · 2019-09-20T11:17:03Z

Test build #111064 has finished for PR 25868 at commit 2a422ea.

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

wangyum · 2019-09-20T11:35:18Z

retest this please

SparkQA · 2019-09-20T12:12:33Z

Test build #111066 has finished for PR 25868 at commit 2a422ea.

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

HyukjinKwon · 2019-09-20T14:47:37Z

cc @zsxwing

SparkQA · 2019-09-22T07:05:02Z

Test build #111144 has finished for PR 25868 at commit 0b75d5b.

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

SparkQA · 2019-09-22T12:03:16Z

Test build #111146 has finished for PR 25868 at commit 1fede27.

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

wangyum · 2019-09-22T13:07:46Z

retest this please

SparkQA · 2019-09-22T15:50:31Z

Test build #111160 has finished for PR 25868 at commit eeda699.

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

wangyum · 2019-09-22T15:51:59Z

retest this please

wangyum · 2019-09-22T16:43:50Z

retest this please

wangyum · 2019-09-22T16:47:15Z

retest this please

SparkQA · 2019-09-22T17:02:18Z

Test build #111155 has finished for PR 25868 at commit 1fede27.

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

SparkQA · 2019-09-22T17:17:37Z

Test build #111162 has finished for PR 25868 at commit eeda699.

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

SparkQA · 2019-09-22T21:16:14Z

Test build #111161 has finished for PR 25868 at commit eeda699.

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

wangyum · 2019-09-22T23:05:48Z

retest this please

wangyum · 2019-09-22T23:15:32Z

retest this please

SparkQA · 2019-09-22T23:38:58Z

Test build #111169 has finished for PR 25868 at commit eeda699.

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

SparkQA · 2019-09-22T23:47:32Z

Test build #111170 has finished for PR 25868 at commit eeda699.

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

wangyum · 2019-09-22T23:49:54Z

retest this please

wangyum · 2019-09-22T23:49:57Z

retest this please

wangyum · 2019-09-22T23:50:02Z

retest this please

SparkQA · 2019-09-23T00:22:44Z

Test build #111174 has finished for PR 25868 at commit eeda699.

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

SparkQA · 2019-09-23T02:26:57Z

Test build #111179 has finished for PR 25868 at commit 2eac612.

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

wangyum · 2019-09-23T02:46:21Z

retest this please

SparkQA · 2019-09-23T03:18:32Z

Test build #111183 has finished for PR 25868 at commit 2eac612.

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

wangyum · 2019-09-23T03:22:21Z

retest this please

SparkQA · 2019-09-23T03:54:46Z

Test build #111184 has finished for PR 25868 at commit 2eac612.

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

wangyum · 2019-09-23T05:05:14Z

retest this please

SparkQA · 2019-09-23T05:41:29Z

Test build #111186 has finished for PR 25868 at commit 2eac612.

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

wangyum · 2019-09-23T15:41:21Z

retest this please

SparkQA · 2019-09-23T16:24:48Z

Test build #111234 has finished for PR 25868 at commit 2eac612.

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

dongjoon-hyun · 2019-09-23T23:16:02Z

So, this adds 9 min 35 seconds additionally?

wangyum · 2019-09-24T01:56:35Z

Yes. It will add 9 min 35 seconds for maven testing.

dongjoon-hyun

+1, LGTM.

dongjoon-hyun · 2019-09-24T04:24:12Z

Thank you, @wangyum . Please merge this after PST midnight to avoid Jenkins reset.

wangyum · 2019-09-24T07:45:56Z

Merged to master.

wangyum · 2019-09-24T07:46:54Z

Thank you @HyukjinKwon @dongjoon-hyun

zsxwing · 2019-09-24T09:08:54Z

...erver/src/test/scala/org/apache/spark/sql/hive/thriftserver/ThriftServerQueryTestSuite.scala

+  override def sparkConf: SparkConf = super.sparkConf
+    // Hive Thrift server should not executes SQL queries in an asynchronous way
+    // because we may set session configuration.
+    .set(HiveUtils.HIVE_THRIFT_SERVER_ASYNC, false)


Does this mean this is broken and the user should also turn if off? If so, should we change the default value? Otherwise, our tests are actually testing something that's rarely used.

Hive Thrift server should not executes SQL queries in an asynchronous way because we may set session configuration.

Could you clarify what's the exact issue? Is it because the background thread is missing some thread-local variables because threads are reused? Can we copy them from the parent thread here? https://github.com/apache/spark/blob/master/sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkExecuteStatementOperation.scala#L186

Ur, this suite is not aiming concurrency stress test. It's just targeting SQL execution one by one.

But it exposes some bugs in the default mode. Seems worth to fix the bug.

For me, that doesn't imply a bug in the default mode. That means we want to run one by one simply in this test suite.

Of course, if there is a bug, definitely we should fix it. But, let's not enable that in this test suite. That is completely a separate issue, isn't it? With a separate UT and a patch, that should be handled.

With HiveUtils.HIVE_THRIFT_SERVER_ASYNC enabled the Thriftserver will still execute queries one by one. The difference is that it will not block the request:

With HIVE_THRIFT_SERVER_ASYNC=false client sends a query in an TExecuteStatementReq. The query executes, and only after it finishes the server responds with a TExecuteStatementResp. Then the client calls TGetOperationStatusReq to see if the result was a success or failure, and then potentially continues fetching results...

With HIVE_THRIFT_SERVER_ASYNC=true client sends a query in an TExecuteStatementReq, and the server starts it in a background thread and immediately returns a handle in the response. Then the client periodically polls with TGetOperationStatusReq until the query is finished, an then potentailly continues fetching results...

In both cases, the Hive JDBC driver executes one query at once and there is no concurrency.

I think this setting does not need to be set.

juliuszsompolski

@wangyum late post-review as I only now noticed this PR.

juliuszsompolski · 2019-10-18T15:26:02Z

...erver/src/test/scala/org/apache/spark/sql/hive/thriftserver/ThriftServerQueryTestSuite.scala

+  override def sparkConf: SparkConf = super.sparkConf
+    // Hive Thrift server should not executes SQL queries in an asynchronous way
+    // because we may set session configuration.
+    .set(HiveUtils.HIVE_THRIFT_SERVER_ASYNC, false)


With HiveUtils.HIVE_THRIFT_SERVER_ASYNC enabled the Thriftserver will still execute queries one by one. The difference is that it will not block the request:

With HIVE_THRIFT_SERVER_ASYNC=false client sends a query in an TExecuteStatementReq. The query executes, and only after it finishes the server responds with a TExecuteStatementResp. Then the client calls TGetOperationStatusReq to see if the result was a success or failure, and then potentially continues fetching results...

With HIVE_THRIFT_SERVER_ASYNC=true client sends a query in an TExecuteStatementReq, and the server starts it in a background thread and immediately returns a handle in the response. Then the client periodically polls with TGetOperationStatusReq until the query is finished, an then potentailly continues fetching results...

In both cases, the Hive JDBC driver executes one query at once and there is no concurrency.

I think this setting does not need to be set.

juliuszsompolski · 2019-10-18T15:34:17Z

...erver/src/test/scala/org/apache/spark/sql/hive/thriftserver/ThriftServerQueryTestSuite.scala


-  override def beforeEach(): Unit = {
+  override def beforeAll(): Unit = {
+    super.beforeAll()


In an earlier PR I commented that the flakiness may be because of async issues.
I meant that calling startThriftServer actually starts some things asynchronously in the background before the server becomes ready.
Moving it to beforeAll instead of beforeEach should help the flakiness by having this race only at the start of the suite and not before every test, but I think adding a 3 or 5 s sleep would make sure it never happens.

I seem to be hitting it in the first test of the suite quite often:

[info] ThriftServerQueryTestSuite: [info] - query_regex_column.sql *** FAILED *** (184 milliseconds) [info] java.sql.SQLException: Could not open client transport with JDBC Uri: jdbc:hive2://localhost:15421: java.net.ConnectException: Connection refused (Connection refused)

pgSQL/text.sql should fail if spark.sql.hive.thriftServer.async is enabled: #25567 (comment)

Anyway, let's try to enable it: #26172

Enable SPARK-28527

2a422ea

dongjoon-hyun added the SQL label Sep 20, 2019

wangyum added 2 commits September 22, 2019 13:28

fix test error

0b75d5b

Enable more tests

1fede27

Move spark.sql.hive.thriftServer.async

eeda699

wangyum changed the title ~~[WIP][SPARK-28527] Enable ThriftServerQueryTestSuite~~ [SPARK-28527][test-maven] Enable ThriftServerQueryTestSuite Sep 22, 2019

wangyum changed the title ~~[SPARK-28527][test-maven] Enable ThriftServerQueryTestSuite~~ [WIP][SPARK-28527][test-hadoop3.2][test-java11] Enable ThriftServerQueryTestSuite Sep 22, 2019

wangyum changed the title ~~[WIP][SPARK-28527][test-hadoop3.2][test-java11] Enable ThriftServerQueryTestSuite~~ [WIP][SPARK-28527][SQL][test-hadoop3.2] Enable ThriftServerQueryTestSuite Sep 22, 2019

wangyum changed the title ~~[WIP][SPARK-28527][SQL][test-hadoop3.2] Enable ThriftServerQueryTestSuite~~ [SPARK-28527][SQL] Enable ThriftServerQueryTestSuite Sep 22, 2019

Remove HiveSQLException

2eac612

wangyum changed the title ~~[SPARK-28527][SQL] Enable ThriftServerQueryTestSuite~~ [SPARK-28527][SQL][test-hadoop3.2][test-java11] Enable ThriftServerQueryTestSuite Sep 23, 2019

dongjoon-hyun changed the title ~~[SPARK-28527][SQL][test-hadoop3.2][test-java11] Enable ThriftServerQueryTestSuite~~ [SPARK-28527][SQL] Enable ThriftServerQueryTestSuite Sep 24, 2019

dongjoon-hyun approved these changes Sep 24, 2019

View reviewed changes

wangyum changed the title ~~[SPARK-28527][SQL] Enable ThriftServerQueryTestSuite~~ [SPARK-28527][SQL][TEST] Enable ThriftServerQueryTestSuite Sep 24, 2019

wangyum closed this in b8b67ae Sep 24, 2019

wangyum deleted the SPARK-28527-enable branch September 24, 2019 07:45

zsxwing reviewed Sep 24, 2019

View reviewed changes

juliuszsompolski reviewed Oct 18, 2019

View reviewed changes

[SPARK-28527][SQL][TEST] Enable ThriftServerQueryTestSuite #25868

[SPARK-28527][SQL][TEST] Enable ThriftServerQueryTestSuite #25868

Uh oh!

Conversation

wangyum commented Sep 20, 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?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

SparkQA commented Sep 20, 2019

Uh oh!

wangyum commented Sep 20, 2019

Uh oh!

SparkQA commented Sep 20, 2019

Uh oh!

HyukjinKwon commented Sep 20, 2019

Uh oh!

SparkQA commented Sep 22, 2019

Uh oh!

SparkQA commented Sep 22, 2019

Uh oh!

wangyum commented Sep 22, 2019

Uh oh!

SparkQA commented Sep 22, 2019

Uh oh!

wangyum commented Sep 22, 2019

Uh oh!

wangyum commented Sep 22, 2019

Uh oh!

wangyum commented Sep 22, 2019

Uh oh!

SparkQA commented Sep 22, 2019

Uh oh!

SparkQA commented Sep 22, 2019

Uh oh!

SparkQA commented Sep 22, 2019

Uh oh!

wangyum commented Sep 22, 2019

Uh oh!

wangyum commented Sep 22, 2019

Uh oh!

SparkQA commented Sep 22, 2019

Uh oh!

SparkQA commented Sep 22, 2019

Uh oh!

wangyum commented Sep 22, 2019

Uh oh!

wangyum commented Sep 22, 2019

Uh oh!

wangyum commented Sep 22, 2019

Uh oh!

SparkQA commented Sep 23, 2019

Uh oh!

SparkQA commented Sep 23, 2019

Uh oh!

wangyum commented Sep 23, 2019

Uh oh!

SparkQA commented Sep 23, 2019

Uh oh!

wangyum commented Sep 23, 2019

Uh oh!

SparkQA commented Sep 23, 2019

Uh oh!

wangyum commented Sep 23, 2019

Uh oh!

SparkQA commented Sep 23, 2019

Uh oh!

wangyum commented Sep 23, 2019

Uh oh!

SparkQA commented Sep 23, 2019

Uh oh!

dongjoon-hyun commented Sep 23, 2019

Uh oh!

wangyum commented Sep 24, 2019

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

wangyum commented Sep 20, 2019 •

edited

Loading

zsxwing Sep 24, 2019 •

edited

Loading

dongjoon-hyun Sep 25, 2019 •

edited

Loading