[SPARK-26431][CORE] Update availableSlots by availableCpus for Barrier TaskSet #23375
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Author
|
ping @jiangxb1987 , please take a look, thanks. |
|
Can one of the admins verify this patch? |
jiangxb1987
reviewed
Dec 23, 2018
| */ | ||
| def resourceOffers(offers: IndexedSeq[WorkerOffer]): Seq[Seq[TaskDescription]] = synchronized { | ||
|
|
||
| def availableSlots(availableCpus: Array[Int]): Int = { |
Contributor
There was a problem hiding this comment.
The result is not always correct, please refer to test("don't schedule for a barrier taskSet if available slots are less than pending tasks")
Member
Author
There was a problem hiding this comment.
What do you mean for "The result is not always correct" ? The way of calculating slots num ?
Contributor
There was a problem hiding this comment.
yea, please refer to the test case above.
Member
Author
There was a problem hiding this comment.
I've seen and tested that suite before submitting this pr. And I follow the original way of calculating the slots, what's the difference ?
jiangxb1987
pushed a commit
that referenced
this pull request
Sep 27, 2019
…` before checking available slots for barrier taskSet ### What changes were proposed in this pull request? availableSlots are computed before the for loop looping over all TaskSets in resourceOffers. But the number of slots changes in every iteration, as in every iteration these slots are taken. The number of available slots checked by a barrier task set has therefore to be recomputed in every iteration from availableCpus. ### Why are the changes needed? Bugfix. This could make resourceOffer attempt to start a barrier task set, even though it has not enough slots available. That would then be caught by the `require` in line 519, which will throw an exception, which will get caught and ignored by Dispatcher's MessageLoop, so nothing terrible would happen, but the exception would prevent resourceOffers from considering further TaskSets. Note that launching the barrier TaskSet can still fail if other requirements are not satisfied, and still can be rolled-back by throwing exception in this `require`. Handling it more gracefully remains a TODO in SPARK-24818, but this fix at least should resolve the situation when it's unable to launch because of insufficient slots. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Added UT Closes #23375 Closes #25946 from juliuszsompolski/SPARK-29263. Authored-by: Juliusz Sompolski <julek@databricks.com> Signed-off-by: Xingbo Jiang <xingbo.jiang@databricks.com> (cherry picked from commit 420abb4) Signed-off-by: Xingbo Jiang <xingbo.jiang@databricks.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
availableCpus decrease as tasks allocated, so, we should update availableSlots by availableCpus for barrier taskset to avoid unnecessary resourceOffer process.
How was this patch tested?
existed.