-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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.
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
[SPARK-33084][SQL][TEST][FOLLOWUP] Add ResetSystemProperties trait to SQLQuerySuite to avoid clearing ivy.home #31694
Conversation
…o SQLQuerySuite and use newer Ivy property for local directory override.
cc folks from PR #29966: @viirya @maropu @wangyum @HyukjinKwon @dongjoon-hyun |
Test build #135604 has finished for PR 31694 at commit
|
Looks like a valid failure, so could you check it? |
Hm. I'm having trouble running the I don't really feel the failure is related; it is a timeout being exceeded in the Is there any good way for me to re-trigger the tests? Should I just push an empty commit? |
Yea, can you push an empty commit? |
retest this please |
Test build #135616 has finished for PR 31694 at commit
|
The I dug a bit deeper and it turns out what I said in the description wasn't quite right (will update now). The issue is that the behavior of
But for
So for the I have updated the patch to leave the system property as |
Kubernetes integration test starting |
Kubernetes integration test status success |
Test build #135672 has finished for PR 31694 at commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc: @HyukjinKwon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM2
Merged to master |
Thanks @HyukjinKwon and @maropu ! Much appreciated. |
What changes were proposed in this pull request?
Add the
ResetSystemProperties
trait toSQLQuerySuite
so that system property changes made by any of the tests will not affect other suites/tests. Specifically, the system property changes made bySPARK-33084: Add jar support Ivy URI in SQL -- jar contains udf class
are targeted here (which sets and then clearsivy.home
).Why are the changes needed?
PR #29966 added a new test case that adjusts the
ivy.home
system property to force Ivy to resolve an artifact from a custom location. At the end of the test, the value is cleared. Clearing the value meant that, if a custom value ofivy.home
was configured externally, it would not apply for tests run after this test case.Does this PR introduce any user-facing change?
No, this is only in tests.
How was this patch tested?
Existing unit tests continue to pass, whether or not
spark.jars.ivySettings
is configured (which adjusts the behavior of Ivy w.r.t. handling ofivy.home
andivy.default.ivy.user.dir
properties).