Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wangyum wants to merge 5 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2a422ea
Enable SPARK-28527
wangyum Sep 20, 2019
0b75d5b
fix test error
wangyum Sep 22, 2019
1fede27
Enable more tests
wangyum Sep 22, 2019
eeda699
Move spark.sql.hive.thriftServer.async
wangyum Sep 22, 2019
2eac612
Remove HiveSQLException
wangyum Sep 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Move spark.sql.hive.thriftServer.async
  • Loading branch information
@wangyum
wangyum committed Sep 22, 2019
commit eeda69946426ea70d33e6c3a1f8e1d056b41c8b0
3 changes: 0 additions & 3 deletions sql/core/src/test/scala/org/apache/spark/sql/test/SharedSparkSession.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,6 @@ trait SharedSparkSessionBase
// this rule may potentially block testing of other optimization rules such as
// ConstantPropagation etc.
.set(SQLConf.OPTIMIZER_EXCLUDED_RULES.key, ConvertToLocalRelation.ruleName)
// Hive Thrift server should not executes SQL queries in an asynchronous way
// because we may set session configuration.
.set("spark.sql.hive.thriftServer.async", "false")
conf.set(
StaticSQLConf.WAREHOUSE_PATH,
conf.get(StaticSQLConf.WAREHOUSE_PATH) + "/" + getClass.getCanonicalName)
Expand Down
8 changes: 7 additions & 1 deletion ...er/src/test/scala/org/apache/spark/sql/hive/thriftserver/ThriftServerQueryTestSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,12 @@ import org.apache.commons.lang3.exception.ExceptionUtils
import org.apache.hadoop.hive.conf.HiveConf.ConfVars
import org.apache.hive.service.cli.HiveSQLException

import org.apache.spark.SparkException
import org.apache.spark.{SparkConf, SparkException}
import org.apache.spark.sql.SQLQueryTestSuite
import org.apache.spark.sql.catalyst.analysis.NoSuchTableException
import org.apache.spark.sql.catalyst.util.fileToString
import org.apache.spark.sql.execution.HiveResult
import org.apache.spark.sql.hive.HiveUtils
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.types._

Expand Down Expand Up @@ -75,6 +76,11 @@ class ThriftServerQueryTestSuite extends SQLQueryTestSuite {
}
}

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)
Copy link
Member

@zsxwing zsxwing Sep 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@dongjoon-hyun dongjoon-hyun Sep 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@zsxwing zsxwing Sep 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@dongjoon-hyun dongjoon-hyun Sep 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@dongjoon-hyun dongjoon-hyun Sep 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@juliuszsompolski juliuszsompolski Oct 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


override val isTestWithConfigSets = false

/** List of test cases to ignore, in lower cases. */
Expand Down