tgravescs commented on code in PR #37268:
URL: https://github.com/apache/spark/pull/37268#discussion_r946847132
##########
core/src/main/scala/org/apache/spark/resource/ResourceProfileBuilder.scala:
##########
@@ -76,6 +79,17 @@ class ResourceProfileBuilder() {
this
}
+ /**
+ * Specify target resource profile type.
+ * @param isTaskResourceProfile will create a [[TaskResourceProfile]] if
true, otherwise a
+ * normal [[ResourceProfile]]
+ * @return This ResourceProfileBuilder
+ */
+ def taskOnly(isTaskResourceProfile: Boolean = true): this.type = {
Review Comment:
>In this PR, we add a new TaskResourceProfile, which is limited to some
scenarios like dynamic allocation off and it will reuse executors. So that we
may want user to explicitly specify it's a TaskResourceProfile to avoid
misunderstanding.
Why does this the user have to explicitly set it.. isn't it implicit if they
don't specify executor resources and they call build()? I guess you could
argue you could perhaps give them a better error message if that isn't really
what they intended, but that likely still becomes an issue with dynamic
allocation support. I'm fine with TaskResourceProfile being public as a
shortcut after building, especially if it makes the schedule code cleaner to
check instance type.
The only difference I can see between this and with dynamic allocation is
with dynamic allocation if you don't specify the executor resources it defaults
to use the initial resources but it gets a new executor based on the current
implementation behavior. if we support reuse executors with dynamic allocation
that restriction goes away and I would expect it to find any executor that
would fit - or like the linked issue we would specify some sort of reuse policy
that user could indicate which type of executors to reuse. Is there some
other use case you had in mind?
Please don't force push the code, it makes reviewing what changed much
harder.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
