[SPARK-34384][CORE] Add missing docs for ResourceProfile APIs #31496

Ngone51 · 2021-02-06T08:56:07Z

What changes were proposed in this pull request?

This PR adds missing docs for ResourceProfile related APIs. Besides, it includes a few minor changes on API:

ResourceProfileBuilder.build -> ResourceProfileBuilder.builder()
Provides java specific API allSupportedExecutorResourcesJList
private ResourceAllocator since it was mistakenly exposed previously

Why are the changes needed?

Add missing API docs

Does this PR introduce any user-facing change?

No, as Apache Spark 3.1 hasn't officially released.

How was this patch tested?

Updated unit tests due to the signature change of build().

Ngone51 · 2021-02-06T08:57:54Z

core/src/main/scala/org/apache/spark/resource/ResourceAllocator.scala

@@ -25,7 +25,7 @@ import org.apache.spark.SparkException
 * Trait used to help executor/worker allocate resources.
 * Please note that this is intended to be used in a single thread.
 */
-trait ResourceAllocator {
+private[spark] trait ResourceAllocator {


This's probably mistakenly exposed in 3.0.

Would this impact folks working on schedulers outside of org.apache.spark?

I think it wouldn't since there's no way to plug-in a custom ResourceAllocator in Spark yet.

core/src/main/scala/org/apache/spark/resource/ResourceProfileBuilder.scala

core/src/main/scala/org/apache/spark/resource/ResourceProfile.scala

core/src/main/scala/org/apache/spark/resource/ExecutorResourceRequests.scala

Ngone51 · 2021-02-06T09:11:11Z

cc @tgravescs @jiangxb1987 @mengxr

cc release manager @HyukjinKwon

core/src/main/scala/org/apache/spark/resource/ResourceProfile.scala

SparkQA · 2021-02-06T09:23:46Z

Test build #134953 has finished for PR 31496 at commit 199ce8d.

This patch fails Java style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-02-06T09:50:16Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39536/

SparkQA · 2021-02-06T10:19:00Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39536/

SparkQA · 2021-02-06T10:52:45Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39537/

SparkQA · 2021-02-06T11:21:27Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39537/

SparkQA · 2021-02-06T12:14:30Z

Test build #134954 has finished for PR 31496 at commit d248c8d.

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

SparkQA · 2021-02-07T03:26:35Z

Test build #134967 has started for PR 31496 at commit 8e72f87.

SparkQA · 2021-02-07T03:57:04Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39551/

SparkQA · 2021-02-07T04:01:28Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39551/

tgravescs · 2021-02-08T16:11:29Z

I'm -1 for any changes here except maybe docs... it is way to late in the release to be changing api's. This was reviewed and went in.

We have already put out an RC and this is not a blocking bug. I don't want last minute changes that were thought about to mess things up.

tgravescs · 2021-02-08T16:38:20Z

I'd really like to express my concern over this PR. There is a reason we go through a SPIP process and review the API's during design and PRs. Many things were discussed, sometimes compromises were made based on feedback, some things may be to allow supporting future enhancements, etc. I really hope this isn't happening in other places in the code.

HyukjinKwon · 2021-02-08T23:52:35Z

I don't think this blocks RC I am preparing now. Same here too. Hope we can do this earlier next time.

@tgravescs, though, some of them like https://github.com/apache/spark/pull/31496/files#diff-ee7e90474f1ce0390fce28f5e4d1d1be689c905ed13069bd869c8689a177e154R150 seems making sense. Maybe it wouldn't hurt to do a fine grained review.

For the API changes such as https://github.com/apache/spark/pull/31496/files#diff-a6d96a65d9905b310451b125acac6610ffbd6b4548461bd1d5a18dc29282814aL57, we might have to follow the standard deprecation process if we're going to change if we will do.

mengxr · 2021-02-09T02:04:39Z

@tgravescs I think there are two separate questions:

Does it block 3.1 release? I agree with you that it shouldn't. It is indeed too late in the release process and the proposed changes are not critical bugs.
Are the current APIs final? I think they are not. That is why they are labeled Evolving or DeveloperApi. So we still want to hear feedback from users, discuss them, and improve the APIs until we feel confident to mark them as stable.

So how about the following (none blocking 3.1 release):

In this PR, we complete the ScalaDoc and hide ResourceAllocator that should be marked private in 3.0. Seems everyone is okay with those changes.
I hope we can agree on the following minor changes:

Seq -> Array: https://github.com/apache/spark/pull/31496/files#diff-319a3f0dfd7de6045eb11ad960180230c47e6f52b1b27d3c9a1f2d72f1615d9dR284
build -> build() https://github.com/apache/spark/pull/31496/files#diff-a6d96a65d9905b310451b125acac6610ffbd6b4548461bd1d5a18dc29282814aR71
Remove unused (and untested?) methods from builder: https://github.com/apache/spark/pull/31496/files#r571388509 and https://github.com/apache/spark/pull/31496/files#r571388779

If we manage to get the above changes into 3.1 in time, I think it would improve API clarity and consistency. Then we can discuss other proposed renaming/refactoring after. Does it sound good?

tgravescs · 2021-02-09T03:56:19Z

The API as you state is Evolving and that is on purpose so we can extend and change as people use it and we learn more. I'm happy to hear the feedback and improve the API during normally development periods, at this point when we already had a 3.1 rc, it is not appropriate to throw API changes in last minute, most of them were discussed and chosen for a reason. This just leads to introducing more issues. I asked repeatedly for people to help review the SPIP and the code along the way - that was the time to help with API changes not right before a release goes out. I think this has happened way to much and I believe even brought up about the 3.0.0 release.

That said, non-api changes or obvious small issues in it should be fixed.
I'm fine with 1. and I'm fine with 2 for:
Seq -> Array: https://github.com/apache/spark/pull/31496/files#diff-319a3f0dfd7de6045eb11ad960180230c47e6f52b1b27d3c9a1f2d72f1615d9dR284
build -> build() https://github.com/apache/spark/pull/31496/files#diff-a6d96a65d9905b310451b125acac6610ffbd6b4548461bd1d5a18dc29282814aR71

The third point api's were intentionally added so I'm against that change at this point. It does look like I forgot to add automated tests for it, we can file a jira and I will add them. We shouldn't remove an api because tests were missed. This is user facing api so the fact its not used internally doesn't mean its not useful. You can argue whether a builder should have a clear/remove api, but it really depends on how users use this set of API's. I have seen many builders with remove apis.

mengxr · 2021-02-09T18:00:22Z

Sounds good. Let's do minimal changes in this PR and open two JIRAs as follow-ups: 1) complete the missing tests, 2) collect API feedback. cc: @Ngone51 @HyukjinKwon

HyukjinKwon · 2021-02-09T21:20:27Z

SGTM

holdenk

Did a quick skim, thanks for working on improving the javadocs :)

holdenk · 2021-02-18T01:24:29Z

core/src/main/scala/org/apache/spark/resource/ResourceAllocator.scala

@@ -25,7 +25,7 @@ import org.apache.spark.SparkException
 * Trait used to help executor/worker allocate resources.
 * Please note that this is intended to be used in a single thread.
 */
-trait ResourceAllocator {
+private[spark] trait ResourceAllocator {


Would this impact folks working on schedulers outside of org.apache.spark?

core/src/main/scala/org/apache/spark/resource/ResourceProfileBuilder.scala

Ngone51 · 2021-02-18T05:51:01Z

Thanks for all the feedback. I've updated PR to include only 2 API changes(Seq -> Array and build -> build()) as @tgravescs agreed. And also filed follow-up JIRAs: SPARK-34460, SPARK-34461

Please take another look, thanks!

SparkQA · 2021-02-18T06:45:06Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39800/

SparkQA · 2021-02-18T07:15:34Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39800/

SparkQA · 2021-02-18T08:41:26Z

Test build #135219 has finished for PR 31496 at commit ea08237.

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

tgravescs · 2021-02-18T15:01:23Z

can we please rename pr and Jira and update the description. It would be good for jira to have description as well.

core/src/main/scala/org/apache/spark/resource/ResourceProfileBuilder.scala

core/src/main/scala/org/apache/spark/resource/TaskResourceRequest.scala

Ngone51 · 2021-02-19T02:58:44Z

Thanks @tgravescs I have updated both PR and JIRA.

SparkQA · 2021-02-19T04:14:04Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39826/

SparkQA · 2021-02-19T05:43:36Z

Test build #135246 has finished for PR 31496 at commit 9e2bd4b.

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

SparkQA · 2021-02-19T05:49:43Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39826/

HyukjinKwon · 2021-02-19T06:57:38Z

@tgravescs and @holdenk, I plan to cut the RC as soon as possible (after the blocker #31550 is merged), but it seems like this PR includes two small changes that might matter in compatibility:

The changes look pretty fine to me, and I would like to merge this soon. I would appreciate if you guys take another look when you guys fine some time :-).

srowen · 2021-02-19T15:18:53Z

There is also marking ResourceAllocator as private[spark] - everyone is OK with that, to be clear?

tgravescs · 2021-02-19T15:32:24Z

yes I think marking ResourceAllocator as private is fine

Ngone51 · 2021-02-19T15:36:44Z

ResourceAllocator is mistakenly exposed previously. And there's no way to plug-in a custom ResourceAllocator into Spark. So it should be fine.

srowen · 2021-02-19T15:40:36Z

OK sounds good to merge for 3.1 before the RC? I'll do so if someone doesn't beat me to it.

core/src/main/scala/org/apache/spark/resource/ResourceProfile.scala

HyukjinKwon · 2021-02-20T03:53:05Z

core/src/main/scala/org/apache/spark/resource/ResourceProfile.scala

+   * Return all supported Spark built-in executor resources, custom resources like GPUs/FPGAs
+   * are excluded.
+   */
+  def allSupportedExecutorResources: Seq[String] = _allSupportedExecutorResources


Can we just def allSupportedExecutorResources: Array[String] = Array(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM) for both cases?

I see, this also looks ok to me.

This looks fine to me ... hope you are ok with it @tgravescs

SparkQA · 2021-02-20T04:53:05Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39869/

SparkQA · 2021-02-20T05:22:00Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39869/

SparkQA · 2021-02-20T06:44:44Z

Test build #135290 has finished for PR 31496 at commit 5ce6fbe.

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

SparkQA · 2021-02-20T09:27:26Z

Test build #135300 has finished for PR 31496 at commit 96d0760.

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

HyukjinKwon

Looks good. I will wait for @tgravescs's input at #31496 (comment) (I am going to cut RC on the first working day from now on).

HyukjinKwon · 2021-02-21T09:29:25Z

@tgravescs, I will merge this one I guess we'd be all fine with #31496 (comment) but will cut the RC in the next working day just in case you have a different thought.

HyukjinKwon · 2021-02-21T09:30:07Z

Merged to master and branch-3.1.

### What changes were proposed in this pull request? This PR adds missing docs for ResourceProfile related APIs. Besides, it includes a few minor changes on API: * ResourceProfileBuilder.build -> ResourceProfileBuilder.builder() * Provides java specific API `allSupportedExecutorResourcesJList` * private `ResourceAllocator` since it was mistakenly exposed previously ### Why are the changes needed? Add missing API docs ### Does this PR introduce _any_ user-facing change? No, as Apache Spark 3.1 hasn't officially released. ### How was this patch tested? Updated unit tests due to the signature change of `build()`. Closes #31496 from Ngone51/resource-profile-api-cleanup. Authored-by: yi.wu <yi.wu@databricks.com> Signed-off-by: HyukjinKwon <gurwls223@apache.org> (cherry picked from commit 546d2eb) Signed-off-by: HyukjinKwon <gurwls223@apache.org>

Ngone51 · 2021-02-21T09:32:29Z

Thanks all!!

api cleanup

199ce8d

github-actions bot added CORE KUBERNETES labels Feb 6, 2021

Ngone51 commented Feb 6, 2021

View reviewed changes

core/src/main/scala/org/apache/spark/resource/ResourceProfile.scala Outdated Show resolved Hide resolved

fix lint-java

d248c8d

fix doc of TaskResourceRequest

8e72f87

holdenk reviewed Feb 18, 2021

View reviewed changes

revert some api changes

ea08237

tgravescs reviewed Feb 18, 2021

View reviewed changes

address comments

9e2bd4b

Ngone51 changed the title ~~[SPARK-34384][CORE] API cleanup for ResourceProfile~~ [SPARK-34384][CORE] Improve javadoc for ResourceProfile APIs Feb 19, 2021

Ngone51 changed the title ~~[SPARK-34384][CORE] Improve javadoc for ResourceProfile APIs~~ [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs Feb 19, 2021

tgravescs approved these changes Feb 19, 2021

View reviewed changes

mridulm reviewed Feb 19, 2021

View reviewed changes

core/src/main/scala/org/apache/spark/resource/ResourceProfile.scala Outdated Show resolved Hide resolved

Ngone51 added 2 commits February 20, 2021 11:44

ensure immutable

12f5481

revert a unused import

5ce6fbe

HyukjinKwon reviewed Feb 20, 2021

View reviewed changes

address hyukjin's comment

96d0760

HyukjinKwon approved these changes Feb 21, 2021

View reviewed changes

HyukjinKwon closed this in 546d2eb Feb 21, 2021

[SPARK-34384][CORE] Add missing docs for ResourceProfile APIs #31496

[SPARK-34384][CORE] Add missing docs for ResourceProfile APIs #31496

Conversation

Ngone51 commented Feb 6, 2021 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ngone51 commented Feb 6, 2021

SparkQA commented Feb 6, 2021

SparkQA commented Feb 6, 2021

SparkQA commented Feb 6, 2021

SparkQA commented Feb 6, 2021

SparkQA commented Feb 6, 2021

SparkQA commented Feb 6, 2021

SparkQA commented Feb 7, 2021

SparkQA commented Feb 7, 2021

SparkQA commented Feb 7, 2021

tgravescs commented Feb 8, 2021

tgravescs commented Feb 8, 2021

HyukjinKwon commented Feb 8, 2021 • edited

mengxr commented Feb 9, 2021 • edited

tgravescs commented Feb 9, 2021

mengxr commented Feb 9, 2021

HyukjinKwon commented Feb 9, 2021

holdenk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ngone51 commented Feb 18, 2021 • edited

SparkQA commented Feb 18, 2021

SparkQA commented Feb 18, 2021

SparkQA commented Feb 18, 2021

tgravescs commented Feb 18, 2021

Ngone51 commented Feb 19, 2021

SparkQA commented Feb 19, 2021

SparkQA commented Feb 19, 2021

SparkQA commented Feb 19, 2021

HyukjinKwon commented Feb 19, 2021

srowen commented Feb 19, 2021

tgravescs commented Feb 19, 2021

Ngone51 commented Feb 19, 2021

srowen commented Feb 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Feb 20, 2021

SparkQA commented Feb 20, 2021

SparkQA commented Feb 20, 2021

SparkQA commented Feb 20, 2021

HyukjinKwon left a comment

Choose a reason for hiding this comment

HyukjinKwon commented Feb 21, 2021

HyukjinKwon commented Feb 21, 2021

Ngone51 commented Feb 21, 2021

Ngone51 commented Feb 6, 2021 •

edited

HyukjinKwon commented Feb 8, 2021 •

edited

mengxr commented Feb 9, 2021 •

edited

Ngone51 commented Feb 18, 2021 •

edited