-
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-33740][SQL] hadoop configs in hive-site.xml can overrides pre-existing hadoop ones #30709
Conversation
…existing hadoop ones
cc @cloud-fan @maropu thanks |
Kubernetes integration test starting |
Kubernetes integration test status success |
Test build #132578 has finished for PR 30709 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.
+1, LGTM. I checked that the code is added by SPARK-31170 at 3.0.0, too.
Merged to master/3.1.
…existing hadoop ones ### What changes were proposed in this pull request? org.apache.hadoop.conf.Configuration#setIfUnset will ignore those with defaults too ### Why are the changes needed? fix a regression ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? new tests Closes #30709 from yaooqinn/SPARK-33740. Authored-by: Kent Yao <yaooqinn@hotmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 31e0bac) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Could you make a backport to |
|
||
def containsInSparkConf(key: String): Boolean = { | ||
sparkConf.contains(key) || sparkConf.contains("spark.hadoop." + key) || | ||
(key.startsWith("hive") && sparkConf.contains("spark." + key)) |
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.
nit: It seems there is no test for the condition (key.startsWith("hive") && sparkConf.contains("spark." + key))
?
@@ -228,14 +228,21 @@ object SharedState extends Logging { | |||
sparkConf: SparkConf, | |||
hadoopConf: Configuration, | |||
initialConfigs: scala.collection.Map[String, String] = Map.empty): Unit = { | |||
|
|||
def containsInSparkConf(key: String): Boolean = { | |||
sparkConf.contains(key) || sparkConf.contains("spark.hadoop." + key) || |
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.
(This is not related to this PR though) The string prefix "spark.hadoop."
seems to be used in many places, so how about defining a global value for it somewhere? I think that the refactoring could make it easy to track the positions where the prefix is used.
lgtm |
… pre-existing hadoop ones Backport #30709 to 3.0 ### What changes were proposed in this pull request? org.apache.hadoop.conf.Configuration#setIfUnset will ignore those with defaults too ### Why are the changes needed? fix a regression ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? new tests Closes #30720 from yaooqinn/SPARK-33740-30. Authored-by: Kent Yao <yaooqinn@hotmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
What changes were proposed in this pull request?
org.apache.hadoop.conf.Configuration#setIfUnset will ignore those with defaults too
Why are the changes needed?
fix a regression
Does this PR introduce any user-facing change?
no
How was this patch tested?
new tests