[SPARK-27959][YARN] Change YARN resource configs to use .amount by tgravescs · Pull Request #24989 · apache/spark

tgravescs · 2019-06-27T21:42:54Z

What changes were proposed in this pull request?

we are adding in generic resource support into spark where we have suffix for the amount of the resource so that we could support other configs.

Spark on yarn already had added configs to request resources via the configs spark.yarn.{executor/driver/am}.resource=, where the is value and unit together. We should change those configs to have a .amount suffix on them to match the spark configs and to allow future configs to be more easily added. YARN itself already supports tags and attributes so if we want the user to be able to pass those from spark at some point having a suffix makes sense. it would allow for a spark.yarn.{executor/driver/am}.resource.{resource}.tag= type config.

How was this patch tested?

Tested via unit tests and manually on a yarn 3.x cluster with GPU resources configured on.

tgravescs · 2019-06-27T21:45:56Z

cc @szilard-nemeth I think you originally add these configs

SparkQA · 2019-06-27T22:04:48Z

Test build #106977 has finished for PR 24989 at commit 428f63a.

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

SparkQA · 2019-06-28T19:38:27Z

Test build #107023 has finished for PR 24989 at commit 6af9988.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

tgravescs · 2019-07-08T14:16:04Z

@vanzin I think you reviewed first change for the resources, you ok with this approach

vanzin

Looks ok.

I was thinking a little about what would happen if a new resource is supported by the scheduler (the user would have to set it in both configs, as your comment suggests) and whether that could be simplified somehow, but that sounds like a separate problem.

tgravescs · 2019-07-16T13:24:58Z

yeah its not ideal to have it hardcoded and then have to specify 2 configs for those. Thought about coming up with some sort of mapping but figured for now we can do this and come up with something else later.

SparkQA · 2019-07-16T13:47:23Z

Test build #107738 has finished for PR 24989 at commit 6e86ab9.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2019-07-16T16:10:32Z

Shall we test this with hadoop-2.7 profile, too?

vanzin · 2019-07-16T16:19:20Z

retest this please

SparkQA · 2019-07-16T16:44:45Z

Test build #107745 has finished for PR 24989 at commit 6e86ab9.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2019-07-16T17:55:43Z

Merging to master.

attilapiros

Mostly NITs otherwise LGTM.

attilapiros · 2019-07-16T16:55:31Z

 private val AMOUNT_AND_UNIT_REGEX = "([0-9]+)([A-Za-z]*)".r
 private val RESOURCE_INFO_CLASS = "org.apache.hadoop.yarn.api.records.ResourceInformation"
+ val YARN_GPU_RESOURCE_CONFIG = "yarn.io/gpu"
+ val YARN_FPGA_RESOURCE_CONFIG = "yarn.io/fpga"


Does it make sense to reuse ResourceUtils.GPU and ResourceUtils.FPGA here?
Like:
valYARN_GPU_RESOURCE_CONFIG="yarn.io/$GPU"valYARN_FPGA_RESOURCE_CONFIG="yarn.io/$FPGA"
For suggesting using as close mapping as possible between the Spark config keys and YARN resource config keys (now and the future).
I know your PR does not touched this part:
spark/resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ResourceRequestHelper.scala
Lines 111 to 118 in 6e86ab9
(ResourceID(SPARK_EXECUTOR_PREFIX, "fpga").amountConf,
s"${YARN_EXECUTOR_RESOURCE_TYPES_PREFIX}${YARN_FPGA_RESOURCE_CONFIG}"),
(ResourceID(SPARK_DRIVER_PREFIX, "fpga").amountConf,
s"${YARN_DRIVER_RESOURCE_TYPES_PREFIX}${YARN_FPGA_RESOURCE_CONFIG}"),
(ResourceID(SPARK_EXECUTOR_PREFIX, "gpu").amountConf,
s"${YARN_EXECUTOR_RESOURCE_TYPES_PREFIX}${YARN_GPU_RESOURCE_CONFIG}"),
(ResourceID(SPARK_DRIVER_PREFIX, "gpu").amountConf,
s"${YARN_DRIVER_RESOURCE_TYPES_PREFIX}${YARN_GPU_RESOURCE_CONFIG}"))
But at that place using the right constant ($GPU or $FPGA) instead of the string literal is reasonable.

attilapiros · 2019-07-16T17:25:52Z

