[SPARK-26082][MESOS][FOLLOWUP] Fix Scala-2.11 build #23755
Conversation
… on MesosClusterScheduler" This reverts commit b4e1d14.
…sClusterScheduler This patch adds UT on testing SPARK-26082 to avoid regression. While apache#23743 reduces the possibility to make a similar mistake, the needed lines of code for adding tests are not that huge, so I guess it might be worth to add them. Newly added UTs. Test "supports setting fetcher cache" fails when apache#23743 is not applied and succeeds when apache#23743 is applied.
|
NOTE: This reverts commit which #23744 is introduced to make sure patches among versions are not diverged. |
|
Test build #102121 has finished for PR 23755 at commit
|
dongjoon-hyun
changed the title
|
@HeartSaVioR . For a follow PR, we had better use a specific title instead of copying a original PR title. |
dongjoon-hyun
changed the title
|
I updated for the PR title and description. For the other PRs, please revise them properly. |
|
@dongjoon-hyun I should've explain the intention: sorry about that. I thought you could revert the commit into master too, and replace the old commit with new commit: then we can just have same title and description. (Actually the commit doesn't make merge conflict so you can cherry-pick again: I just wanted to make sure build passes for every branches.) For other branches we already reverted the commit, so the title and description should be valid given I'm not mistaken during old PR. Does it make sense? |
|
This one is better, @HeartSaVioR . +1, LGTM. I tested this one manually with |
|
There are several instances to cause Scala-2.11 build break. We had better prevent before merging. But, if that happens, I prefer this commit. This commit is better than full reverting because the other people can easily identify what cause Scala-2.11 build break. Thank you again for your time. |
|
Yeah I'm seeing the benefit. Thanks for considering it thoughtfully! Btw I guess you've found missing spot on referring PR's number in test section and it applies same in previous commit. I wanted to fix via replacing old commit with new commit at this time. Hope my intention is clear. |
|
|
|
OK. If we would want to also correct the description, what about making change like below:
There's nothing I could do myself in above step: I'd like to just propose one of approaches. I'm sure I respect your decision and how to proceed is up to your preference. Please mention me anytime when I need to make some change on your approach. Thanks! |
|
Btw, maybe worth to discuss how to handle backport between master and 2.x version lines? I'd rather think we cannot avoid compilation issue between scala 2.12 and 2.11 unless we run build for both. |
|
I just confused your wording. When I commented Thank you for suggestion. I don't think we need that (1), (2), (3). It's too minor. Please just fix the test case names in your new PR on branch-2.4/2.3. |
|
For the record, it's worth to have b8d6669 . |
|
OK never mind. I'll just update the PR for branch-2.4/branch-2.3. |
## What changes were proposed in this pull request? apache#23744 added a UT to prevent a future regression. However, it breaks Scala-2.11 build. This fixes that. ## How was this patch tested? Manual test with Scala-2.11 profile. Closes apache#23755 from HeartSaVioR/SPARK-26082-FOLLOW-UP-V2. Authored-by: Jungtaek Lim (HeartSaVioR) <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>
3 participants
What changes were proposed in this pull request?
#23744 added a UT to prevent a future regression. However, it breaks Scala-2.11 build. This fixes that.
How was this patch tested?
Manual test with Scala-2.11 profile.