You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/02/06 08:56:20 UTC

[GitHub] [spark] Ngone51 opened a new pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Ngone51 opened a new pull request #31496:
URL: https://github.com/apache/spark/pull/31496


   <!--
   Thanks for sending a pull request!  Here are some tips for you:
     1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
     2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
     3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
     4. Be sure to keep the PR description updated to reflect all changes.
     5. Please write your PR title to summarize what this PR proposes.
     6. If possible, provide a concise example to reproduce the issue for a faster review.
     7. If you want to add a new configuration, please read the guideline first for naming configurations in
        'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
   -->
   
   ### What changes were proposed in this pull request?
   <!--
   Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue. 
   If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
     1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
     2. If you fix some SQL features, you can provide some references of other DBMSes.
     3. If there is design documentation, please add the link.
     4. If there is a discussion in the mailing list, please add the link.
   -->
   This PR includes a few improvements for `ResourceProfile` related APIs:
   
   * Add or update ScalaDoc for APIs
   * Remove unnecessary APIs
   * Renaming some APIs for better user experience
   
   ### Why are the changes needed?
   <!--
   Please clarify why the changes are needed. For instance,
     1. If you propose a new API, clarify the use case for a new API.
     2. If you fix a bug, you can clarify why it is a bug.
   -->
   
   For better user experience
   
   
   ### Does this PR introduce _any_ user-facing change?
   <!--
   Note that it means *any* user-facing change including all aspects such as the documentation fix.
   If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
   If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
   If no, write 'No'.
   -->
   
   No, as Apache Spark 3.1 hasn't officially released.
   
   ### How was this patch tested?
   <!--
   If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
   If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
   If tests were not added, please describe why they were not added and/or why it was difficult to add.
   -->
   
   Updated unit tests.
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774466806


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/134954/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774466806


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/134954/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] HyukjinKwon commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-782826741


   @tgravescs, I will merge this one I guess we'd be all fine with https://github.com/apache/spark/pull/31496#discussion_r579589432 but will cut the RC in the next working day just in case you have a different thought.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-782563410


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/39869/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] HyukjinKwon commented on a change in pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #31496:
URL: https://github.com/apache/spark/pull/31496#discussion_r579587931