+
+ private[yarn] def getYarnResourcesAndAmounts(
+ sparkConf: SparkConf,
+ componentName: String): Map[String, String] = {


Nit: in getYarnResourcesFromSparkResources the second parameter is the SparkConf what about taking the componentName (prefix) to the first place here too.

attilapiros · 2019-07-16T18:26:51Z

It is already closed. Anyway if you think my comments useful even I can do this small changes in a follow-up or in a minor PR.

## What changes were proposed in this pull request? we are adding in generic resource support into spark where we have suffix for the amount of the resource so that we could support other configs. Spark on yarn already had added configs to request resources via the configs spark.yarn.{executor/driver/am}.resource=<some amount>, where the <some amount> is value and unit together. We should change those configs to have a `.amount` suffix on them to match the spark configs and to allow future configs to be more easily added. YARN itself already supports tags and attributes so if we want the user to be able to pass those from spark at some point having a suffix makes sense. it would allow for a spark.yarn.{executor/driver/am}.resource.{resource}.tag= type config. ## How was this patch tested? Tested via unit tests and manually on a yarn 3.x cluster with GPU resources configured on. Closesapache#24989 from tgravescs/SPARK-27959-yarn-resourceconfigs. Authored-by: Thomas Graves <tgraves@nvidia.com> Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>

tgravescs added 3 commits June 27, 2019 13:06

[SPARK-27959] Change YARN resource configs to use .amount
481abaf

Fix unit test
bef554b

fix conflicting resources
428f63a

tgravescs added 2 commits June 28, 2019 13:50

Fix tests for change to .amount
0d87d08

fix import order:
6af9988

dongjoon-hyun added the YARN label Jun 28, 2019

vanzin reviewed Jul 15, 2019
View reviewed changes

Comment threadresource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ResourceRequestHelper.scala Outdated
Comment thread...e-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/ResourceRequestHelperSuite.scala Outdated

rework
6e86ab9

vanzin changed the title ~~[SPARK-27959][YARN][test-hadoop3.2] Change YARN resource configs to use .amount~~[SPARK-27959][YARN] Change YARN resource configs to use .amountJul 16, 2019

vanzin closed this in 43d68cdJul 16, 2019

attilapiros reviewed Jul 16, 2019
View reviewed changes

	(ResourceID(SPARK_EXECUTOR_PREFIX, "fpga").amountConf,
	s"${YARN_EXECUTOR_RESOURCE_TYPES_PREFIX}${YARN_FPGA_RESOURCE_CONFIG}"),
	(ResourceID(SPARK_DRIVER_PREFIX, "fpga").amountConf,
	s"${YARN_DRIVER_RESOURCE_TYPES_PREFIX}${YARN_FPGA_RESOURCE_CONFIG}"),
	(ResourceID(SPARK_EXECUTOR_PREFIX, "gpu").amountConf,
	s"${YARN_EXECUTOR_RESOURCE_TYPES_PREFIX}${YARN_GPU_RESOURCE_CONFIG}"),
	(ResourceID(SPARK_DRIVER_PREFIX, "gpu").amountConf,
	s"${YARN_DRIVER_RESOURCE_TYPES_PREFIX}${YARN_GPU_RESOURCE_CONFIG}"))

Uh oh!

Conversation

tgravescs commented Jun 27, 2019• edited by dongjoon-hyun Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

tgravescs commented Jun 27, 2019

Uh oh!

SparkQA commented Jun 27, 2019

Uh oh!

SparkQA commented Jun 28, 2019

Uh oh!

tgravescs commented Jul 8, 2019

Uh oh!

vanzin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tgravescs commented Jul 16, 2019

Uh oh!

SparkQA commented Jul 16, 2019

Uh oh!

dongjoon-hyun commented Jul 16, 2019

Uh oh!

vanzin commented Jul 16, 2019

Uh oh!

SparkQA commented Jul 16, 2019

Uh oh!

vanzin commented Jul 16, 2019

Uh oh!

attilapiros left a comment

Choose a reason for hiding this comment

Uh oh!

attilapirosJul 16, 2019

Choose a reason for hiding this comment

Uh oh!

attilapirosJul 16, 2019

Choose a reason for hiding this comment

Uh oh!

attilapiros commented Jul 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tgravescs commented Jun 27, 2019•
edited by dongjoon-hyun
Loading