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

Jump to bottom
Closed
yaooqinn wants to merge 2 commits into from
Closed

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

yaooqinn wants to merge 2 commits into from

Conversation

@yaooqinn
Copy link
Member

@yaooqinn yaooqinn commented Dec 26, 2023

What changes were proposed in this pull request?

This PR enabled the assertion in HiveMetastoreLazyInitializationSuite

Why are the changes needed?

fix test intenton

Does this PR introduce any user-facing change?

no

How was this patch tested?

pass HiveMetastoreLazyInitializationSuite

Was this patch authored or co-authored using generative AI tooling?

no

@github-actions github-actions bot added the SQL label Dec 26, 2023
@yaooqinn
Copy link
Member Author

yaooqinn commented Dec 26, 2023

cc @LuciferYang @dongjoon-hyun thanks.

MaxGekk
MaxGekk reviewed Dec 26, 2023
View reviewed changes
...ftserver/src/test/scala/org/apache/spark/sql/hive/HiveMetastoreLazyInitializationSuite.scala Outdated
.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

@yaooqinn yaooqinn closed this in d0245d3 Dec 28, 2023
yaooqinn added a commit that referenced this pull request Dec 28, 2023
@yaooqinn
[SPARK-46514][TESTS] Fix HiveMetastoreLazyInitializationSuite
5d4a913 
### What changes were proposed in this pull request?

This PR enabled the assertion in HiveMetastoreLazyInitializationSuite

### Why are the changes needed?

fix test intenton

### Does this PR introduce _any_ user-facing change?

no

### How was this patch tested?

pass HiveMetastoreLazyInitializationSuite

### Was this patch authored or co-authored using generative AI tooling?

no

Closes #44500 from yaooqinn/SPARK-46514.

Authored-by: Kent Yao <[email protected]>
Signed-off-by: Kent Yao <[email protected]>
(cherry picked from commit d0245d3)
Signed-off-by: Kent Yao <[email protected]>
yaooqinn added a commit that referenced this pull request Dec 28, 2023
@yaooqinn
[SPARK-46514][TESTS] Fix HiveMetastoreLazyInitializationSuite
ee44914 
### What changes were proposed in this pull request?

This PR enabled the assertion in HiveMetastoreLazyInitializationSuite

### Why are the changes needed?

fix test intenton

### Does this PR introduce _any_ user-facing change?

no

### How was this patch tested?

pass HiveMetastoreLazyInitializationSuite

### Was this patch authored or co-authored using generative AI tooling?

no

Closes #44500 from yaooqinn/SPARK-46514.

Authored-by: Kent Yao <[email protected]>
Signed-off-by: Kent Yao <[email protected]>
(cherry picked from commit d0245d3)
Signed-off-by: Kent Yao <[email protected]>
@yaooqinn
Copy link
Member Author

yaooqinn commented Dec 28, 2023

Thank you all. Merged to master/3.5/3.4

szehon-ho pushed a commit to szehon-ho/spark that referenced this pull request Feb 7, 2024
@yaooqinn @dongjoon-hyun
[SPARK-46514][TESTS] Fix HiveMetastoreLazyInitializationSuite
e55f970 
### What changes were proposed in this pull request?

This PR enabled the assertion in HiveMetastoreLazyInitializationSuite

### Why are the changes needed?

fix test intenton

### Does this PR introduce _any_ user-facing change?

no

### How was this patch tested?

pass HiveMetastoreLazyInitializationSuite

### Was this patch authored or co-authored using generative AI tooling?

no

Closes apache#44500 from yaooqinn/SPARK-46514.

Authored-by: Kent Yao <[email protected]>
Signed-off-by: Kent Yao <[email protected]>
(cherry picked from commit d0245d3)
Signed-off-by: Kent Yao <[email protected]>
turboFei pushed a commit to turboFei/spark that referenced this pull request Nov 6, 2025
@yaooqinn
[SPARK-46514][TESTS] Fix HiveMetastoreLazyInitializationSuite
f38ec70 
### What changes were proposed in this pull request?

This PR enabled the assertion in HiveMetastoreLazyInitializationSuite

### Why are the changes needed?

fix test intenton

### Does this PR introduce _any_ user-facing change?

no

### How was this patch tested?

pass HiveMetastoreLazyInitializationSuite

### Was this patch authored or co-authored using generative AI tooling?

no

Closes apache#44500 from yaooqinn/SPARK-46514.

Authored-by: Kent Yao <[email protected]>
Signed-off-by: Kent Yao <[email protected]>
(cherry picked from commit d0245d3)
Signed-off-by: Kent Yao <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LuciferYang LuciferYang LuciferYang approved these changes

@MaxGekk MaxGekk MaxGekk left review comments

@beliefer beliefer beliefer left review comments

@dongjoon-hyun dongjoon-hyun dongjoon-hyun left review comments

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@yaooqinn @LuciferYang @MaxGekk @beliefer @dongjoon-hyun