Skip to content
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

Jump to bottom

[SPARK-31914][SQL][Test][test-hive1.2][test-java11] Apply SharedThriftServer to all thrift server related tests #28738

Closed
wants to merge 3 commits into from
Closed

[SPARK-31914][SQL][Test][test-hive1.2][test-java11] Apply SharedThriftServer to all thrift server related tests #28738

wants to merge 3 commits into from

Conversation

yaooqinn
Copy link
Member

@yaooqinn yaooqinn commented Jun 5, 2020 •
edited

What changes were proposed in this pull request?

  1. This PR apply SharedThriftServer trait to all ThriftServer related tests including
HiveThriftBinaryServerSuite
HiveThriftCleanUpScratchDirSuite
HiveThriftHttpServerSuite
JdbcConnectionUriSuite
SingleSessionSuite
SparkMetadataOperationSuite
SparkThriftServerProtocolVersionsSuite
UISeleniumSuite

SharedThriftServer can use a random free port to reduce the flakiness of these tests.

  1. JDBC related tests are moved to the JdbcConnectionSuite to test both http and binary mode. Before some of them are duplicated, and others are only tested in one mode.
  2. This PR also re-enabled the UISeleniumSuite which has been ignored since 2.0.

Why are the changes needed?

  1. reduce the flakiness of tests.
  2. improve test coverage
  3. simplification and remove some duplicated tests

Does this PR introduce any user-facing change?

no

How was this patch tested?

existing tests

@yaooqinn yaooqinn changed the title [SPARK-31914][SQL][Test][test-hive1.2][test-java11] Apply SharedThriftServer to all ThriftServer related tests [SPARK-31914][SQL][Test][test-hive1.2][test-java11] Apply SharedThriftServer to all thrift server related tests Jun 5, 2020
@yaooqinn yaooqinn marked this pull request as draft June 5, 2020 13:24
@yaooqinn yaooqinn mentioned this pull request Jun 5, 2020
Closed
@juliuszsompolski
Copy link
Contributor

Nice cleanup of the Thriftserver tests!
I think it still has some merit to test the Thriftserver started with the start-thriftserver.sh script, so I would not remove that old infra completely, but maybe leave some basic suite for that?

@SparkQA
Copy link

SparkQA commented Jun 5, 2020

Test build #123566 has finished for PR 28738 at commit 9fde365.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class HiveThriftBinaryServerSuite extends SharedThriftServer
  • class SingleSessionSuite extends SharedThriftServer
  • class HiveThriftCleanUpScratchDirSuite extends SharedThriftServer
  • class HiveThriftHttpServerSuite extends SharedThriftServer
  • class JdbcConnectionUriSuite extends SharedThriftServer
  • class SparkMetadataOperationSuite extends SharedThriftServer
  • class SparkThriftServerProtocolVersionsSuite extends SharedThriftServer

@yaooqinn
Copy link
Member Author

yaooqinn commented Jun 5, 2020 •
edited

Nice cleanup of the Thriftserver tests!
I think it still has some merit to test the Thriftserver started with the start-thriftserver.sh script, so I would not remove that old infra completely, but maybe leave some basic suite for that?

start-thriftserver.sh script with port retry is flaky for testing, maybe we can dockerize some of them with integration test?

@SparkQA
Copy link

SparkQA commented Jun 5, 2020

Test build #123567 has finished for PR 28738 at commit 9812e50.

  • This patch fails Spark unit tests.
  • This patch does not merge cleanly.
  • This patch adds the following public classes (experimental):
  • class HiveThriftBinaryServerSuite extends JdbcConnectionSuite
  • class HiveThriftHttpServerSuite extends JdbcConnectionSuite
  • trait JdbcConnectionSuite extends SharedThriftServer

juliuszsompolski
juliuszsompolski reviewed Jun 5, 2020
View reviewed changes
Copy link
Contributor

@juliuszsompolski juliuszsompolski left a comment

Choose a reason for hiding this comment

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

start-thriftserver.sh script with port retry is flaky for testing, maybe we can dockerize some of them with integration test?

You could do a port 0 trick with start-thriftserver.sh as well.

...ftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/HiveThriftServer2Suites.scala
Comment on lines -1184 to -1194
/**
* String to scan for when looking for the thrift binary endpoint running.
* This can change across Hive versions.
*/
val THRIFT_BINARY_SERVICE_LIVE = "Starting ThriftBinaryCLIService on port"

/**
* String to scan for when looking for the thrift HTTP endpoint running.
* This can change across Hive versions.
*/
val THRIFT_HTTP_SERVICE_LIVE = "Started ThriftHttpCLIService in http"
Copy link
Contributor

Choose a reason for hiding this comment

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

If this becomes flaky because of the port, you could set port 0, and turn the use of these lines into a regex that would parse the port that got assigned...

...ftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/HiveThriftServer2Suites.scala
Comment on lines -1257 to -1261
successLines.foreach { r =>
if (line.contains(r)) {
serverStarted.trySuccess(())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

... by parsing for the port here instead of the line.contains.

@SparkQA
Copy link

SparkQA commented Jun 5, 2020

Test build #123571 has finished for PR 28738 at commit 70d0ab3.

  • This patch fails Spark unit tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@github-actions
Copy link

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

@github-actions github-actions bot added the Stale label Sep 14, 2020
@github-actions github-actions bot closed this Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliuszsompolski juliuszsompolski juliuszsompolski left review comments

Assignees
No one assigned
Labels
SQL Stale
Projects
None yet
Milestone
No milestone
3 participants
@yaooqinn @juliuszsompolski @SparkQA