SPARK-2269 Refactor mesos scheduler resourceOffers and add unit test #1487
Conversation
|
Can one of the admins verify this patch? |
tnachen
changed the title
|
Do I need to specify someone to verify this patch? |
|
Jenkins, test this please. Naw this is a message from jenkins. |
| val conf = new SparkConf | ||
| conf.set("spark.executor.memory", "100m") | ||
| conf.set("spark.home", "/path") | ||
| val sc = new SparkContext("local-cluster[2 , 1 , 512]", "test", conf) |
There was a problem hiding this comment.
it would be better not to create a whole SparkContext here for the tests (creating a local context like this is really expensive). Instead, could this test just create a MesosSchedulerBackend directly? I think you can even just pass it null for the SparkContext (or you could mock one, depending on how it is used).
|
QA tests have started for PR 1487. This patch merges cleanly. |
|
Don't have a ton of time to look ATM but I kicked off the tests. |
|
@pwendell sounds good, I updated the PR addressing your comments |
|
QA results for PR 1487: |
|
@pwendell The console said the test failed but in a very unrelated way, is CI failing in general? |
|
Jenkins, retest this please. Jenkins, add to whitelist. |
|
QA tests have started for PR 1487. This patch merges cleanly. |
|
QA results for PR 1487: |
|
@pwendell this seems to pass tests now, mind to take a look? |
|
retest this please |
|
@tnachen What is the state of this patch? Is the existing logic relevant after the recent changes? |
|
QA tests have started for PR 1487 at commit
|
|
QA tests have finished for PR 1487 at commit
|
|
hi @andrewor14 yes the logic is still the same, it is good to merge as well. |
|
@andrewor14 can someone help merge this? It's been here for 2 months |
| offer.getHostname, | ||
| getResource(offer.getResourcesList, "cpus").toInt) | ||
|
|
||
| private def inClassLoader()(fun: => Unit) = { |
There was a problem hiding this comment.
Can you add a javadoc on why this is needed?
There was a problem hiding this comment.
This is actually just a refactor of what's already in the backend, I don't actualy know why we need to set and restore class loaders
|
Hey @tnachen sorry for leaving this sitting. This seems like a small enough refactor get into 1.2 at this point. Once you rebase to master and address the minor comments I will merge this. |
tnachen
force-pushed
the
resource_offer_refactor
branch
from
f86768b
to
4ea5dec
Compare
|
@andrewor14 thanks for the review, I just rebased and should address all your comments too |
|
Test build #23222 has started for PR 1487 at commit
|
|
Test build #23222 has finished for PR 1487 at commit
|
|
Test PASSed. |
|
Ok LGTM I'm merging this into master and 1.2 |
Author: Timothy Chen <tnachen@gmail.com> Closes #1487 from tnachen/resource_offer_refactor and squashes the following commits: 4ea5dec [Timothy Chen] Rebase from master and address comments 9ccab09 [Timothy Chen] Address review comments e6494dc [Timothy Chen] Refactor class loading 8207428 [Timothy Chen] Refactor mesos scheduler resourceOffers and add unit test (cherry picked from commit a878660) Signed-off-by: Andrew Or <andrew@databricks.com>
* Update rio to use same docker image as boson. * Add pipelines to trigger boson unit tests whith new PR.
No description provided.