[SPARK-28203][Core][Python] PythonRDD should respect SparkContext's hadoop configuration#25002
[SPARK-28203][Core][Python] PythonRDD should respect SparkContext's hadoop configuration#25002advancedxy wants to merge 2 commits intoapache:masterfrom
Conversation
…adoop configuration. This commit also makes merging hadoop conf consistent in several places of PythonRDD
core/src/test/scala/org/apache/spark/api/python/PythonHadoopUtilSuite.scala
Outdated
Show resolved
Hide resolved
| */ | ||
| def mapToConf(map: java.util.Map[String, String]): Configuration = { | ||
| val conf = new Configuration() | ||
| val conf = new Configuration(false) |
There was a problem hiding this comment.
I'm wondering if this doesn't break anything. Did you run the UT locally?
There was a problem hiding this comment.
Internally this is only called in PythonRDD and I have replaced all the invocations with merged SparkContext's hadoop conf. So it shouldn't break things in spark side. I ran the UTs of Scala side, haven't run python unit tests though.
mapToConf from PythonHadoopUtilSuite to PythonRDDSuite
|
@dongjoon-hyun sorry for the delay. I added a test case which should clearly show the wrongly used case in PythonRDD. |
|
Gently ping @dongjoon-hyun. |
|
seems reasonable to me |
|
ok to test |
|
yea, it seems fine to me too. Let me take another look since this fixes RDD API which is pretty conservative lately. |
|
Test build #108433 has finished for PR 25002 at commit
|
|
@HyukjinKwon The failure looks unrelated to this commit, could you trigger the test again or add |
|
retest this please |
|
Test build #108445 has finished for PR 25002 at commit
|
Still unrelated to this commit. Test should be re-triggered. Sorry for the disturbance, @HyukjinKwon |
|
Gently ping @cloud-fan, @HyukjinKwon and @dongjoon-hyun. |
|
retest this please |
|
Test build #108962 has finished for PR 25002 at commit
|
|
@HyukjinKwon tests are passed. Do you have any other concerns? |
|
Merged to master. |
gatorsmile
left a comment
gatorsmile
left a comment
There was a problem hiding this comment.
There are 92 usages of new Configuration(), whose default is true.
@advancedxy Could you please check whether their usage is right?
|
@gatorsmile I did a quick search, most of the usage happens in XxxSuite file, so it may be ignored. Let me do have a thorough look and report back. |
|
@gatorsmile I believe only two occurrences of I will create a jira to resolve that. |
|
@advancedxy Thanks! |
6 participants
What changes were proposed in this pull request?
How was this patch tested?
Added a new test and existed tests