Skip to content

Conversation

@drcrallen
Copy link
Contributor

The offer acceptance workflow is a little hard to follow and not very extensible for future considerations for offers. This is a patch that makes the workflow a little more explicit in its handling of offer resources.

@rxin
Copy link
Contributor

rxin commented Jun 16, 2016

Thanks for the pull request, but the refactoring seems to have made it more difficult to read?

@drcrallen
Copy link
Contributor Author

I broke something in the test suite, evaluating and I'll open this back in a bit

@drcrallen drcrallen closed this Jun 17, 2016
@drcrallen drcrallen reopened this Jun 17, 2016
def calculateUsableResources(sc: SparkContext, availableCpus: Int, availableMem: Int):
Option[(Int, Int)] = {
val desiredMemory = executorMemory(sc)
val desiredCpu = executorCores(availableCpus)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea, but it does make it harder to read. Can we refactor so we can collapse global and resource limits, and introduce like a case class for each condition, where it knows how to verify and logs error message when it doesn't match?
I think it makes adding and understanding how offers are matched better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do that. Will be a few weeks before I can tackle this again though.

@tnachen
Copy link
Contributor

tnachen commented Jun 22, 2016

ok to test

@SparkQA
Copy link

SparkQA commented Jun 22, 2016

Test build #60997 has finished for PR 13715 at commit 9e0aedf.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member

(ping @drcrallen)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants