Skip to content

[SPARK-46514][TESTS] Fix HiveMetastoreLazyInitializationSuite #44500

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
yaooqinn wants to merge 2 commits into from
Closed

[SPARK-46514][TESTS] Fix HiveMetastoreLazyInitializationSuite #44500

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
adf33b6
[SPARK-46514][TESTS] Fix HiveMetastoreLazyInitializationSuite
yaooqinn Dec 26, 2023
0fc526c
Update sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive…
yaooqinn Dec 28, 2023
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
5 changes: 3 additions & 2 deletions ...erver/src/test/scala/org/apache/spark/sql/hive/HiveMetastoreLazyInitializationSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ class HiveMetastoreLazyInitializationSuite extends SparkFunSuite {
.master("local[2]")
.enableHiveSupport()
.config("spark.hadoop.hive.metastore.uris", "thrift://127.0.0.1:11111")
.config("spark.ui.enabled", "false")
.config("hive.metastore.connect.retries", "1")
Copy link
Member

@MaxGekk MaxGekk Dec 26, 2023

Choose a reason for hiding this comment

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

Could you clarify why do you need this, please. Won't it lead to test flakiness?

Copy link
Member

@dongjoon-hyun dongjoon-hyun Dec 27, 2023

Choose a reason for hiding this comment

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

+1 for @MaxGekk 's comment.

Copy link
Member Author

@yaooqinn yaooqinn Dec 27, 2023

Choose a reason for hiding this comment

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

Hi @MaxGekk @dongjoon-hyun, as thrift://127.0.0.1:11111 is an unreachable HMS endpoint, it's better to fail at once w/o unnecessary retries to reduce the test time. It will not increase the flakiness.

Copy link
Member Author

@yaooqinn yaooqinn Dec 27, 2023

Choose a reason for hiding this comment

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

Another thing is that this SparkSession instance is not for sharing and will be closed in the final block.

Copy link
Contributor

@LuciferYang LuciferYang Dec 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think the purpose of this configuration is to allow for fast failure in order to save some test time, right? Even without this change, the test can still succeed, correct?

Copy link
Member Author

@yaooqinn yaooqinn Dec 27, 2023

Choose a reason for hiding this comment

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

Yes, the purpose here is the same as ui.enabled for lightweight testing. There is no increase in risk of flakiness here. It's just a piggybacking change. I did this regarding to low possibility of future changes against this file. It's also OK to remove them and leave it as-is if you insist.

Copy link
Member

@dongjoon-hyun dongjoon-hyun Dec 27, 2023

Choose a reason for hiding this comment

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

Fail-fast is not a bug fix. To be clear, please proceed that first before this PR, @yaooqinn .

Copy link
Member Author

@yaooqinn yaooqinn Dec 28, 2023

Choose a reason for hiding this comment

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

Thanks @dongjoon-hyun, removed

Copy link
Member Author

@yaooqinn yaooqinn Dec 28, 2023

Choose a reason for hiding this comment

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

Since this is restored, the PR is only one line change, I will merge it now

.getOrCreate()
val originalLevel = LogManager.getRootLogger.asInstanceOf[Logger].getLevel
val originalClassLoader = Thread.currentThread().getContextClassLoader
Expand Down Expand Up @@ -61,11 +63,10 @@ class HiveMetastoreLazyInitializationSuite extends SparkFunSuite {
spark.sql("show tables")
})
for (msg <- Seq(
"show tables",
"Could not connect to meta store",
"org.apache.thrift.transport.TTransportException",
"Connection refused")) {
exceptionString.contains(msg)
assert(exceptionString.contains(msg))
}
} finally {
Thread.currentThread().setContextClassLoader(originalClassLoader)
Expand Down