##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfile.scala
##########
@@ -242,17 +251,37 @@ class ResourceProfile(
 
 object ResourceProfile extends Logging {
   // task resources
+  /**
+   * built-in task resource: cpus
+   */
   val CPUS = "cpus"
   // Executor resources
   // Make sure add new executor resource in below allSupportedExecutorResources
+  /**
+   * built-in executor resource: cores
+   */
   val CORES = "cores"
+  /**
+   * built-in executor resource: cores
+   */
   val MEMORY = "memory"
+  /**
+   * built-in executor resource: offHeap
+   */
   val OFFHEAP_MEM = "offHeap"
+  /**
+   * built-in executor resource: memoryOverhead
+   */
   val OVERHEAD_MEM = "memoryOverhead"
+  /**
+   * built-in executor resource: pyspark.memory
+   */
   val PYSPARK_MEM = "pyspark.memory"
 
-  // all supported spark executor resources (minus the custom resources like GPUs/FPGAs)
-  val allSupportedExecutorResources = Seq(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)
+  /**
+   * All supported Spark built-in executor resources, custom resources like GPUs/FPGAs are excluded.
+   */
+  val allSupportedExecutorResources = Array(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)

Review comment:
       Oh @mridulm I think this is actually for Java users (to hide Scala specific classes to end Java users). I believe we have done this in places such as `Dataset`.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-781788234


   **[Test build #135246 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/135246/testReport)** for PR 31496 at commit [`9e2bd4b`](https://github.com/apache/spark/commit/9e2bd4b2fb59eca549baa88dc772e25833751e5c).


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-781087483


   **[Test build #135219 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/135219/testReport)** for PR 31496 at commit [`ea08237`](https://github.com/apache/spark/commit/ea0823708f5e4e24e5846344575613775ed14033).


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774442742


   **[Test build #134954 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/134954/testReport)** for PR 31496 at commit [`d248c8d`](https://github.com/apache/spark/commit/d248c8d75b0a0299985565823a10c7b1116a4eb1).


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-782594380


   **[Test build #135300 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/135300/testReport)** for PR 31496 at commit [`96d0760`](https://github.com/apache/spark/commit/96d07604b79c52bba4ff3aa4b892f363e73b2faa).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] HyukjinKwon commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-782826847


   Merged to master and branch-3.1.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-782576173


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/135290/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] HyukjinKwon commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-775543510


   I don't think 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.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] mengxr edited a comment on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
mengxr edited a comment on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-775599608


   @tgravescs I think there are two separate questions:
   
   1. 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.
   2. 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):
   
   1. 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.
   2. 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?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774465669


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/39537/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774440621


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/134953/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774590783


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/39551/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774457019


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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Ngone51 commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
Ngone51 commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-782153155


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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] mridulm commented on a change in pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
mridulm commented on a change in pull request #31496:
URL: https://github.com/apache/spark/pull/31496#discussion_r579614384



##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfile.scala
##########
@@ -242,17 +252,48 @@ class ResourceProfile(
 
 object ResourceProfile extends Logging {
   // task resources
+  /**
+   * built-in task resource: cpus
+   */
   val CPUS = "cpus"
   // Executor resources
   // Make sure add new executor resource in below allSupportedExecutorResources
+  /**
+   * built-in executor resource: cores
+   */
   val CORES = "cores"
+  /**
+   * built-in executor resource: cores
+   */
   val MEMORY = "memory"
+  /**
+   * built-in executor resource: offHeap
+   */
   val OFFHEAP_MEM = "offHeap"
+  /**
+   * built-in executor resource: memoryOverhead
+   */
   val OVERHEAD_MEM = "memoryOverhead"
+  /**
+   * built-in executor resource: pyspark.memory
+   */
   val PYSPARK_MEM = "pyspark.memory"
 
-  // all supported spark executor resources (minus the custom resources like GPUs/FPGAs)
-  val allSupportedExecutorResources = Seq(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)
+  private val _allSupportedExecutorResources =
+    Seq(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)
+
+  /**
+   * Return all supported Spark built-in executor resources, custom resources like GPUs/FPGAs
+   * are excluded.
+   */
+  def allSupportedExecutorResources: Seq[String] = _allSupportedExecutorResources

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




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] HyukjinKwon edited a comment on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
HyukjinKwon edited a comment on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-775543510


   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.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-782576564


   **[Test build #135300 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/135300/testReport)** for PR 31496 at commit [`96d0760`](https://github.com/apache/spark/commit/96d07604b79c52bba4ff3aa4b892f363e73b2faa).


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774585276


   **[Test build #134967 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/134967/testReport)** for PR 31496 at commit [`8e72f87`](https://github.com/apache/spark/commit/8e72f87c5efb03a4589663bf1637ebde030d9f0b).


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] HyukjinKwon removed a comment on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
HyukjinKwon removed a comment on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-781871010


   @tgravescs and @holdenk, I plan to cut the RC as soon as possible but it seems like this PR includes two small changes that might matter in compatibility:
   
   - https://github.com/apache/spark/pull/31496/files#diff-6f08c35f1a0ae172ac66c7a8148e2516f3f2ce4887b592a64fb8207055dbb4e3R28
   - https://github.com/apache/spark/pull/31496/files#diff-a6d96a65d9905b310451b125acac6610ffbd6b4548461bd1d5a18dc29282814aR95
   
   Would you mind taking a quick look please?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] tgravescs commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
tgravescs commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-775257493


   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.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774589737


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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-781112499


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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Ngone51 commented on a change in pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #31496:
URL: https://github.com/apache/spark/pull/31496#discussion_r579605235



##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfile.scala
##########
@@ -242,17 +252,48 @@ class ResourceProfile(
 
 object ResourceProfile extends Logging {
   // task resources
+  /**
+   * built-in task resource: cpus
+   */
   val CPUS = "cpus"
   // Executor resources
   // Make sure add new executor resource in below allSupportedExecutorResources
+  /**
+   * built-in executor resource: cores
+   */
   val CORES = "cores"
+  /**
+   * built-in executor resource: cores
+   */
   val MEMORY = "memory"
+  /**
+   * built-in executor resource: offHeap
+   */
   val OFFHEAP_MEM = "offHeap"
+  /**
+   * built-in executor resource: memoryOverhead
+   */
   val OVERHEAD_MEM = "memoryOverhead"
+  /**
+   * built-in executor resource: pyspark.memory
+   */
   val PYSPARK_MEM = "pyspark.memory"
 
-  // all supported spark executor resources (minus the custom resources like GPUs/FPGAs)
-  val allSupportedExecutorResources = Seq(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)
+  private val _allSupportedExecutorResources =
+    Seq(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)
+
+  /**
+   * Return all supported Spark built-in executor resources, custom resources like GPUs/FPGAs
+   * are excluded.
+   */
+  def allSupportedExecutorResources: Seq[String] = _allSupportedExecutorResources

Review comment:
       I see, this also looks ok to me.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774442019


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/39536/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-781788234


   **[Test build #135246 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/135246/testReport)** for PR 31496 at commit [`9e2bd4b`](https://github.com/apache/spark/commit/9e2bd4b2fb59eca549baa88dc772e25833751e5c).


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774466514


   **[Test build #134954 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/134954/testReport)** for PR 31496 at commit [`d248c8d`](https://github.com/apache/spark/commit/d248c8d75b0a0299985565823a10c7b1116a4eb1).
    * This patch **fails PySpark unit tests**.
    * This patch merges cleanly.
    * This patch adds no public classes.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-782573293


   **[Test build #135290 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/135290/testReport)** for PR 31496 at commit [`5ce6fbe`](https://github.com/apache/spark/commit/5ce6fbe62563f025bfae27f6c89043bad3d23b2a).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-781848149






----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] HyukjinKwon closed pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
HyukjinKwon closed pull request #31496:
URL: https://github.com/apache/spark/pull/31496


   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-781803797


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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] HyukjinKwon commented on a change in pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #31496:
URL: https://github.com/apache/spark/pull/31496#discussion_r579589432



##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfile.scala
##########
@@ -242,17 +252,48 @@ class ResourceProfile(
 
 object ResourceProfile extends Logging {
   // task resources
+  /**
+   * built-in task resource: cpus
+   */
   val CPUS = "cpus"
   // Executor resources
   // Make sure add new executor resource in below allSupportedExecutorResources
+  /**
+   * built-in executor resource: cores
+   */
   val CORES = "cores"
+  /**
+   * built-in executor resource: cores
+   */
   val MEMORY = "memory"
+  /**
+   * built-in executor resource: offHeap
+   */
   val OFFHEAP_MEM = "offHeap"
+  /**
+   * built-in executor resource: memoryOverhead
+   */
   val OVERHEAD_MEM = "memoryOverhead"
+  /**
+   * built-in executor resource: pyspark.memory
+   */
   val PYSPARK_MEM = "pyspark.memory"
 
-  // all supported spark executor resources (minus the custom resources like GPUs/FPGAs)
-  val allSupportedExecutorResources = Seq(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)
+  private val _allSupportedExecutorResources =
+    Seq(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)
+
+  /**
+   * Return all supported Spark built-in executor resources, custom resources like GPUs/FPGAs
+   * are excluded.
+   */
+  def allSupportedExecutorResources: Seq[String] = _allSupportedExecutorResources

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




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Ngone51 edited a comment on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
Ngone51 edited a comment on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-781074838


   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! 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-782591826


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/39879/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-782599592


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/135300/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-781841961


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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774430515


   **[Test build #134953 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/134953/testReport)** for PR 31496 at commit [`199ce8d`](https://github.com/apache/spark/commit/199ce8d73436fb92023eee507afcc4f0b4d7305e).


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774465669


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/39537/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] mengxr commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
mengxr commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-775599608


   @tgravescs I think there are two separate questions:
   
   1. 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.
   2. 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):
   
   1. 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.
   2. 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 clarify/consistency. Then we can discuss other proposed renaming/refactoring after. Does it sound good?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-781098064


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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Ngone51 commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
Ngone51 commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-781779555


   Thanks @tgravescs I have updated both PR and JIRA.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-782563403


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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] mengxr commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
mengxr commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-776127620


   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 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-782563410


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/39869/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] tgravescs commented on a change in pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
tgravescs commented on a change in pull request #31496:
URL: https://github.com/apache/spark/pull/31496#discussion_r572172451



##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfile.scala
##########
@@ -242,20 +251,40 @@ class ResourceProfile(
 
 object ResourceProfile extends Logging {
   // task resources
+  /**
+   * built-in task resource: cpus
+   */
   val CPUS = "cpus"
   // Executor resources
   // Make sure add new executor resource in below allSupportedExecutorResources
+  /**
+   * built-in executor resource: cores
+   */
   val CORES = "cores"
+  /**
+   * built-in executor resource: cores
+   */
   val MEMORY = "memory"
+  /**
+   * built-in executor resource: offHeap
+   */
   val OFFHEAP_MEM = "offHeap"
+  /**
+   * built-in executor resource: memoryOverhead
+   */
   val OVERHEAD_MEM = "memoryOverhead"
+  /**
+   * built-in executor resource: pyspark.memory
+   */
   val PYSPARK_MEM = "pyspark.memory"
 
-  // all supported spark executor resources (minus the custom resources like GPUs/FPGAs)
-  val allSupportedExecutorResources = Seq(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)
+  /**
+   * All supported Spark built-in executor resources, custom resources like GPUs/FPGAs are excluded.
+   */
+  val allSupportedExecutorResources = Array(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)
 
-  val UNKNOWN_RESOURCE_PROFILE_ID = -1
-  val DEFAULT_RESOURCE_PROFILE_ID = 0
+  private[spark] val UNKNOWN_RESOURCE_PROFILE_ID = -1
+  private[spark] val DEFAULT_RESOURCE_PROFILE_ID = 0

Review comment:
       yes it should be public in case users want to track it and easily compare




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Ngone51 commented on a change in pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #31496:
URL: https://github.com/apache/spark/pull/31496#discussion_r578146307



##########
File path: 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 {

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




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-782557663


   **[Test build #135290 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/135290/testReport)** for PR 31496 at commit [`5ce6fbe`](https://github.com/apache/spark/commit/5ce6fbe62563f025bfae27f6c89043bad3d23b2a).


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] HyukjinKwon commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-781874079


   @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:
   
   - https://github.com/apache/spark/pull/31496/files#diff-6f08c35f1a0ae172ac66c7a8148e2516f3f2ce4887b592a64fb8207055dbb4e3R28
   - https://github.com/apache/spark/pull/31496/files#diff-a6d96a65d9905b310451b125acac6610ffbd6b4548461bd1d5a18dc29282814aR95
   
   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 :-).


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] tgravescs commented on a change in pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
tgravescs commented on a change in pull request #31496:
URL: https://github.com/apache/spark/pull/31496#discussion_r578494381



##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfileBuilder.scala
##########
@@ -26,17 +26,19 @@ import org.apache.spark.annotation.{Evolving, Since}
 
 
 /**
- * Resource profile builder to build a Resource profile to associate with an RDD.
- * A ResourceProfile allows the user to specify executor and task requirements for an RDD
- * that will get applied during a stage. This allows the user to change the resource
+ * Resource profile builder to build a [[ResourceProfile]] to associate with an RDD.
+ * A [[ResourceProfile]] allows the user to specify executor and task resource requirements
+ * for an RDD that will get applied during a stage. This allows the user to change the resource
  * requirements between stages.
  *
  */
 @Evolving
 @Since("3.1.0")
 class ResourceProfileBuilder() {
 
+  // Task resource requests that specified by users, mapped from resource name to the request.
   private val _taskResources = new ConcurrentHashMap[String, TaskResourceRequest]()
+  // Executor resource requests that specified by users, mapped from resource name to the request.

Review comment:
       nit remove "that"

##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfileBuilder.scala
##########
@@ -26,17 +26,19 @@ import org.apache.spark.annotation.{Evolving, Since}
 
 
 /**
- * Resource profile builder to build a Resource profile to associate with an RDD.
- * A ResourceProfile allows the user to specify executor and task requirements for an RDD
- * that will get applied during a stage. This allows the user to change the resource
+ * Resource profile builder to build a [[ResourceProfile]] to associate with an RDD.
+ * A [[ResourceProfile]] allows the user to specify executor and task resource requirements
+ * for an RDD that will get applied during a stage. This allows the user to change the resource
  * requirements between stages.
  *
  */
 @Evolving
 @Since("3.1.0")
 class ResourceProfileBuilder() {
 
+  // Task resource requests that specified by users, mapped from resource name to the request.

Review comment:
       remove "that"

##########
File path: core/src/main/scala/org/apache/spark/resource/TaskResourceRequest.scala
##########
@@ -20,11 +20,15 @@ package org.apache.spark.resource
 import org.apache.spark.annotation.{Evolving, Since}
 
 /**
- * A task resource request. This is used in conjunction with the ResourceProfile to
+ * A task resource request. This is used in conjunction with the [[ResourceProfile]] to
  * programmatically specify the resources needed for an RDD that will be applied at the
  * stage level.
  *
- * Use TaskResourceRequests class as a convenience API.
+ * Use [[TaskResourceRequests]] class as a convenience API.
+ *
+ * @param resourceName Resource name
+ * @param amount Expected amount of the resource, must be either less than or equal to 0.5

Review comment:
       perhaps should clarify to be similar to text in TaskResourceRequests:
   
    @param amount Amount requesting as a Double to support fractional resource requests.
                    Valid values are less than or equal to 0.5 or whole numbers. This essentially
                    lets you configure X number of tasks to run on a single resource,
                    ie amount equals 0.5 translates into 2 tasks per resource address.
      

##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfileBuilder.scala
##########
@@ -54,11 +56,21 @@ class ResourceProfileBuilder() {
     _executorResources.asScala.asJava
   }
 
+  /**
+   * Add executor resource requests
+   * @param requests The detailed executor resource requests, see [[ExecutorResourceRequests]]
+   * @return this.type

Review comment:
       perhaps better to say return "This ResourceProfileBuilder"

##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfileBuilder.scala
##########
@@ -54,11 +56,21 @@ class ResourceProfileBuilder() {
     _executorResources.asScala.asJava
   }
 
+  /**
+   * Add executor resource requests
+   * @param requests The detailed executor resource requests, see [[ExecutorResourceRequests]]
+   * @return this.type
+   */
   def require(requests: ExecutorResourceRequests): this.type = {
     _executorResources.putAll(requests.requests.asJava)
     this
   }
 
+  /**
+   * Add task resource requests
+   * @param requests The detailed task resource requests, see [[TaskResourceRequest]]
+   * @return this.type

Review comment:
       perhaps better to say return "This ResourceProfileBuilder"




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] HyukjinKwon commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-781871010


   @tgravescs and @holdenk, I plan to cut the RC as soon as possible but it seems like this PR includes two small changes that might matter in compatibility:
   
   - https://github.com/apache/spark/pull/31496/files#diff-6f08c35f1a0ae172ac66c7a8148e2516f3f2ce4887b592a64fb8207055dbb4e3R28
   - https://github.com/apache/spark/pull/31496/files#diff-a6d96a65d9905b310451b125acac6610ffbd6b4548461bd1d5a18dc29282814aR95
   
   Would you mind taking a quick look please?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774442012


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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Ngone51 commented on a change in pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #31496:
URL: https://github.com/apache/spark/pull/31496#discussion_r578148676



##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfileBuilder.scala
##########
@@ -17,71 +17,67 @@
 
 package org.apache.spark.resource
 
-import java.util.{Map => JMap}
 import java.util.concurrent.ConcurrentHashMap
 
 import scala.collection.JavaConverters._
 
+import org.apache.spark.SparkException
 import org.apache.spark.annotation.{Evolving, Since}
+import org.apache.spark.util.Utils
 
 
 /**
- * Resource profile builder to build a Resource profile to associate with an RDD.
- * A ResourceProfile allows the user to specify executor and task requirements for an RDD
- * that will get applied during a stage. This allows the user to change the resource
+ * Resource profile builder to build a [[ResourceProfile]] to associate with an RDD.
+ * A [[ResourceProfile]] allows the user to specify executor and task resource requirements
+ * for an RDD that will get applied during a stage. This allows the user to change the resource
  * requirements between stages.
  *
  */
 @Evolving
 @Since("3.1.0")
 class ResourceProfileBuilder() {
 
+  // Task resource requests that specified by users, mapped from resource name to the request.
   private val _taskResources = new ConcurrentHashMap[String, TaskResourceRequest]()
+  // Executor resource requests that specified by users, mapped from resource name to the request.
   private val _executorResources = new ConcurrentHashMap[String, ExecutorResourceRequest]()
 
-  def taskResources: Map[String, TaskResourceRequest] = _taskResources.asScala.toMap
-  def executorResources: Map[String, ExecutorResourceRequest] = _executorResources.asScala.toMap
-
-  /**
-   * (Java-specific) gets a Java Map of resources to TaskResourceRequest
-   */
-  def taskResourcesJMap: JMap[String, TaskResourceRequest] = _taskResources.asScala.asJava
-
   /**
-   * (Java-specific) gets a Java Map of resources to ExecutorResourceRequest
+   * Add executor resource requests
+   * @param requests The detailed executor resource requests, see [[ExecutorResourceRequests]]
+   * @return this.type
    */
-  def executorResourcesJMap: JMap[String, ExecutorResourceRequest] = {
-    _executorResources.asScala.asJava
-  }
-
-  def require(requests: ExecutorResourceRequests): this.type = {
+  def executorRequire(requests: ExecutorResourceRequests): this.type = {
     _executorResources.putAll(requests.requests.asJava)
     this
   }
 
-  def require(requests: TaskResourceRequests): this.type = {
+  /**
+   * Add task resource requests
+   * @param requests The detailed task resource requests, see [[TaskResourceRequest]]
+   * @return this.type
+   */
+  def taskRequire(requests: TaskResourceRequests): this.type = {
     _taskResources.putAll(requests.requests.asJava)
     this
   }
 
-  def clearExecutorResourceRequests(): this.type = {
-    _executorResources.clear()
-    this
-  }
-
-  def clearTaskResourceRequests(): this.type = {
-    _taskResources.clear()
-    this
-  }
-
   override def toString(): String = {
     "Profile executor resources: " +
       s"${_executorResources.asScala.map(pair => s"${pair._1}=${pair._2.toString()}")}, " +
       s"task resources: ${_taskResources.asScala.map(pair => s"${pair._1}=${pair._2.toString()}")}"
   }
 
-  def build: ResourceProfile = {
-    new ResourceProfile(executorResources, taskResources)
+  def build(): ResourceProfile = {
+    if (!Utils.isTesting) {
+      if (_taskResources.isEmpty) {
+        throw new SparkException("Empty task resource request.")
+      }
+      if (_executorResources.isEmpty) {
+        throw new SparkException("Empty executor resource request.")
+      }

Review comment:
       I think users should at least assign cores to executors, otherwise, executors can fail to launch.
   
   I have reverted this change to make this PR clean according to @tgravescs 's feedback.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-782557663


   **[Test build #135290 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/135290/testReport)** for PR 31496 at commit [`5ce6fbe`](https://github.com/apache/spark/commit/5ce6fbe62563f025bfae27f6c89043bad3d23b2a).


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] mridulm commented on a change in pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
mridulm commented on a change in pull request #31496:
URL: https://github.com/apache/spark/pull/31496#discussion_r579495725



##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfile.scala
##########
@@ -242,17 +251,37 @@ class ResourceProfile(
 
 object ResourceProfile extends Logging {
   // task resources
+  /**
+   * built-in task resource: cpus
+   */
   val CPUS = "cpus"
   // Executor resources
   // Make sure add new executor resource in below allSupportedExecutorResources
+  /**
+   * built-in executor resource: cores
+   */
   val CORES = "cores"
+  /**
+   * built-in executor resource: cores
+   */
   val MEMORY = "memory"
+  /**
+   * built-in executor resource: offHeap
+   */
   val OFFHEAP_MEM = "offHeap"
+  /**
+   * built-in executor resource: memoryOverhead
+   */
   val OVERHEAD_MEM = "memoryOverhead"
+  /**
+   * built-in executor resource: pyspark.memory
+   */
   val PYSPARK_MEM = "pyspark.memory"
 
-  // all supported spark executor resources (minus the custom resources like GPUs/FPGAs)
-  val allSupportedExecutorResources = Seq(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)
+  /**
+   * All supported Spark built-in executor resources, custom resources like GPUs/FPGAs are excluded.
+   */
+  val allSupportedExecutorResources = Array(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)

Review comment:
       Why did we change this to Array ?
   Seq is immutable, while Array allows updates (accidental or otherwise).
   Can we revert this ?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] srowen commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-782155494


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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774442019


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/39536/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Ngone51 commented on a change in pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #31496:
URL: https://github.com/apache/spark/pull/31496#discussion_r579605207



##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfile.scala
##########
@@ -242,17 +252,48 @@ class ResourceProfile(
 
 object ResourceProfile extends Logging {
   // task resources
+  /**
+   * built-in task resource: cpus
+   */
   val CPUS = "cpus"
   // Executor resources
   // Make sure add new executor resource in below allSupportedExecutorResources
+  /**
+   * built-in executor resource: cores
+   */
   val CORES = "cores"
+  /**
+   * built-in executor resource: cores
+   */
   val MEMORY = "memory"
+  /**
+   * built-in executor resource: offHeap
+   */
   val OFFHEAP_MEM = "offHeap"
+  /**
+   * built-in executor resource: memoryOverhead
+   */
   val OVERHEAD_MEM = "memoryOverhead"
+  /**
+   * built-in executor resource: pyspark.memory
+   */
   val PYSPARK_MEM = "pyspark.memory"
 
-  // all supported spark executor resources (minus the custom resources like GPUs/FPGAs)
-  val allSupportedExecutorResources = Seq(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)
+  private val _allSupportedExecutorResources =
+    Seq(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)
+
+  /**
+   * Return all supported Spark built-in executor resources, custom resources like GPUs/FPGAs
+   * are excluded.
+   */
+  def allSupportedExecutorResources: Seq[String] = _allSupportedExecutorResources

Review comment:
       I see, this also looks ok to me.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] tgravescs commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
tgravescs commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-781404497


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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774598906


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/134967/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] tgravescs commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
tgravescs commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-775643486


   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.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774432154


   **[Test build #134953 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/134953/testReport)** for PR 31496 at commit [`199ce8d`](https://github.com/apache/spark/commit/199ce8d73436fb92023eee507afcc4f0b4d7305e).
    * This patch **fails Java style tests**.
    * This patch merges cleanly.
    * This patch adds no public classes.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774430515


   **[Test build #134953 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/134953/testReport)** for PR 31496 at commit [`199ce8d`](https://github.com/apache/spark/commit/199ce8d73436fb92023eee507afcc4f0b4d7305e).


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] HyukjinKwon commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-776251685


   SGTM


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-782560231


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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Ngone51 commented on a change in pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #31496:
URL: https://github.com/apache/spark/pull/31496#discussion_r579588923



##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfile.scala
##########
@@ -242,17 +251,37 @@ class ResourceProfile(
 
 object ResourceProfile extends Logging {
   // task resources
+  /**
+   * built-in task resource: cpus
+   */
   val CPUS = "cpus"
   // Executor resources
   // Make sure add new executor resource in below allSupportedExecutorResources
+  /**
+   * built-in executor resource: cores
+   */
   val CORES = "cores"
+  /**
+   * built-in executor resource: cores
+   */
   val MEMORY = "memory"
+  /**
+   * built-in executor resource: offHeap
+   */
   val OFFHEAP_MEM = "offHeap"
+  /**
+   * built-in executor resource: memoryOverhead
+   */
   val OVERHEAD_MEM = "memoryOverhead"
+  /**
+   * built-in executor resource: pyspark.memory
+   */
   val PYSPARK_MEM = "pyspark.memory"
 
-  // all supported spark executor resources (minus the custom resources like GPUs/FPGAs)
-  val allSupportedExecutorResources = Seq(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)
+  /**
+   * All supported Spark built-in executor resources, custom resources like GPUs/FPGAs are excluded.
+   */
+  val allSupportedExecutorResources = Array(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)

Review comment:
       Make sense to me. We changed to `Array` for Java-friendly purpose. 
   
   I have added the java specific func to address the immutable concern: 12f5481
   
   But this also introduces a new API, @tgravescs  are you ok with it?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] mridulm commented on a change in pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
mridulm commented on a change in pull request #31496:
URL: https://github.com/apache/spark/pull/31496#discussion_r579495725



##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfile.scala
##########
@@ -242,17 +251,37 @@ class ResourceProfile(
 
 object ResourceProfile extends Logging {
   // task resources
+  /**
+   * built-in task resource: cpus
+   */
   val CPUS = "cpus"
   // Executor resources
   // Make sure add new executor resource in below allSupportedExecutorResources
+  /**
+   * built-in executor resource: cores
+   */
   val CORES = "cores"
+  /**
+   * built-in executor resource: cores
+   */
   val MEMORY = "memory"
+  /**
+   * built-in executor resource: offHeap
+   */
   val OFFHEAP_MEM = "offHeap"
+  /**
+   * built-in executor resource: memoryOverhead
+   */
   val OVERHEAD_MEM = "memoryOverhead"
+  /**
+   * built-in executor resource: pyspark.memory
+   */
   val PYSPARK_MEM = "pyspark.memory"
 
-  // all supported spark executor resources (minus the custom resources like GPUs/FPGAs)
-  val allSupportedExecutorResources = Seq(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)
+  /**
+   * All supported Spark built-in executor resources, custom resources like GPUs/FPGAs are excluded.
+   */
+  val allSupportedExecutorResources = Array(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)

Review comment:
       Why did we change this to Array ?
   Seq is immutable, while Array allows updates (accidental or otherwise).
   Can we revert this ? Or use some other immutable collection.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-781133188


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/39800/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-781839349


   **[Test build #135246 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/135246/testReport)** for PR 31496 at commit [`9e2bd4b`](https://github.com/apache/spark/commit/9e2bd4b2fb59eca549baa88dc772e25833751e5c).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-782591826


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/39879/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] tgravescs commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
tgravescs commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-782150453


   yes I think marking ResourceAllocator as private is fine


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774589368


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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-782599592


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/135300/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-782576173


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/135290/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-781133188


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/39800/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Ngone51 commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
Ngone51 commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774430459


   
   cc @tgravescs @jiangxb1987 @mengxr 
   
   cc release manager @HyukjinKwon 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Ngone51 commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
Ngone51 commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-782827127


   Thanks all!!


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774446882


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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] tgravescs commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
tgravescs commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-775276262


   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.
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Ngone51 commented on a change in pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #31496:
URL: https://github.com/apache/spark/pull/31496#discussion_r571388509



##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfileBuilder.scala
##########
@@ -17,71 +17,67 @@
 
 package org.apache.spark.resource
 
-import java.util.{Map => JMap}
 import java.util.concurrent.ConcurrentHashMap
 
 import scala.collection.JavaConverters._
 
+import org.apache.spark.SparkException
 import org.apache.spark.annotation.{Evolving, Since}
+import org.apache.spark.util.Utils
 
 
 /**
- * Resource profile builder to build a Resource profile to associate with an RDD.
- * A ResourceProfile allows the user to specify executor and task requirements for an RDD
- * that will get applied during a stage. This allows the user to change the resource
+ * Resource profile builder to build a [[ResourceProfile]] to associate with an RDD.
+ * A [[ResourceProfile]] allows the user to specify executor and task resource requirements
+ * for an RDD that will get applied during a stage. This allows the user to change the resource
  * requirements between stages.
  *
  */
 @Evolving
 @Since("3.1.0")
 class ResourceProfileBuilder() {
 
+  // Task resource requests that specified by users, mapped from resource name to the request.
   private val _taskResources = new ConcurrentHashMap[String, TaskResourceRequest]()
+  // Executor resource requests that specified by users, mapped from resource name to the request.
   private val _executorResources = new ConcurrentHashMap[String, ExecutorResourceRequest]()
 
-  def taskResources: Map[String, TaskResourceRequest] = _taskResources.asScala.toMap
-  def executorResources: Map[String, ExecutorResourceRequest] = _executorResources.asScala.toMap
-
-  /**
-   * (Java-specific) gets a Java Map of resources to TaskResourceRequest
-   */
-  def taskResourcesJMap: JMap[String, TaskResourceRequest] = _taskResources.asScala.asJava

Review comment:
       These resources should be available after we get the `ResourceProfile` after `build()`, and it's more reasonable to get them from `ResourceProfile` instead of `ResourceProfileBuilder`.

##########
File path: core/src/main/scala/org/apache/spark/resource/ExecutorResourceRequests.scala
##########
@@ -132,6 +144,14 @@ class ExecutorResourceRequests() extends Serializable {
     this
   }
 
+  /**
+   * Add a certain [[ExecutorResourceRequest]] to the request set.
+   */
+  def addRequest(treq: ExecutorResourceRequest): this.type = {

Review comment:
       Add this as same to what `TaskResourceRequest` does. 

##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfileBuilder.scala
##########
@@ -17,71 +17,67 @@
 
 package org.apache.spark.resource
 
-import java.util.{Map => JMap}
 import java.util.concurrent.ConcurrentHashMap
 
 import scala.collection.JavaConverters._
 
+import org.apache.spark.SparkException
 import org.apache.spark.annotation.{Evolving, Since}
+import org.apache.spark.util.Utils
 
 
 /**
- * Resource profile builder to build a Resource profile to associate with an RDD.
- * A ResourceProfile allows the user to specify executor and task requirements for an RDD
- * that will get applied during a stage. This allows the user to change the resource
+ * Resource profile builder to build a [[ResourceProfile]] to associate with an RDD.
+ * A [[ResourceProfile]] allows the user to specify executor and task resource requirements
+ * for an RDD that will get applied during a stage. This allows the user to change the resource
  * requirements between stages.
  *
  */
 @Evolving
 @Since("3.1.0")
 class ResourceProfileBuilder() {
 
+  // Task resource requests that specified by users, mapped from resource name to the request.
   private val _taskResources = new ConcurrentHashMap[String, TaskResourceRequest]()
+  // Executor resource requests that specified by users, mapped from resource name to the request.
   private val _executorResources = new ConcurrentHashMap[String, ExecutorResourceRequest]()
 
-  def taskResources: Map[String, TaskResourceRequest] = _taskResources.asScala.toMap
-  def executorResources: Map[String, ExecutorResourceRequest] = _executorResources.asScala.toMap
-
-  /**
-   * (Java-specific) gets a Java Map of resources to TaskResourceRequest
-   */
-  def taskResourcesJMap: JMap[String, TaskResourceRequest] = _taskResources.asScala.asJava
-
   /**
-   * (Java-specific) gets a Java Map of resources to ExecutorResourceRequest
+   * Add executor resource requests
+   * @param requests The detailed executor resource requests, see [[ExecutorResourceRequests]]
+   * @return this.type
    */
-  def executorResourcesJMap: JMap[String, ExecutorResourceRequest] = {
-    _executorResources.asScala.asJava
-  }
-
-  def require(requests: ExecutorResourceRequests): this.type = {
+  def executorRequire(requests: ExecutorResourceRequests): this.type = {
     _executorResources.putAll(requests.requests.asJava)
     this
   }
 
-  def require(requests: TaskResourceRequests): this.type = {
+  /**
+   * Add task resource requests
+   * @param requests The detailed task resource requests, see [[TaskResourceRequest]]
+   * @return this.type
+   */
+  def taskRequire(requests: TaskResourceRequests): this.type = {
     _taskResources.putAll(requests.requests.asJava)
     this
   }
 
-  def clearExecutorResourceRequests(): this.type = {
-    _executorResources.clear()
-    this
-  }
-
-  def clearTaskResourceRequests(): this.type = {
-    _taskResources.clear()
-    this
-  }

Review comment:
       These 2 are not used anywhere. And I think it's useless for a `builder`.

##########
File path: 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 {

Review comment:
       This's probably mistakenly exposed in 3.0.

##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfileBuilder.scala
##########
@@ -17,71 +17,67 @@
 
 package org.apache.spark.resource
 
-import java.util.{Map => JMap}
 import java.util.concurrent.ConcurrentHashMap
 
 import scala.collection.JavaConverters._
 
+import org.apache.spark.SparkException
 import org.apache.spark.annotation.{Evolving, Since}
+import org.apache.spark.util.Utils
 
 
 /**
- * Resource profile builder to build a Resource profile to associate with an RDD.
- * A ResourceProfile allows the user to specify executor and task requirements for an RDD
- * that will get applied during a stage. This allows the user to change the resource
+ * Resource profile builder to build a [[ResourceProfile]] to associate with an RDD.
+ * A [[ResourceProfile]] allows the user to specify executor and task resource requirements
+ * for an RDD that will get applied during a stage. This allows the user to change the resource
  * requirements between stages.
  *
  */
 @Evolving
 @Since("3.1.0")
 class ResourceProfileBuilder() {
 
+  // Task resource requests that specified by users, mapped from resource name to the request.
   private val _taskResources = new ConcurrentHashMap[String, TaskResourceRequest]()
+  // Executor resource requests that specified by users, mapped from resource name to the request.
   private val _executorResources = new ConcurrentHashMap[String, ExecutorResourceRequest]()
 
-  def taskResources: Map[String, TaskResourceRequest] = _taskResources.asScala.toMap
-  def executorResources: Map[String, ExecutorResourceRequest] = _executorResources.asScala.toMap
-
-  /**
-   * (Java-specific) gets a Java Map of resources to TaskResourceRequest
-   */
-  def taskResourcesJMap: JMap[String, TaskResourceRequest] = _taskResources.asScala.asJava
-
   /**
-   * (Java-specific) gets a Java Map of resources to ExecutorResourceRequest
+   * Add executor resource requests
+   * @param requests The detailed executor resource requests, see [[ExecutorResourceRequests]]
+   * @return this.type
    */
-  def executorResourcesJMap: JMap[String, ExecutorResourceRequest] = {
-    _executorResources.asScala.asJava
-  }
-
-  def require(requests: ExecutorResourceRequests): this.type = {
+  def executorRequire(requests: ExecutorResourceRequests): this.type = {
     _executorResources.putAll(requests.requests.asJava)
     this
   }
 
-  def require(requests: TaskResourceRequests): this.type = {
+  /**
+   * Add task resource requests
+   * @param requests The detailed task resource requests, see [[TaskResourceRequest]]
+   * @return this.type
+   */
+  def taskRequire(requests: TaskResourceRequests): this.type = {

Review comment:
       Renamed `require` to `taskRequire` and `executorRequire` separately. So users can understand usage better from the naming.

##########
File path: core/src/main/scala/org/apache/spark/resource/ExecutorResourceRequests.scala
##########
@@ -132,6 +144,14 @@ class ExecutorResourceRequests() extends Serializable {
     this
   }
 
+  /**
+   * Add a certain [[ExecutorResourceRequest]] to the request set.
+   */
+  def addRequest(treq: ExecutorResourceRequest): this.type = {

Review comment:
       Actually, I'm thinking do we really need to expose  `TaskResourceRequest` as a public API? As users can always add requests from the above convenient APIs. cc @tgravescs 

##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfile.scala
##########
@@ -242,20 +251,40 @@ class ResourceProfile(
 
 object ResourceProfile extends Logging {
   // task resources
+  /**
+   * built-in task resource: cpus
+   */
   val CPUS = "cpus"
   // Executor resources
   // Make sure add new executor resource in below allSupportedExecutorResources
+  /**
+   * built-in executor resource: cores
+   */
   val CORES = "cores"
+  /**
+   * built-in executor resource: cores
+   */
   val MEMORY = "memory"
+  /**
+   * built-in executor resource: offHeap
+   */
   val OFFHEAP_MEM = "offHeap"
+  /**
+   * built-in executor resource: memoryOverhead
+   */
   val OVERHEAD_MEM = "memoryOverhead"
+  /**
+   * built-in executor resource: pyspark.memory
+   */
   val PYSPARK_MEM = "pyspark.memory"
 
-  // all supported spark executor resources (minus the custom resources like GPUs/FPGAs)
-  val allSupportedExecutorResources = Seq(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)
+  /**
+   * All supported Spark built-in executor resources, custom resources like GPUs/FPGAs are excluded.
+   */
+  val allSupportedExecutorResources = Array(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)
 
-  val UNKNOWN_RESOURCE_PROFILE_ID = -1
-  val DEFAULT_RESOURCE_PROFILE_ID = 0
+  private[spark] val UNKNOWN_RESOURCE_PROFILE_ID = -1
+  private[spark] val DEFAULT_RESOURCE_PROFILE_ID = 0

Review comment:
       No need to expose these two, right? I'm also thinking about whether it's necessary to expose the method `ResourceProfile.id`. cc @tgravescs 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] holdenk commented on a change in pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
holdenk commented on a change in pull request #31496:
URL: https://github.com/apache/spark/pull/31496#discussion_r578061391



##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfileBuilder.scala
##########
@@ -17,71 +17,67 @@
 
 package org.apache.spark.resource
 
-import java.util.{Map => JMap}
 import java.util.concurrent.ConcurrentHashMap
 
 import scala.collection.JavaConverters._
 
+import org.apache.spark.SparkException
 import org.apache.spark.annotation.{Evolving, Since}
+import org.apache.spark.util.Utils
 
 
 /**
- * Resource profile builder to build a Resource profile to associate with an RDD.
- * A ResourceProfile allows the user to specify executor and task requirements for an RDD
- * that will get applied during a stage. This allows the user to change the resource
+ * Resource profile builder to build a [[ResourceProfile]] to associate with an RDD.
+ * A [[ResourceProfile]] allows the user to specify executor and task resource requirements
+ * for an RDD that will get applied during a stage. This allows the user to change the resource
  * requirements between stages.
  *
  */
 @Evolving
 @Since("3.1.0")
 class ResourceProfileBuilder() {
 
+  // Task resource requests that specified by users, mapped from resource name to the request.
   private val _taskResources = new ConcurrentHashMap[String, TaskResourceRequest]()
+  // Executor resource requests that specified by users, mapped from resource name to the request.
   private val _executorResources = new ConcurrentHashMap[String, ExecutorResourceRequest]()
 
-  def taskResources: Map[String, TaskResourceRequest] = _taskResources.asScala.toMap
-  def executorResources: Map[String, ExecutorResourceRequest] = _executorResources.asScala.toMap
-
-  /**
-   * (Java-specific) gets a Java Map of resources to TaskResourceRequest
-   */
-  def taskResourcesJMap: JMap[String, TaskResourceRequest] = _taskResources.asScala.asJava
-
   /**
-   * (Java-specific) gets a Java Map of resources to ExecutorResourceRequest
+   * Add executor resource requests
+   * @param requests The detailed executor resource requests, see [[ExecutorResourceRequests]]
+   * @return this.type
    */
-  def executorResourcesJMap: JMap[String, ExecutorResourceRequest] = {
-    _executorResources.asScala.asJava
-  }
-
-  def require(requests: ExecutorResourceRequests): this.type = {
+  def executorRequire(requests: ExecutorResourceRequests): this.type = {
     _executorResources.putAll(requests.requests.asJava)
     this
   }
 
-  def require(requests: TaskResourceRequests): this.type = {
+  /**
+   * Add task resource requests
+   * @param requests The detailed task resource requests, see [[TaskResourceRequest]]
+   * @return this.type
+   */
+  def taskRequire(requests: TaskResourceRequests): this.type = {
     _taskResources.putAll(requests.requests.asJava)
     this
   }
 
-  def clearExecutorResourceRequests(): this.type = {
-    _executorResources.clear()
-    this
-  }
-
-  def clearTaskResourceRequests(): this.type = {
-    _taskResources.clear()
-    this
-  }
-
   override def toString(): String = {
     "Profile executor resources: " +
       s"${_executorResources.asScala.map(pair => s"${pair._1}=${pair._2.toString()}")}, " +
       s"task resources: ${_taskResources.asScala.map(pair => s"${pair._1}=${pair._2.toString()}")}"
   }
 
-  def build: ResourceProfile = {
-    new ResourceProfile(executorResources, taskResources)
+  def build(): ResourceProfile = {
+    if (!Utils.isTesting) {
+      if (_taskResources.isEmpty) {
+        throw new SparkException("Empty task resource request.")
+      }
+      if (_executorResources.isEmpty) {
+        throw new SparkException("Empty executor resource request.")
+      }

Review comment:
       This seems like a new constraint, can you explain why?

##########
File path: 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 {

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

##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfileBuilder.scala
##########
@@ -17,71 +17,67 @@
 
 package org.apache.spark.resource
 
-import java.util.{Map => JMap}
 import java.util.concurrent.ConcurrentHashMap
 
 import scala.collection.JavaConverters._
 
+import org.apache.spark.SparkException
 import org.apache.spark.annotation.{Evolving, Since}
+import org.apache.spark.util.Utils
 
 
 /**
- * Resource profile builder to build a Resource profile to associate with an RDD.
- * A ResourceProfile allows the user to specify executor and task requirements for an RDD
- * that will get applied during a stage. This allows the user to change the resource
+ * Resource profile builder to build a [[ResourceProfile]] to associate with an RDD.
+ * A [[ResourceProfile]] allows the user to specify executor and task resource requirements
+ * for an RDD that will get applied during a stage. This allows the user to change the resource
  * requirements between stages.
  *
  */
 @Evolving
 @Since("3.1.0")
 class ResourceProfileBuilder() {
 
+  // Task resource requests that specified by users, mapped from resource name to the request.
   private val _taskResources = new ConcurrentHashMap[String, TaskResourceRequest]()
+  // Executor resource requests that specified by users, mapped from resource name to the request.
   private val _executorResources = new ConcurrentHashMap[String, ExecutorResourceRequest]()
 
-  def taskResources: Map[String, TaskResourceRequest] = _taskResources.asScala.toMap
-  def executorResources: Map[String, ExecutorResourceRequest] = _executorResources.asScala.toMap
-
-  /**
-   * (Java-specific) gets a Java Map of resources to TaskResourceRequest
-   */
-  def taskResourcesJMap: JMap[String, TaskResourceRequest] = _taskResources.asScala.asJava
-
   /**
-   * (Java-specific) gets a Java Map of resources to ExecutorResourceRequest
+   * Add executor resource requests
+   * @param requests The detailed executor resource requests, see [[ExecutorResourceRequests]]
+   * @return this.type
    */
-  def executorResourcesJMap: JMap[String, ExecutorResourceRequest] = {
-    _executorResources.asScala.asJava
-  }
-
-  def require(requests: ExecutorResourceRequests): this.type = {
+  def executorRequire(requests: ExecutorResourceRequests): this.type = {
     _executorResources.putAll(requests.requests.asJava)
     this
   }
 
-  def require(requests: TaskResourceRequests): this.type = {
+  /**
+   * Add task resource requests
+   * @param requests The detailed task resource requests, see [[TaskResourceRequest]]
+   * @return this.type
+   */
+  def taskRequire(requests: TaskResourceRequests): this.type = {

Review comment:
       We might need to mention this in the migration guide then yeah? Personally I'd rather deprecate the old ones and point to the new than delete the API but I understand it's evolving.

##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfileBuilder.scala
##########
@@ -17,71 +17,67 @@
 
 package org.apache.spark.resource
 
-import java.util.{Map => JMap}
 import java.util.concurrent.ConcurrentHashMap
 
 import scala.collection.JavaConverters._
 
+import org.apache.spark.SparkException
 import org.apache.spark.annotation.{Evolving, Since}
+import org.apache.spark.util.Utils
 
 
 /**
- * Resource profile builder to build a Resource profile to associate with an RDD.
- * A ResourceProfile allows the user to specify executor and task requirements for an RDD
- * that will get applied during a stage. This allows the user to change the resource
+ * Resource profile builder to build a [[ResourceProfile]] to associate with an RDD.
+ * A [[ResourceProfile]] allows the user to specify executor and task resource requirements
+ * for an RDD that will get applied during a stage. This allows the user to change the resource
  * requirements between stages.
  *
  */
 @Evolving
 @Since("3.1.0")
 class ResourceProfileBuilder() {
 
+  // Task resource requests that specified by users, mapped from resource name to the request.
   private val _taskResources = new ConcurrentHashMap[String, TaskResourceRequest]()
+  // Executor resource requests that specified by users, mapped from resource name to the request.
   private val _executorResources = new ConcurrentHashMap[String, ExecutorResourceRequest]()
 
-  def taskResources: Map[String, TaskResourceRequest] = _taskResources.asScala.toMap
-  def executorResources: Map[String, ExecutorResourceRequest] = _executorResources.asScala.toMap
-
-  /**
-   * (Java-specific) gets a Java Map of resources to TaskResourceRequest
-   */
-  def taskResourcesJMap: JMap[String, TaskResourceRequest] = _taskResources.asScala.asJava
-
   /**
-   * (Java-specific) gets a Java Map of resources to ExecutorResourceRequest
+   * Add executor resource requests
+   * @param requests The detailed executor resource requests, see [[ExecutorResourceRequests]]
+   * @return this.type
    */
-  def executorResourcesJMap: JMap[String, ExecutorResourceRequest] = {
-    _executorResources.asScala.asJava
-  }
-
-  def require(requests: ExecutorResourceRequests): this.type = {
+  def executorRequire(requests: ExecutorResourceRequests): this.type = {
     _executorResources.putAll(requests.requests.asJava)
     this
   }
 
-  def require(requests: TaskResourceRequests): this.type = {
+  /**
+   * Add task resource requests
+   * @param requests The detailed task resource requests, see [[TaskResourceRequest]]
+   * @return this.type
+   */
+  def taskRequire(requests: TaskResourceRequests): this.type = {
     _taskResources.putAll(requests.requests.asJava)
     this
   }
 
-  def clearExecutorResourceRequests(): this.type = {
-    _executorResources.clear()
-    this
-  }
-
-  def clearTaskResourceRequests(): this.type = {
-    _taskResources.clear()
-    this
-  }

Review comment:
       Clear could be useful in a builder no?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774440621


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/134953/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-782576564


   **[Test build #135300 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/135300/testReport)** for PR 31496 at commit [`96d0760`](https://github.com/apache/spark/commit/96d07604b79c52bba4ff3aa4b892f363e73b2faa).


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Ngone51 commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
Ngone51 commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-781074838


   Thanks for all the feedback. I've updated PR to include only 2 API changes(Seq -> Array and build -> build()) as @tgravescs agreed. Please take another look, thanks! 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Ngone51 commented on a change in pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #31496:
URL: https://github.com/apache/spark/pull/31496#discussion_r578147035



##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfileBuilder.scala
##########
@@ -17,71 +17,67 @@
 
 package org.apache.spark.resource
 
-import java.util.{Map => JMap}
 import java.util.concurrent.ConcurrentHashMap
 
 import scala.collection.JavaConverters._
 
+import org.apache.spark.SparkException
 import org.apache.spark.annotation.{Evolving, Since}
+import org.apache.spark.util.Utils
 
 
 /**
- * Resource profile builder to build a Resource profile to associate with an RDD.
- * A ResourceProfile allows the user to specify executor and task requirements for an RDD
- * that will get applied during a stage. This allows the user to change the resource
+ * Resource profile builder to build a [[ResourceProfile]] to associate with an RDD.
+ * A [[ResourceProfile]] allows the user to specify executor and task resource requirements
+ * for an RDD that will get applied during a stage. This allows the user to change the resource
  * requirements between stages.
  *
  */
 @Evolving
 @Since("3.1.0")
 class ResourceProfileBuilder() {
 
+  // Task resource requests that specified by users, mapped from resource name to the request.
   private val _taskResources = new ConcurrentHashMap[String, TaskResourceRequest]()
+  // Executor resource requests that specified by users, mapped from resource name to the request.
   private val _executorResources = new ConcurrentHashMap[String, ExecutorResourceRequest]()
 
-  def taskResources: Map[String, TaskResourceRequest] = _taskResources.asScala.toMap
-  def executorResources: Map[String, ExecutorResourceRequest] = _executorResources.asScala.toMap
-
-  /**
-   * (Java-specific) gets a Java Map of resources to TaskResourceRequest
-   */
-  def taskResourcesJMap: JMap[String, TaskResourceRequest] = _taskResources.asScala.asJava
-
   /**
-   * (Java-specific) gets a Java Map of resources to ExecutorResourceRequest
+   * Add executor resource requests
+   * @param requests The detailed executor resource requests, see [[ExecutorResourceRequests]]
+   * @return this.type
    */
-  def executorResourcesJMap: JMap[String, ExecutorResourceRequest] = {
-    _executorResources.asScala.asJava
-  }
-
-  def require(requests: ExecutorResourceRequests): this.type = {
+  def executorRequire(requests: ExecutorResourceRequests): this.type = {
     _executorResources.putAll(requests.requests.asJava)
     this
   }
 
-  def require(requests: TaskResourceRequests): this.type = {
+  /**
+   * Add task resource requests
+   * @param requests The detailed task resource requests, see [[TaskResourceRequest]]
+   * @return this.type
+   */
+  def taskRequire(requests: TaskResourceRequests): this.type = {

Review comment:
       That's a good idea, but I have reverted this change as @tgravescs objects. We probably could make it in a separate PR later.

##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfileBuilder.scala
##########
@@ -17,71 +17,67 @@
 
 package org.apache.spark.resource
 
-import java.util.{Map => JMap}
 import java.util.concurrent.ConcurrentHashMap
 
 import scala.collection.JavaConverters._
 
+import org.apache.spark.SparkException
 import org.apache.spark.annotation.{Evolving, Since}
+import org.apache.spark.util.Utils
 
 
 /**
- * Resource profile builder to build a Resource profile to associate with an RDD.
- * A ResourceProfile allows the user to specify executor and task requirements for an RDD
- * that will get applied during a stage. This allows the user to change the resource
+ * Resource profile builder to build a [[ResourceProfile]] to associate with an RDD.
+ * A [[ResourceProfile]] allows the user to specify executor and task resource requirements
+ * for an RDD that will get applied during a stage. This allows the user to change the resource
  * requirements between stages.
  *
  */
 @Evolving
 @Since("3.1.0")
 class ResourceProfileBuilder() {
 
+  // Task resource requests that specified by users, mapped from resource name to the request.
   private val _taskResources = new ConcurrentHashMap[String, TaskResourceRequest]()
+  // Executor resource requests that specified by users, mapped from resource name to the request.
   private val _executorResources = new ConcurrentHashMap[String, ExecutorResourceRequest]()
 
-  def taskResources: Map[String, TaskResourceRequest] = _taskResources.asScala.toMap
-  def executorResources: Map[String, ExecutorResourceRequest] = _executorResources.asScala.toMap
-
-  /**
-   * (Java-specific) gets a Java Map of resources to TaskResourceRequest
-   */
-  def taskResourcesJMap: JMap[String, TaskResourceRequest] = _taskResources.asScala.asJava
-
   /**
-   * (Java-specific) gets a Java Map of resources to ExecutorResourceRequest
+   * Add executor resource requests
+   * @param requests The detailed executor resource requests, see [[ExecutorResourceRequests]]
+   * @return this.type
    */
-  def executorResourcesJMap: JMap[String, ExecutorResourceRequest] = {
-    _executorResources.asScala.asJava
-  }
-
-  def require(requests: ExecutorResourceRequests): this.type = {
+  def executorRequire(requests: ExecutorResourceRequests): this.type = {
     _executorResources.putAll(requests.requests.asJava)
     this
   }
 
-  def require(requests: TaskResourceRequests): this.type = {
+  /**
+   * Add task resource requests
+   * @param requests The detailed task resource requests, see [[TaskResourceRequest]]
+   * @return this.type
+   */
+  def taskRequire(requests: TaskResourceRequests): this.type = {
     _taskResources.putAll(requests.requests.asJava)
     this
   }
 
-  def clearExecutorResourceRequests(): this.type = {
-    _executorResources.clear()
-    this
-  }
-
-  def clearTaskResourceRequests(): this.type = {
-    _taskResources.clear()
-    this
-  }

Review comment:
       reverted




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-781190932


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/135219/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-781190932


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/135219/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774435857


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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Ngone51 commented on a change in pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #31496:
URL: https://github.com/apache/spark/pull/31496#discussion_r571390421



##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfile.scala
##########
@@ -242,20 +251,40 @@ class ResourceProfile(
 
 object ResourceProfile extends Logging {
   // task resources
+  /**
+   * built-in task resource: cpus
+   */
   val CPUS = "cpus"
   // Executor resources
   // Make sure add new executor resource in below allSupportedExecutorResources
+  /**
+   * built-in executor resource: cores
+   */
   val CORES = "cores"
+  /**
+   * built-in executor resource: cores
+   */
   val MEMORY = "memory"
+  /**
+   * built-in executor resource: offHeap
+   */
   val OFFHEAP_MEM = "offHeap"
+  /**
+   * built-in executor resource: memoryOverhead
+   */
   val OVERHEAD_MEM = "memoryOverhead"
+  /**
+   * built-in executor resource: pyspark.memory
+   */
   val PYSPARK_MEM = "pyspark.memory"
 
-  // all supported spark executor resources (minus the custom resources like GPUs/FPGAs)
-  val allSupportedExecutorResources = Seq(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)
+  /**
+   * All supported Spark built-in executor resources, custom resources like GPUs/FPGAs are excluded.
+   */
+  val allSupportedExecutorResources = Array(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)

Review comment:
       changed from `Seq` to `Array` for Java friendly purpose.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774590783


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/39551/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] srowen commented on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-782142097


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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774442742


   **[Test build #134954 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/134954/testReport)** for PR 31496 at commit [`d248c8d`](https://github.com/apache/spark/commit/d248c8d75b0a0299985565823a10c7b1116a4eb1).


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-781087483


   **[Test build #135219 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/135219/testReport)** for PR 31496 at commit [`ea08237`](https://github.com/apache/spark/commit/ea0823708f5e4e24e5846344575613775ed14033).


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-781848149






----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-774598906


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/134967/
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Ngone51 commented on a change in pull request #31496: [SPARK-34384][CORE] Add missing docs for ResourceProfile APIs

Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #31496:
URL: https://github.com/apache/spark/pull/31496#discussion_r579605057



##########
File path: core/src/main/scala/org/apache/spark/resource/ResourceProfile.scala
##########
@@ -242,17 +252,48 @@ class ResourceProfile(
 
 object ResourceProfile extends Logging {
   // task resources
+  /**
+   * built-in task resource: cpus
+   */
   val CPUS = "cpus"
   // Executor resources
   // Make sure add new executor resource in below allSupportedExecutorResources
+  /**
+   * built-in executor resource: cores
+   */
   val CORES = "cores"
+  /**
+   * built-in executor resource: cores
+   */
   val MEMORY = "memory"
+  /**
+   * built-in executor resource: offHeap
+   */
   val OFFHEAP_MEM = "offHeap"
+  /**
+   * built-in executor resource: memoryOverhead
+   */
   val OVERHEAD_MEM = "memoryOverhead"
+  /**
+   * built-in executor resource: pyspark.memory
+   */
   val PYSPARK_MEM = "pyspark.memory"
 
-  // all supported spark executor resources (minus the custom resources like GPUs/FPGAs)
-  val allSupportedExecutorResources = Seq(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)
+  private val _allSupportedExecutorResources =
+    Seq(CORES, MEMORY, OVERHEAD_MEM, PYSPARK_MEM, OFFHEAP_MEM)
+
+  /**
+   * Return all supported Spark built-in executor resources, custom resources like GPUs/FPGAs
+   * are excluded.
+   */
+  def allSupportedExecutorResources: Seq[String] = _allSupportedExecutorResources

Review comment:
       Array is mutable, as @mridulm mentioned too.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #31496: [SPARK-34384][CORE] API cleanup for ResourceProfile

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31496:
URL: https://github.com/apache/spark/pull/31496#issuecomment-781179692


   **[Test build #135219 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/135219/testReport)** for PR 31496 at commit [`ea08237`](https://github.com/apache/spark/commit/ea0823708f5e4e24e5846344575613775ed14033).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org