[SPARK-2140] Updating heap memory calculation for YARN stable and alpha.

[copester]

Updated pull request, reflecting YARN stable and alpha states. I am getting intermittent test failures on my own test infrastructure. Is that tracked anywhere yet?

[AmplabJenkins]

Can one of the admins verify this patch?

[tgravescs]

can you just remove the entire calculateAMMemory functions and just use the args.amMemory where they are called (ClientBase.createContainerLaunchContext)?

[tgravescs]

Jenkins, test this please

[mridulm]

Hey Tom, All that mod business was done so that we allocate container memory in multiples of minimum resource capability. I think this was required (atleast a long while ago).

If that is not the case, then we need to definitely clean all that complicated computation up !

[tgravescs]

Hey Mridul,

Yarn actually handles doing the multiple for you. Perhaps really early 0.23 builds didn't do this. For instance minimum container size of 512m and you ask for 700m, it will round that up and give you a 1g container. Obviously one thing it doesn't do now is change your heap size. It will be exactly what the user requests vs us rounding it up. If we think that is important then we can put it back in. I am leaning on the side of removing unneeded logic and giving the user the size they really request.

Also as you will notice that logic hasn't been there in the hadoop 2.x port anyway which I think more people are using at this point.

[tgravescs]

Jenkins, test this please

[sryza]

I think it's preferable to give the user the size they actually request. This avoids them requesting the same size later under different conditions and unexpectedly OOMing. Though it's possibly worth warning users about how much their containers sizes got rounded up?

[tgravescs]

not sure why jenkins isn't running on this.... try again.

test this please

[copester]

Independent of Jenkins, I notice that some of the test suites fails sometimes. This is unrelated to this patch, just thought I'd mention it.

[mateiz]

Jenkins, test this please

[mateiz]

It's been down this afternoon

[tgravescs]

Jenkins, test this please

[SparkQA]

Can one of the admins verify this patch?

[copester]

Is this merge blocking on Jenkins? I can provide a JUnit test report if you want?

[tgravescs]

Jenkins, test this please

[tgravescs]

yes just want to get a clean jenkins run. Otherwise it looks good. Jenkins should be back up now.

[tgravescs]

I was looking at this a bit more and I think our checks around the memory args in ClientBase.validateArgs are now out of date. It used to be that the memory specific by the user had to be greater then the memory overhead, but not we just take that on so those checks are no longer valid.

Would you mind taking a look at those and fixing along with this? If not we can file a separate jira for it also since it wasn't introduced by you.

[tgravescs]

Jenkins, test this please

[tgravescs]

@mateiz @pwendell @andrewor14 anyone know why jenkins isn't picking this pr up?

[JoshRosen]

Manually triggered Jenkins using my new script; automated triggering should be added shortly.

[SparkQA]

QA tests have started for PR 2253 at commit 5ad89da.

• This patch merges cleanly.

[andrewor14]

LGTM, I was actually making a similar change in parallel.

[tgravescs]

The changes look fine. I'm waiting for Jenkins. I'll file a separate jira to handle changing ClientBase.validateArgs.

[copester]

Has Jenkins been stuck for others, or is my pr just special?

[JoshRosen]

@copester It should actually be testing now, triggered manually by my private hook: https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/19/consoleFull. My current theory is that the Jenkins plugin is recognizing new PRs but not comments on those PRs. We're in the process of building a new mechanism for triggering PRs to replace the Jenkins plugin; I hope to have this live by this afternoon.

[copester]

@JoshRosen thanks. I'm brand new to the Spark community and just wanted to make sure my workflow was correct per testing.

[SparkQA]

QA tests have finished for PR 2253 at commit 5ad89da.

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

[tgravescs]

+1. Thanks @copester