[GitHub] [spark] mridulm commented on a change in pull request #27583: [SPARK-29149][YARN] Update YARN cluster manager For Stage Level Scheduling

[GitBox <gi...@apache.org>]

mridulm commented on a change in pull request #27583: [SPARK-29149][YARN]  Update YARN cluster manager For Stage Level Scheduling
URL: https://github.com/apache/spark/pull/27583#discussion_r380132387
 
 

 ##########
 File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAllocator.scala
 ##########
 @@ -142,22 +150,31 @@ private[yarn] class YarnAllocator(
   } else {
     0
   }
-  // Number of cores per executor.
-  protected val executorCores = sparkConf.get(EXECUTOR_CORES)
+  // Number of cores per executor for the default profile
+  protected val defaultExecutorCores = sparkConf.get(EXECUTOR_CORES)
 
 
 Review comment:
   I am wondering if we want to make the locking semantics more formal in this class.
   Earlier, it was volatiles and concurrent hashmap (or sets back by concurrent hashmap) to eliminate need for locking - but lot of state changes were in context of 'this' being synchronized.
   
   Do we want to make sure all changes are guarded by a lock now ? Either use 'this' everywhere or some explicit private lock object and mark it via "\@GuardedBy"
   It is becoming slightly difficult too reason about the MT-safety of this class.
   Do you have any thoughts Tom ?

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


With regards,
Apache Git Services

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